Do I need to worry about a failure in these synchronization APIs?

[ahmd]

Say, I have the following code:

template <typename T> 
struct SYNCHED_DATA
{
    SYNCHED_DATA()
    {
        hMutex = ::CreateMutex(NULL, FALSE, NULL);
    }
    ~SYNCHED_DATA()
    {
        if(hMutex)
            CloseHandle(hMutex);
        hMutex = NULL;
    }

    void set(T* pV)
    {
        if(pV)
        {
            ::WaitForSingleObject(hMutex, INFINITE);
            var = *pV;
            ::ReleaseMutex(hMutex);
        }
    }
    void get(T* pV)
    {
        if(pV)
        {
            ::WaitForSingleObject(hMutex, INFINITE);
            *pV = var;
            ::ReleaseMutex(hMutex);
        }
    }

private:
    HANDLE hMutex;
    T var;

    SYNCHED_DATA(const SYNCHED_DATA& s)
    {
    }
    SYNCHED_DATA& operator = (const SYNCHED_DATA& s)
    {
    }
};

Do I need to worry about WaitForSingleObject() and/or ReleaseMutex() failing in this case?

[Igor Vartanov]

Sure you need. For example, your hMutex may get corrupted on some reason, so you'd need to catch that situation.

[Arjay]

Btw, why are you using a mutex instead of a critical section?

[ahmd]

Sure you need. For example, your hMutex may get corrupted on some reason, so you'd need to catch that situation.

Why would it get corrupted, Igor? The only way this could happen in my view is if I terminate a thread while the mutex is not released yet, but I don't event use the TerminateThread API...

Btw, why are you using a mutex instead of a critical section?

Several reasons:

1. This quote from MSDN for EnterCriticalSection API:

This function can raise EXCEPTION_POSSIBLE_DEADLOCK if a wait operation on the critical section times out.

Anything that has this kind of vague message would not be on my radar of APIs that I would use. Knowing Microsoft's track record I would not bet my software on something that may unexpectedly throw an exception.

2. It is not relevant for this example, but with WaitForSingleObject I can specify a timeout.

3. I can also use more than just one synchronization object with WaitFor...Object APIs.

4. With all of the above reasons it is easier to stick with the one well-trodden method for most of your synchronization stuff. That way your code becomes less error-prone. And I know how difficult it is to debug code that has a synch issue that a developer has overlooked.

[superbonzo]

Why would it get corrupted ?

it can be for any reason; for example, your code somewhere else could do something wrong corrupting your hmutex variable. In this case, the failure of WaitForSingleObject would not show the source of the error, but at least it would show that there's an error somewhere, instead of letting it pass away, waiting to bite you later. The more you can check runtime consistency the better it is, isn't it ?

BTW, note that "worrying" about those API failures doesn't necessarily mean doing something special when such a failure occurs. More specifically, if an error is non-recoverable then the only thing you can ( and should ) do is to let the program die ( eventually with some useful output for debug purposes ).

Anything that has this kind of vague message would not be on my radar of APIs that I would use.

why vague ? there's even a registry value for getting/setting the timeout parameter ... note that if you expect your SYNCHED_DATA accessors to lock for a long period of time then probably your synchronization design is flawed to begin with ...

[ahmd]

note that if you expect your SYNCHED_DATA accessors to lock for a long period of time then probably your synchronization design is flawed to begin with ...

superbonzo, what I learned is that if your synchronization lock is set up with a timeout you may be asking for trouble. A prolonged waiting period can come from such sources as a debugger, or an anti-virus software that intercepted an API and showed a user message. So in that situation a lock can last for hours or even days until a user responds to the message. The only time I would accept a timeout on the accessor lock is when the thread needs to quit (after say, an exit command.) In that case a timeout error result returned from a lock would indicate that synchronized data cannot be saved...

[superbonzo]

superbonzo, what I learned is that if your synchronization lock is set up with a timeout you may be asking for trouble.

