Reading and writing using the same critical section object

Question

I need to write a class that reads and writes to a file. When I do a write operation, reading should not happen, and vice versa. Can I use one critical section object for this? 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.

Answer 1

If you are talking about the same file for reading / writing, you need to use the same critical section (as you did at the moment), otherwise one stream can read the file and the other stream writes to it, which is exactly what you use to avoid critical sections.

However, as your FileWorker file is currently written, you can read / write arbitrary files but have one global critical section. This still works in this scenario, but ultimately it will add extra overhead if you rarely have to fight for the same file. This may be an acceptable compromise for you, depending on your usage patterns and how important time is for this code.

In addition, as Behemoth pointed out, you will have problems with one global critical section if you create two FileWorkers files with overlapping lifetimes. You will need something similar to what was suggested to make sure that you are not trying to initialize a critical section that has already been initialized, or to delete the one that has already been deleted.

Answer 2

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

To implement locking, I would recommend writing a small helper class to support fixed 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 as follows:

 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 RAII .

Answer 3

To protect a shared resource, you need the same critical section for all threads. The code is fine except for the constructor and destructor. This code leads to undefined behavior:

 FileWorker a; FileWorker b; 

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

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

When you need to synchronize data on each file, the right way is to implement an abstraction on top of your I / O files (HANDLE, FILE * s, std :: fstream, etc.). For instance.

 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 cannot be copied or assigned, this is important because a critical section must be copied.