DeleteCriticalSection causes exception

[radboudp]

Hi again,

In my now maybe well known service (see thread: Threads - how much time do they really use???) I have encountered a second problem in which I hope you guys can aid me. In return I am going to post to of my own base classes here that might be found usefull. :)

For critical sections (cs) I use a self build class that uses the low level OS calls for cs's:

--- Code ---

class CMyCritSection
{
public:
CMyCritSection();
virtual ~CMyCritSection();

// Attributes
public:
private:
CRITICAL_SECTION m_critSec;
int m_nLockCount;

// Methods
public:
void Lock(); // Locks the critical section
void Unlock(); // Releases the critical section

bool TryLock(); // Tries to lock the section. If it is not already locked, it takes ownership of the thread. If it is already locked it returns false without locking.

int GetLockCount() // Returns the number of times this object is locked...
{ return m_nLockCount; }

private:
};


//---------------------------------------------------------------------------
// Construction/Destruction

CMyCritSection::CMyCritSection()
: m_nLockCount( 0 )
{
::InitializeCriticalSection( &m_critSec );
}

CMyCritSection::~CMyCritSection()
{
try
{
::DeleteCriticalSection( &m_critSec );
}
catch ( CException *ex )
{
TCHAR szErrorStr[1024];
UINT uiHelpId = 0;

ex->GetErrorMessage( szErrorStr, 1024, &uiHelpId );

DWORD dwLastError = ::GetLastError();
_ASSERT( FALSE );
}
catch ( ... )
{
DWORD dwLastError = ::GetLastError();
_ASSERT( FALSE );
}
}

//---------------------------------------------------------------------------

void CMyCritSection::Lock()
{
::EnterCriticalSection( &m_critSec );
++ m_nLockCount;
}

void CMyCritSection::Unlock()
{
-- m_nLockCount;
::LeaveCriticalSection( &m_critSec );
}

bool CMyCritSection::TryLock()
{
bool bLocked = (( ::TryEnterCriticalSection( &m_critSec ) == 0 ) ? false : true );

if ( bLocked )
++ m_nLockCount;

return bLocked;
}

//---------------------------------------------------------------------------

--- End code ---

This class I use to protect many resources that are used in multiple threads. I have also used it to create a template class to protect basic attributes with it:

--- Code ---

//---------------------------------------------------------------------------

template< class TYPE >
class CSaveAttrib
{
// Constructor:
public:
CSaveAttrib()
{}
CSaveAttrib( TYPE val )
{ Set( val ); }
CSaveAttrib( CSaveAttrib<TYPE> &val )
{ Set( (TYPE) val ); }

// Attributes:
private:
TYPE m_value;
CMyCritSection m_lock;

// Methods:
public:
TYPE Get()
{ m_lock.Lock(); TYPE ret = m_value; m_lock.Unlock(); return ret; }
void Set( TYPE val )
{ m_lock.Lock(); m_value = val; m_lock.Unlock(); }

// Operators:
public:
// Locking...
void Lock() { m_lock.Lock(); }
void Unlock() { m_lock.Unlock(); }
bool TryLock() { return m_lock.TryLock(); }

// Returns a copy of the attribute
operator TYPE()
{ return Get(); }

// Assignment operator
TYPE operator= ( TYPE val )
{ m_lock.Lock(); TYPE ret = (m_value = val); m_lock.Unlock(); return ret; }

// Arithmic assignment operators
TYPE operator+= ( TYPE val )
{ m_lock.Lock(); TYPE ret = (m_value += val); m_lock.Unlock(); return ret; }
TYPE operator-= ( TYPE val )
{ m_lock.Lock(); TYPE ret = (m_value -= val); m_lock.Unlock(); return ret; }
TYPE operator*= ( TYPE val )
{ m_lock.Lock(); TYPE ret = (m_value *= val); m_lock.Unlock(); return ret; }
TYPE operator/= ( TYPE val )
{ m_lock.Lock(); TYPE ret = (m_value /= val); m_lock.Unlock(); return ret; }
TYPE operator%= ( TYPE val )
{ m_lock.Lock(); TYPE ret = (m_value %= val); m_lock.Unlock(); return ret; }
TYPE operator^= ( TYPE val )
{ m_lock.Lock(); TYPE ret = (m_value ^= val); m_lock.Unlock(); return ret; }
TYPE operator&= ( TYPE val )
{ m_lock.Lock(); TYPE ret = (m_value &= val); m_lock.Unlock(); return ret; }
TYPE operator|= ( TYPE val )
{ m_lock.Lock(); TYPE ret = (m_value |= val); m_lock.Unlock(); return ret; }

// Increment/decrement operators
TYPE operator++ ()
{ m_lock.Lock(); TYPE ret = (++ m_value); m_lock.Unlock(); return ret; }
TYPE operator-- ()
{ m_lock.Lock(); TYPE ret = (-- m_value); m_lock.Unlock(); return ret; }

TYPE operator++ ( int )
{ m_lock.Lock(); TYPE ret = (m_value ++); m_lock.Unlock(); return ret; }
TYPE operator-- ( int )
{ m_lock.Lock(); TYPE ret = (m_value --); m_lock.Unlock(); return ret; }

// Bitshift assignment operators
TYPE operator<<= ( TYPE val )
{ m_lock.Lock(); TYPE ret = (m_value <<= val); m_lock.Unlock(); return ret; }
TYPE operator>>= ( TYPE val )
{ m_lock.Lock(); TYPE ret = (m_value >>= val); m_lock.Unlock(); return ret; }

// Unary Arithmetic Operators
TYPE operator+ ()
{ m_lock.Lock(); TYPE ret = (+m_value); m_lock.Unlock(); return ret; }
TYPE operator- ()
{ m_lock.Lock(); TYPE ret = (-m_value); m_lock.Unlock(); return ret; }
TYPE operator! ()
{ m_lock.Lock(); TYPE ret = (!m_value); m_lock.Unlock(); return ret; }
TYPE operator~ ()
{ m_lock.Lock(); TYPE ret = (~m_value); m_lock.Unlock(); return ret; }
};