ok, but a critical section is not really set up with a time out ( something that makes no sense for a critical section ). That "time out" appears to be a system-wide setting used to signal an exceptional deadlock condition that should not be handled by the program. Indeed, note that the default registry value is set to 30 days ! ( and according to a google search it seems that values >= 1 hours are simply ignored anyway ).
So, I don't think it's something you should worry about; if it were, then critical sections would be pointless ... yes, an especially evil/stupid user could always modify the registry value rendering your program faulty, but this is possible in so many other ways that it cannot be your responsability.

[OReubens]

Critical sections have their uses. So do mutexes.
It's your job as a programmer to pick the most appriopriate solution, not the most programmatically convenient one.

Critical sections are intended for what it's name implies. a "critical section of code" where you don't want 2 threads to enter the section of code that guarded by the CS object (note that you can have more than one bit of code guarded by the same CS object). So you would typically lock and unlock the CS in the same block of code as part of the normal flow of code through that code. A typical CS will be locked only for a very short amount of time.
This is what a CS is intended for AND what it is optimised for. In this (and only this) approach it will provide the best overall performance.

You _can_ use a CS as a more generic lock in a lock it somewhere, and unlock it in a totally different bit of code, but that's really not what you should be using a CS for, that's what mutexes are intended for.

Why does this matter ? Because a CS does NOT yield to another thread when the CS appears locked. Instead the CS assumes that it'll become available "soon" and starts spinning in a loop. If the CS does indeed come available, you win and code proceeds without a costly context switch. if it doesn't, you loose... because you waited in vain for the CS to become available.



Mutexes otoh are intended for a lock and release type approach on a shared resource and where it is expected that the lock will remain for more than the typical time in a timeslice.

You can use a mutex on a place where a CS would have worked. But you loose out in that case. a mutex is many many times slower to lock and release than a critical section. In addition, when the mutex is locked, the OS will typically yield te remaining timeslice immediately. if the lock is released "soon" after your attemt, you loose because you just lost the remaining time of your timeslice.


Learn to use CS and mutex appripriately, not because either one seems more programmatically convenient. They both have a specific purpose and different behaviour.

[Arjay]

Several reasons:

1. This quote from MSDN for EnterCriticalSection API:

Anything that has this kind of vague message would not be on my radar of APIs that I would use. Knowing Microsoft's track record I would not bet my software on something that may unexpectedly throw an exception.

2. It is not relevant for this example, but with WaitForSingleObject I can specify a timeout.

3. I can also use more than just one synchronization object with WaitFor...Object APIs.

4. With all of the above reasons it is easier to stick with the one well-trodden method for most of your synchronization stuff. That way your code becomes less error-prone. And I know how difficult it is to debug code that has a synch issue that a developer has overlooked.

As the other guys have stated, use the correct tool for the job. Neither type of synchronization is really appropriate given your description of holding a lock for hours or days waiting on a response from a user. Given this type of need, you'll probably want to use some sort of workflow engine like BizTalk, Windows Workflow Foundation, or roll your own using a database as persistent storage.

