A Problem About singleton and CRITICAL_SECTION

[multiapple]

I decide to design a Log class which is thread-safe singleton.But there are some problems very annoying.When i use Mutex to lock,the log go fine.But when i use the CRITICAL_SECTION,the log will go wrong.And there is a very incredible phenomenon,the number of lock and unlock isn't same.The code is a little mess,maybe you can copy to your compiler.Here is my code:
#include<iostream>
#include<memory>
#include<fstream>
#include<Windows.h>
#include<string>
using namespace std;

#define MUTEX
#ifdef CRITICALSECTION
#define Mutex CRITICAL_SECTION
#define LOCK EnterCriticalSection
#define UNLOCK LeaveCriticalSection
#endif
#ifdef MUTEX
#define Mutex HANDLE
#define LOCK WaitForSingleObject
#define UNLOCK ReleaseMutex
#endif
class Lock
{
private:
Mutex _mutex;
public:
static int c,d;//In order to test the number of lock and unlock is the same
Lock(Mutex mutex):_mutex(mutex){
#ifdef MUTEX
LOCK(_mutex,INFINITE);
#endif
c++;
#ifdef CRITICALSECTION
LOCK(&_mutex);
#endif
}
~Lock(){
#ifdef MUTEX
UNLOCK(_mutex);
#endif
d++;
#ifdef CRITICALSECTION
UNLOCK(&_mutex);
#endif
}
};
int Lock::c = 0;
int Lock::d = 0;
class Log
{
private:
static Mutex m_mutex;
static ofstream m_log;
static Log* instance;
Log& operator = (Log&);
Log(Log&);
Log(){
#ifdef CRITICALSECTION
InitializeCriticalSection(&m_mutex);
#endif
};
public:
static Log& getinstance()
{
Lock m_lock(m_mutex);
if(!m_log.is_open())
m_log.open("server.log",ios::app);
return *instance;
}
Log& operator << (string s)
{
Lock m_lock(m_mutex);
m_log<<s.c_str()<<endl;
return *this;
}
};
#ifdef CRITICALSECTION
Mutex Log::m_mutex;
#endif
#ifdef MUTEX
Mutex Log::m_mutex = CreateMutex(NULL,false,"LOG");
#endif
Log* Log::instance = new Log();
ofstream Log::m_log;


DWORD WINAPI thread_1()
{
for(int i = 0;i < 20000;i++){
Log::getinstance()<<"here is thread_1";
}
return 0;
}
DWORD WINAPI thread_2()
{
for(int i = 0;i < 20000;i++){
Log::getinstance()<<"here is thread_2";
}
return 0;
}
DWORD WINAPI thread_3()
{
for(int i = 0;i < 20000;i++){
Log::getinstance()<<"here is thread_3";
}
return 0;
}
DWORD WINAPI thread_4()
{
for(int i = 0;i < 20000;i++){
Log::getinstance()<<"here is thread_4";
}
return 0;
}
int main()
{
HANDLE h1 = CreateThread(NULL,NULL,(LPTHREAD_START_ROUTINE)&thread_1,NULL,NULL,NULL);
HANDLE h2 = CreateThread(NULL,NULL,(LPTHREAD_START_ROUTINE)&thread_2,NULL,NULL,NULL);
HANDLE h3 = CreateThread(NULL,NULL,(LPTHREAD_START_ROUTINE)&thread_3,NULL,NULL,NULL);
HANDLE h4 = CreateThread(NULL,NULL,(LPTHREAD_START_ROUTINE)&thread_4,NULL,NULL,NULL);
WaitForSingleObject(h1,INFINITE);
WaitForSingleObject(h2,INFINITE);
WaitForSingleObject(h3,INFINITE);
WaitForSingleObject(h4,INFINITE);
cout<<Lock::c<<endl;
cout<<Lock::d<<endl;
return 0;
}

[multiapple]

There still has a another problem.When you put m_log.open("server.log",ios::app) in Log class construction function,the compile will yell at you.

[JLBorges]

Use standard C++ facilities, perhaps? (portable, debugged, robust code).
http://en.cppreference.com/w/cpp/thread

If counters to verify that the number of lock and unlock is the same are needed, they must be atomic.
http://en.cppreference.com/w/cpp/atomic

You may not want to keep the log file open. (possible loss of data on abnormal termination; though std::endl flushes the stream from C++, the os may still be caching the data).

Note: The Microsoft implementation of std::mutex and friends uses Win32 critical sections (performance).

Something like this, perhaps:

#include <iostream>
#include <fstream>
#include <mutex>
#include <future>
#include <string>
#include <vector>

class Log
{
    private:
        static std::mutex m_mutex;

    public:
        static Log& instance()
        {
            static Log singleton ;
            return singleton ;
        }

