Reading and writing using same critical section object

Question

I need to write a class which reads from and writes to a file. When I do a write operation, read should not take place and also vice versa. Can I use a single critical section object for that? Like this:

FileWorker.h

class FileWorker
{
public:
    FileWorker();
    void WriteIntoFile(const char* fileName, const char* stringToWrite);
    void ReadFromFile(const char* fileName, char* stringToRead, int* stringLength);
    ~FileWorker();
};

FileWorker.cpp

#include <windows.h>
#include "FileWorker.h"

static CRITICAL_SECTION g_criticalSection;

FileWorker::FileWorker()
{
#ifdef WIN32APP
    InitializeCriticalSection(&g_criticalSection);
#endif
}

void FileWorker::ReadFromFile(const char *fileName, char *stringToRead, int *stringLength)
{
    EnterCriticalSection(&g_criticalSection);
    // Do Read
    LeaveCriticalSection(&g_criticalSection);
}

void FileWorker::WriteIntoFile(const char *fileName, const char *stringToWrite)
{
    EnterCriticalSection(&g_criticalSection);
    // Do Write
    LeaveCriticalSection(&g_criticalSection);
}

FileWorker::~FileWorker()
{
#ifdef WIN32APP
    DeleteCriticalSection(&g_criticalSection);
#endif
}

Thanks.

Mike O'Connor · Answer 1 · 2011-06-09T05:24:01.203

If you're talking about the same file for reading/writing, then you need to use the same critical section (as you have currently done), otherwise one thread could be reading the file, while another thread is writing to it, which is exactly what you are using the critical sections to avoid.

However, the way your FileWorker class is currently written, you can read/write arbitrary files, but have a single global critical section. That still works in this scenario, but it will end up adding extra overhead if there's rarely contention for the same file. This might be an acceptable tradeoff for you, depending on your usage patterns and how time critical this code is.

Also, as Begemoth pointed out, you will have issues with a single global critical section if you create two FileWorkers with overlapping lifetimes. You'll need something similar to what was proposed to make sure you don't try to initialize a critical section that's already been initialized, or delete one that's already been deleted.

@Kiril.. I guess Mike is emphasizing what I have done is right. — bdhar, Jun 09 '11 at 05:04
thanks for the comment. Can I use a named mutex based on the file name instead? so that the other files need not wait for the lock. — bdhar, Jun 09 '11 at 06:02

score 2 · Answer 2 · edited May 23 '17 at 12:26

As the other answers have pointed out, a single global critical section leads to problems when using multiple instances of FileWorker at once. You should make the critical section a member of FileWorker, and initialize/delete it in the constructor/destructor.

For the implementation of the locking, I would recommend writing a little helper class to support scoped locking:

class ScopedCriticalSection {
public:
    ScopedCriticalSection(CRITICAL_SECTION & criticalSection)
      : m_criticalSection(criticalSection)
    {
        EnterCriticalSection(&m_criticalSection);
    }

    ~ScopedCriticalSection() {
        LeaveCriticalSection(&m_criticalSection);
    }
private:
    CRITICAL_SECTION & m_criticalSection;
}

You can use this object like this:

void FileWorker::ReadFromFile(const char *fileName, char *stringToRead, 
                              int *stringLength)
{
    ScopedCriticalSection guard(m_criticalSection); // enters the cs
    // read stuff
} // the critical section is left when the guard is destroyed

To understand how this works, read about RAII.

Begemoth · Answer 3 · 2011-06-09T07:06:54.147

You need to the same critical section by all threads to protect the shared resource. The code is ok except constructor and destructor. This code results to undefined behavior:

FileWorker a;
FileWorker b;

becasuse the g_criticalSection is initialized twice without intermediate deletion. You need to add static member functions to initialize and finalize your class.

static void FileWorker::initialize()
{
  InitializeCriticalSection(&g_criticalSection);
}

static void FileWorker::finialize()
{
  DeleteCriticalSection(&g_criticalSection);
}

When you need to synchronize on per-file basis the right way is to implement an abstraction on top of you file I/O means (HANDLEs, FILE*s, std::fstream, etc). E.g.

class SyncronizedOutStream
{
  CRITICAL_SECTION cs;
  std::ofstream ostm;

  SyncronizedOutStream(const SyncronizedOutStream&);
  void operator= (const SyncronizedOutStream&);
public:
  explicit SyncronizedOutStream(const std::string& filename) 
    : ostm(filename.c_str())
  {
    InitializeCriticalSection(&cs);
  }

  ~SyncronizedOutStream()
  {
    DeleteCriticalSection(&cs);
  }

  template<typename T>
  SyncronizedOutStream& operator<< (T x)
  {
     ostm << x;
     return *this;
  }

  void lock()
  {
    EnterCriticalSection(&cs);
  }

  void release()
  {
    LeaveCriticalSection(&cs);
  }
};

Instances of this class are not copyable and not assignable this is important because critical section must be copied.

I don't think the stream output operator should be `const`. Also, when using any locking-mechanism, it is smart to bind the lock to the lifetime of an automatic variable. Why do you not lock the file directly in the output operator? — Björn Pollex, Jun 09 '11 at 07:05
@Space_C0wb0y yes const on the stream output operator is wrong, thanks. Locking on one output operation so fine grained that I do not think it is reasonable. I would rather use a separate guard class to protect series of the operator<< applications. — Begemoth, Jun 09 '11 at 07:11

Reading and writing using same critical section object

3 Answers3