1
\$\begingroup\$

I have an older class that I am working on and I'm updating it to remove "Windows" specific code. It had some "Thread Protection Code" that used CRITICAL_SECTION. I'm in the process of changing the use of CRITICAL_SECTION to use std::mutex and std::lock_guard. One thing to take note of is that my class does have static functions and this class is of a singleton type.

This is what my old code would have looked like.

-Logger.h-

#pragma once

#include "Singleton.h"

class Logger final : public Singleton {
public: 
    // enum here

private:
    // private variables not of concern

    CRITICAL_SECTION criticalSection_;

public:
    explicit Logger( const std::string& filename );
    virtual ~Logger();

    static void log( const std::string& text, LoggerType type );
    static void log( const std::ostringstream& stream, LoggerType type );
    static void log( const char* text, LoggerType type )
};

-Logger.cpp-

#include "Logger.h"

#include "BlockThread.h" // old class that would block threads - replacing with mutex lock_guard

#include "TextFileWriter.h" // file handler the Logger uses to write text files

// since this is a singleton type class as I only want a single logger per
// application run; here I use a static pointer to this class, the reason
// the log functions are static to begin with.
static Logger* spLogger_ = nullptr;

Logger::Logger( const std::string& filename ) :
Singleton( LOGGER ) {  // Base class takes an enum type of Singleton
    // variable initializations
    // ...
    // ------------------------

    InitializeCriticalSection( &criticalSection_ );
    BlockThread blockThread( criticalSection ); // Enter Critical Section

    // Start the log file
    TextFileWriter file( filename_, false, false ); //

    spLogger_ = this;
}

Logger::~Logger() {
    spLogger_ = nullptr;

    DeleteCriticalSection( &criticalSection_ );
}

void Logger::log( const std::string& text, LoggerType type ) {
    log( text.c_str(), type );
}

void Logger::log( const std::ostringstream& stream, LoggerType type ) {
    log( stream.str().c_str(), type );
}

void Logger::log( const char* text, LoggerType type ) {
    if ( nullptr = spLogger_ ) {
        // output error message
        return; 
    }

    BlockThread blockThread( spLogger->criticalSection_ ); // Enter Critical Section

    std::ostringstream stream;

    // setup logger's text formatting 

    // get the date and time 

    // push the date and time into the stream

    // push the text message into the stream

    // print stream to console

    try {
        TextFileWriter file (spLogger_->filename_, true, false );
        file.write( stream.str() );
    } catch( ... ) {
        // output error message failed to write to file.
    }
}

If you need to see the BlockThread class I'll show it here just incase...

-BlockThread.h-

#pragma once

class BlockThread final {
private:
    CRITICAL_SECTION* criticalSection_;

public:
    explicit BlockThread( CRITICAL_SECTION& criticalSection );
    ~BlockThread();
};

-BlockThread.cpp-

#include "BlockThread.h"

BlockThread::BlockThread( CRITICAL_SECTION& criticalSection ) {
    criticalSection_ = &criticalSection;
    EnterCriticalSection( criticalSection_ );
}

BlockThread::~BlockThread() {
    LeaveCriticalSection( criticalSection_ );
}

This was how my old classes were set up.


This is my attempt to remove BlockThread class and the CRITICAL_SECTION and its related functions by replacing them with std::mutex and std::lock_guard

-Logger.h-

#pragma once

#include "Singleton.h"

#include <mutex> // added this for mutex

class Logger final : public Singleton {
public:
    // public enum

private:
    // private members

    // CRITICAL_SECTION
    static std::mutex critical_;

public:
    // constructor, destructor and log function declarations same as above

};

-Logger.cpp-

#include "Logger.h"

static Logger* spLogger_ = nullptr;
std::mutex Logger::critical_{};

Logger::Logger( const std::string& filename ) {
    // init variables

    // InitializeCriticalSection( &criticalSection_ )
    // BlockThread blockThread( criticalSection_ );
    std::lock_guard<std::mutex> lock(critical_ );

    TextFileWriter file( filename, false, false );

    spLogger_ = this;
}

Logger::~Logger() {
    spLogger_ = nullptr;

    // DeleteCriticalSection( &criticalSection_ );

    // Left empty don't think I have to do anything since `lock_guard`
    // is destroyed once it leaves its scope...
}

// skip the first two log functions no difference here...

void Logger::log( const char* text, LoggerType type ) {
    // check if spLogger_ is null if so print error message & return

    // BlockThread blockThread( spLogger_->criticalSection_ ); // Enter Critical Section

    std::lock_guard<std::mutex> lock( spLogger_->criticalSection_ );

    // text formatting, date-time and message

    // print stream to console

    // try catch block same as above.
}

There's a set of questions that are sort of related; that's why I feel they are all important in this situation...

What I would like to know:

  • Is the replacement of my original CRITICAL_SECTION it's related functions and my original BlockThread class with std::mutex and std::lock_guard correct?
  • Will this behave in the same manner or fashion?
  • If not what would be the correct way to replace them as I'm not real familiar with std::mutex & std::lock_guard? - I'm trying to get a better grasp and understanding of how to use them properly.
  • Is there anything that I am missing, any corner cases or code smell such as possible data races, dead lock, or live lock?
  • -Note- I'm not really concerned with the commented out code as I know the original class works fine; I'm mainly concerned where I made the replacements.
  • What can I do to make this code follow modern C++ standards; to make it portable, cross-platform capable etc.

-Edit-

One of the major motivations for writing this question is that I have other classes in a decent size library that had previously used CRITICAL_SECTION its related functions and my old BlockThread class. Once I know how to properly replace them with std::mutex, std::lock_guard or any of their variations... Then I should easily be able to replace all thread safety features in my library.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Browse other questions tagged or ask your own question.