//---------------------------------------------------------------------------

--- End code ---

As I said I protect many things with it. My service has a global theService, much like the global theApp you get with a standard MFC application project. In the class of this service I have used CSaveAttrib<> many times (32 at present time). Some are statistics, others are flags etc... CMyCritSection is also used to protect list (MFC CPtrList), IO objects, etc...

Example:
CSaveAttrib<bool> m_bIsClosing;
CSaveAttrib<UINT> m_uiSentItems;
CSaveAttrib<bool> m_bIsActive;

Question one:
Do you guys feel that it is a problem to have many instances of my CMyCritSection??

Question two:
On destruction of the theService instance when the service is stopped I get an exception on two of these CSaveAttrib<> instances. The exception occurs in the ::DeleteCriticalSection() call in the destructor of CMyCritSection. Any clue why this may be?

Many thanks in advance!

-Radboud

[MrViggy]

Q2: Yeah, you've overloaded operator= in your template, but not in your critical section class. So, when that member of the template is copied, you now have two instances of your templated class pointing to the same exact CRITICAL_SECTION handle. I'm guessing that the exception is thrown when the second instance of your template is destructed, and it tries to delete an already deleted critical section.

Viggy

[OReubens]

'problems' in the critical section class:
1) The LockCount is somewhat overkill for a critical section. If the section is locked, it will be 1, if it isn't it will be 0. It will never have more than 1 lock (or you have an error).
2) ::DeleteCriticalSection() does not throw a CException*, so it's no use haviong a try/catch(CException*) around it.


You can really have as many critical sections as your app requires, so long as there is free memory.
However there is a practical side to this as well. Too many critical sections will slow down your code.
It really is somewhat of a dillemma. Not enough of them, and threads will wait on eachother too much. Too much of them and you waste memory, causing a bigger memory footprint (and more pages thus slower), and you waste CPU cycles on locking/unlocking critsections that aren't needed. The specific nature in which you defined CSaveAttrib leads me to believe you are having WAY too much of them...


You really need to ONLY protect pieces of code where multiple threads will have simultaneous unsafe access to the same objects. Note the specific definition here... 'simultaneous unsafe access'. If 100 threads only ever read a variable that never gets changed by any of them, you don't need any protection for it.
Even if one thread sometimes writes/changes the variable, it may not necessarily need synchronisation with the other threads. It depends what you're trying to achieve.
Most of the time, when I'm asked to review/debug/optimise a piece of multithreaded code someone else has written, I end up deleting more than half of all the synchronisations (because they aren't needed and are slowing down throughput), and quite often i have to conclude that the really critical synchronisations that are absolutely necessary aren't even there...




Your CSaveAttrib<> template class has an inherent flaw in it.
It will likely fail to work properly on any basic types (void, int, char, float, double pointers,...) It may also fail to work properly on some classes depending on how the class itself is implemented.
The type stored is not declared as volatile. Therefor, the compiler is free to optimize access to the type and may decide not to store/load the actual variable in between subsequent calls to the operators.

Anyway, using CSaveAttrib on integer types is somewhat of a bad idea anyway. Since it'll have unnecessary delays for the critical section. For integer types, you're better off using the Interlocked...() type functions.

[OReubens]

Q2: Yeah, you've overloaded operator= in your template, but not in your critical section class. So, when that member of the template is copied, you now have two instances of your templated class pointing to the same exact CRITICAL_SECTION handle. I'm guessing that the exception is thrown when the second instance of your template is destructed, and it tries to delete an already deleted critical section.


As far as I can see, this shouldn't be a problem here, since the = operator doesn't copy the m_lock variable, it only copies the type.

[OReubens]

As to the Q2...

My personal guess would be that you are somehow overwriting the memory where the cricical section is residing causing the DeleteCriticalSection to crash.

You could also be deleting a pointer to an allocated CSaveAttrib() more than once.

[MrViggy]

As far as I can see, this shouldn't be a problem here, since the = operator doesn't copy the m_lock variable, it only copies the type.
Yep. My mistake. :blush:

I need another cup of coffee!

Viggy