        Log& operator << ( std::string s )
        {
            std::lock_guard<std::mutex> guard(m_mutex) ;
            std::ofstream( "server.log", std::ios::app ) << s << '\n' ;
            return *this;
        }
};

std::mutex Log::m_mutex ;

Log& log_message( std::string msg ) { return Log::instance() << msg ; }

int main()
{
    std::vector< std::future<Log&> > futures ;

    for( int i = 0 ; i < 10 ; ++i )
    {
        futures.push_back( std::async( std::launch::async,
                                       log_message, "thread #" + std::to_string(i) ) ) ;
    }

    for( auto& future : futures ) future.get() ; // wait for threads to finish
    std::cout << std::ifstream( "server.log" ).rdbuf() ;
}

http://coliru.stacked-crooked.com/a/a7959fb1a6d65e73
http://rextester.com/TLB64112

[multiapple]

Tks very much.But i still confuse why mutex work but CRITICAL_SECTION doesn't.

[JLBorges]

> i still confuse why mutex work but CRITICAL_SECTION doesn't.

This is wrong.

#ifdef CRITICALSECTION
#define Mutex CRITICAL_SECTION
//...
#endif

// ...

class Lock
{
    // ...
   Mutex _mutex;
   // ... 
   Lock(Mutex mutex):_mutex(mutex) // **** copy initialise a critical section ???? ****
   { /* ... */ }

    // ...
};

A critical section object cannot be moved or copied. The process must also not modify the object, but must treat it as logically opaque. Use only the critical section functions to manage critical section objects.
https://msdn.microsoft.com/en-us/library/windows/desktop/ms683472(v=vs.85).aspx

Without making excessive use of the preprocessor:

#include <iostream>
#include <mutex>
#include <atomic>
#include <fstream>
#include <windows.h>
#include <future>
#include <string>
#include <vector>

 namespace win
 {
     template < typename T > struct basic_lock
     {
        void lock() { static_cast<T*>(this)->do_lock() ; ++lock_cnt ; }
        void unlock() { static_cast<T*>(this)->do_unlock() ; ++unlock_cnt ; }

        std::atomic<int> lock_cnt {};
        std::atomic<int> unlock_cnt {};

        basic_lock() = default ;
        basic_lock( const basic_lock& ) = delete ;
        basic_lock( basic_lock&& ) = delete ;
        basic_lock& operator= ( const basic_lock& ) = delete ;
        basic_lock& operator= ( basic_lock&& ) = delete ;
        ~basic_lock() = default ;
     };

     struct mutex : basic_lock<mutex>
     {
         mutex() : hm( ::CreateMutex( nullptr, false, nullptr ) ) {}
         ~mutex() { ::CloseHandle(hm) ; }

         void do_lock() { ::WaitForSingleObject( hm, INFINITE ) ; }
         void do_unlock() { ::ReleaseMutex(hm) ; }

         HANDLE hm  ;
     };

     struct critical_section : basic_lock<critical_section>
     {
         critical_section() { ::InitializeCriticalSection( std::addressof(crit_sec) ) ; }
         ~critical_section() { ::DeleteCriticalSection( std::addressof(crit_sec) ) ; }

         void do_lock() { ::EnterCriticalSection( std::addressof(crit_sec) ) ; }
         void do_unlock() { ::LeaveCriticalSection( std::addressof(crit_sec) ) ; ; }

         CRITICAL_SECTION crit_sec ;
     };
 }

template < typename LOCK_TYPE > struct Log
{
    static Log& instance()
    {
        static Log singleton ;
        return singleton ;
    }

    Log& operator << ( std::string s )
    {
        std::lock_guard<LOCK_TYPE> guard(lock) ;
        std::ofstream( "server.Log", std::ios::app ) << s << '\n' ;
        return *this;
    }

    LOCK_TYPE lock {} ;
};

template < typename LOCK_TYPE >
void log_message( std::string msg ) { Log<LOCK_TYPE>::instance() << msg ; }

template < typename LOCK_TYPE > void test_it()
{
    std::vector< std::future<void> > futures ;

    for( int i = 0 ; i < 10 ; ++i )
    {
        futures.push_back( std::async( std::launch::async,
                                       log_message<LOCK_TYPE>, "thread #" + std::to_string(i) ) ) ;
    }

    for( auto& future : futures ) future.wait() ; // wait for threads to finish
    std::cout << "#locks: " << Log<LOCK_TYPE>::instance().lock.lock_cnt
              << "    #unlocks: " << Log<LOCK_TYPE>::instance().lock.unlock_cnt << '\n' ;
}

int main()
{
    std::cout << "           win::mutex: " ; test_it<win::mutex>() ;
    std::cout << "win::critical_section: " ; test_it<win::critical_section>() ;
}

http://rextester.com/QPYQ58990