CODEWITHSUNDEEP
c++multithreadingfilefile-writing
Vladislav Rakovskiy
Vladislav Rakovskiy

Reputation: 3

c++: Writing to a file in multithreaded program

So I have multiple threads writing to the same file by calling Log::write method.

class Log
{
private:
    ofstream log;
    string file_path;
public:
    Log(string);
    void write(string);
};
Log::Log(string _file_path)
{
    file_path=_file_path;
}
void Log::write(string str)
{
    EnterCriticalSection(&CriticalSection);
    log.open(file_path.c_str(),std::ofstream::app);
    log<<str+'\n';
    log.close();
    LeaveCriticalSection(&CriticalSection);
}

Is it safe if threads will call Log::write method of the same object at the same time?

Upvotes: 0

Views: 3686

Answers (1)

David Haim
David Haim

Reputation: 26476

Your code is wasteful and does not follow C++ idioms.

Starting from the end : yes, write is thread safe, because win32 CRITICAL_SECTION protects it from concurrent modifications.

although:

  1. why open and close the stream each time? this is very wasteful thing to do. open the stream in the constructor and leave it open. the destructor will deal with closing the stream.

  2. if you want to use Win32 critical section, at least make it RAII safe. make a class which wraps a reference to critical section, locking it in a constructor and unlocking it in the destructor. this way even if an exception is thrown - you are guaranteed that the lock will be unlocked.

  3. where is the deceleration of CriticalSection anyway? it should be a member of Log.

  4. are you aware of std::mutex?

  5. why are you passing strings by value? it is very un-efficient. pass then by const reference.

  6. you use snake_case for some of the variables (file_path) but upper camel case for other (CriticalSection). use the same convention.

  7. str is never a good name for a string variable, and the file stream is not a log. is the thing that does the actual logging. logger is a better name. in my correction is just named it m_file_stream.

Corrected code:

class Log
{

private:

    std::mutex m_lock;
    std::ofstream m_file_stream;
    std::string m_file_path;

public:

    Log(const std::string& file_path);
    void write(const std::string& log);
};

Log::Log(const std::string& file_path):
   m_file_path(file_path)
{
     m_file_stream.open(m_file_path.c_str());
     if (!m_file_stream.is_open() || !m_file_stream.good())
     {
        //throw relevant exception.
     }
 }

 void Log::write(const std::string& log)
 {
    std::lock_guard<std::mutex> lock(m_lock);
    m_file_stream << log << '\n';
 }

Upvotes: 3

Related Questions