As far as a critical section, using RAII approach with a locker that automatically unlocks as it goes out of scope will eliminate most thread synchronization issues. Using RAII, you are guaranteed that the unlock always occurs (unlike your original code snippet). In addition, if you encapsulate your thread synchronization code within accessor methods of a class and only access the class through the accessor methods, you are going have some pretty bullet proof code that is easy to debug. On the other hand, if you have code where you are locking all over the place, the code will be fragile and difficult to debug (especially if your code doesn't use RAII).

[ahmd]

the CS assumes that it'll become available "soon" and starts spinning in a loop. If the CS does indeed come available, you win and code proceeds without a costly context switch. if it doesn't, you loose... because you waited in vain for the CS to become available.

Yeah, I forgot. It's a very good point about CS. I did some timing analysis and the wait in CS seems to run generally 10 times faster than the WaitForSingleObject on mutex. So if timing is important, then CS is the one to go with. Here's the test:

		#include <intrin.h>      //Needed for __rdtsc()

		ULONGLONG _uiCnt00[100] = {0};
		ULONGLONG _uiCnt01[100] = {0};
		ULONGLONG _uiCnt02[100] = {0};
		ULONGLONG _uiCnt03[100] = {0};
		ULONGLONG _uiCnt04[100] = {0};
		ULONGLONG _uiCnt05[100] = {0};

		ULONGLONG _uiCnt10[100] = {0};
		ULONGLONG _uiCnt11[100] = {0};
		ULONGLONG _uiCnt12[100] = {0};
		ULONGLONG _uiCnt13[100] = {0};
		ULONGLONG _uiCnt14[100] = {0};
		ULONGLONG _uiCnt15[100] = {0};
		ULONGLONG _uiCnt16[100] = {0};

		VERIFY(::SetThreadPriority(::GetCurrentThread(), THREAD_PRIORITY_TIME_CRITICAL));

		for(int i = 0; i < 100; i++)
		{
			CRITICAL_SECTION cs;
			_uiCnt00[i] = __rdtsc();
			::InitializeCriticalSection(&cs);
			_uiCnt01[i] = __rdtsc();
			::EnterCriticalSection(&cs);
			_uiCnt02[i] = __rdtsc();
			::EnterCriticalSection(&cs);
			_uiCnt03[i] = __rdtsc();
			::LeaveCriticalSection(&cs);
			_uiCnt04[i] = __rdtsc();
			::LeaveCriticalSection(&cs);
			_uiCnt05[i] = __rdtsc();
			::DeleteCriticalSection(&cs);

			_uiCnt10[i] = __rdtsc();
			HANDLE hMutex = ::CreateMutex(NULL, FALSE, NULL);

			_uiCnt11[i] = __rdtsc();
			::WaitForSingleObject(hMutex, INFINITE);
			_uiCnt12[i] = __rdtsc();
			::WaitForSingleObject(hMutex, INFINITE);
			_uiCnt13[i] = __rdtsc();
			::ReleaseMutex(hMutex);
			_uiCnt14[i] = __rdtsc();
			::ReleaseMutex(hMutex);
			_uiCnt15[i] = __rdtsc();

			::CloseHandle(hMutex);
			_uiCnt16[i] = __rdtsc();

			Sleep(1);
		}

		VERIFY(::SetThreadPriority(::GetCurrentThread(), THREAD_PRIORITY_NORMAL));

		ULONGLONG uiCnt00 = 0;
		ULONGLONG uiCnt01 = 0;
		ULONGLONG uiCnt02 = 0;
		ULONGLONG uiCnt03 = 0;
		ULONGLONG uiCnt04 = 0;
		ULONGLONG uiCnt05 = 0;

		ULONGLONG uiCnt10 = 0;
		ULONGLONG uiCnt11 = 0;
		ULONGLONG uiCnt12 = 0;
		ULONGLONG uiCnt13 = 0;
		ULONGLONG uiCnt14 = 0;
		ULONGLONG uiCnt15 = 0;
		ULONGLONG uiCnt16 = 0;

		for(int i = 0; i < 100; i++)
		{
			uiCnt00 += _uiCnt00[i];
			uiCnt01 += _uiCnt01[i];
			uiCnt02 += _uiCnt02[i];
			uiCnt03 += _uiCnt03[i];
			uiCnt04 += _uiCnt04[i];
			uiCnt05 += _uiCnt05[i];

			uiCnt10 += _uiCnt10[i];
			uiCnt11 += _uiCnt11[i];
			uiCnt12 += _uiCnt12[i];
			uiCnt13 += _uiCnt13[i];
			uiCnt14 += _uiCnt14[i];
			uiCnt15 += _uiCnt15[i];
			uiCnt16 += _uiCnt16[i];
		}

		uiCnt00 /= 100;
		uiCnt01 /= 100;
		uiCnt02 /= 100;
		uiCnt03 /= 100;
		uiCnt04 /= 100;
		uiCnt05 /= 100;

		uiCnt10 /= 100;
		uiCnt11 /= 100;
		uiCnt12 /= 100;
		uiCnt13 /= 100;
		uiCnt14 /= 100;
		uiCnt15 /= 100;
		uiCnt16 /= 100;


		_tprintf(_T("CriticalSection:\n")
			_T("InitializeCriticalSection = %I64d cycles\n")
			_T("EnterCriticalSection = %I64d cycles\n")
			_T("EnterCriticalSection = %I64d cycles\n")
			_T("LeaveCriticalSection = %I64d cycles\n")
			_T("LeaveCriticalSection = %I64d cycles\n")
			, 
			uiCnt01 - uiCnt00,
			uiCnt02 - uiCnt01,
			uiCnt03 - uiCnt02,
			uiCnt04 - uiCnt03,
			uiCnt05 - uiCnt04
			);


		_tprintf(_T("Mutex:\n")
			_T("CreateMutex = %I64d cycles\n")
			_T("WaitForSingleObject = %I64d cycles\n")
			_T("WaitForSingleObject = %I64d cycles\n")
			_T("ReleaseMutex = %I64d cycles\n")
			_T("ReleaseMutex = %I64d cycles\n")
			_T("CloseHandle = %I64d cycles\n")
			, 
			uiCnt11 - uiCnt10,
			uiCnt12 - uiCnt11,
			uiCnt13 - uiCnt12,
			uiCnt14 - uiCnt13,
			uiCnt15 - uiCnt14,
			uiCnt16 - uiCnt15
			);

and the results:

CriticalSection:
InitializeCriticalSection = 2490 cycles
EnterCriticalSection = 167 cycles
EnterCriticalSection = 232 cycles
LeaveCriticalSection = 79 cycles
LeaveCriticalSection = 190 cycles
Mutex:
CreateMutex = 5448 cycles
WaitForSingleObject = 2330 cycles
WaitForSingleObject = 1842 cycles
ReleaseMutex = 1753 cycles
ReleaseMutex = 2663 cycles
CloseHandle = 3717 cycles

[Arjay]

It might be more interesting if you ran your tests against multiple threads.

[ahmd]

It might be more interesting if you ran your tests against multiple threads.

Sorry, too busy. I'll leave it up to you, Arjay.

[Arjay]

Sorry, too busy. I'll leave it up to you, Arjay.

Try using RAII - it might free up some of the time spent getting multi-threaded programs to work.

Cheers.

[ahmd]

Try using RAII - it might free up some of the time spent getting multi-threaded programs to work.

Oh, yes, RAII. By the way, it couldn't 've been a worse named concept... speaking of confusing people with just its name Sure, it's a great practice but it's not a point here, Arjay. It should be applied on a "level higher" than my struct here, or in other words, if I use this template somewhere. Then including it, say in a class, will automatically invoke constructor and destructor.

[Arjay]

Oh, yes, RAII. By the way, it couldn't 've been a worse named concept... speaking of confusing people with just its name Sure, it's a great practice but it's not a point here, Arjay. It should be applied on a "level higher" than my struct here, or in other words, if I use this template somewhere. Then including it, say in a class, will automatically invoke constructor and destructor.

It kind of is the point because you don't seem to be using it.

4. With all of the above reasons it is easier to stick with the one well-trodden method for most of your synchronization stuff. That way your code becomes less error-prone. And I know how difficult it is to debug code that has a synch issue that a developer has overlooked.

If you use RAII, then you'll find your threading code is less error prone. Since you seem to be concerned with a deadlock with regard to a critical section, you might be missing the point of using RAII completely. If you use RAII and you keep the locks to a brief time, you won't get into deadlock issue due to not ever unlocking. If you do, you have a major flaw in your design (and you'll run into that during development).