Reading and writing using same critical section object

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::~FileWo开发者_StackOverflowrker()
{
#ifdef WIN32APP
    DeleteCriticalSection(&g_criticalSection);
#endif
}

Thanks.

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.

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.

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.