3

We are a couple of newbies in MFC and we are building a multi-threded application. We come across the article in the URL that warns us not to use CCriticalSection since its implementation is broken. We are interested to know if anyone has any experience in using CCriticalSection and do you come across any problems or bugs? Is CCriticalSection usable and production ready if we use VC++ 2008 to build our application?

http://www.flounder.com/avoid_mfc_syncrhonization.htm

thx

umtang41
  • 33
  • 1
  • 3

2 Answers2

11

I think that article is based on a fundamental misunderstanding of what CSingleLock is for and how to use it.

You cannot lock the same CSingleLock multiple times, but you are not supposed to. CSingleLock, as its name suggests, is for locking something ONCE.

Each CSingleLock just manages one lock on some other object (e.g. a CCriticalSection which you pass it during construction), with the aim of automatically releasing that lock when the CSingleLock goes out of scope.

If you want to lock the underlying object multiple times you would use multiple CSingleLocks; you would not use a single CSingleLock and try to lock it multiple times.

Wrong (his example):

CCriticalSection crit;
CSingleLock lock(&crit);
lock.Lock();
lock.Lock();
lock.Unlock();
lock.Unlock();

Right:

CCriticalSection crit;
CSingleLock lock1(&crit);
CSingleLock lock2(&crit);
lock1.Lock();
lock2.Lock();
lock2.Unlock();
lock1.Unlock();

Even better (so you get RAII):

CCriticalSection crit;
// Scope the objects
{
    CSingleLock lock1(&crit, TRUE); // TRUE means it (tries to) locks immediately.
    // Do stuff which needs the lock (if IsLocked returns success)
    CSingleLock lock2(&crit, TRUE);
    // Do stuff which needs the lock (if IsLocked returns success)
}
// crit is unlocked now.

(Of course, you would never intentionally get two locks on the same underlying critical section in a single block like that. That'd usually only happen as a result of calling functions which get a lock while inside something else that already has its own lock.)

(Also, you should check CSingleLock.IsLocked to see if the lock was successful. I've left those checks out for brevity, and because they were left out of the original example.)

If CCriticalSection itself suffers from the same problem then that certainly is a problem, but he's presented no evidence of that that I can see. (Maybe I missed something. I can't find the source to CCriticalSection in my MFC install to verify that way, either.)

Leo Davidson
  • 6,093
  • 1
  • 27
  • 29
  • His understanding of what the locks are supposed to do seem to be based on a comp-sci background or implementations of similarily-named locks from other platforms and/or implementations, based on the tone of his argument. – Arafangion Dec 06 '10 at 12:24
  • 1
    He's mistaken CSingleLock for the critical section itself. CSingleLock manages one lock (and provides RAII for it), not the entire critical section. – Leo Davidson Dec 06 '10 at 12:27
  • Now, you absolutely need another +1, but you've already gotten mine! :( – Arafangion Dec 06 '10 at 12:54
  • I think his rant was due to the fact that both the mutant (aka mutex) object and critical section are natively recursive on Windows, while MFC's wrapper puts a restriction on this. – wj32 Dec 07 '10 at 09:28
  • MFC's wrapper does not put a restriction on recursion, as far as I can see, at least in the critical section case (I haven't looked at the mutex case much). You should be able to lock a CCriticalSection as many times as you want, just like a native CRITICAL_SECTION. On the other hand, each CSingleLock instance should only be used for a single lock. You can point multiple CSingleLocks at the same CCriticalSection so this is not a problem. CSingleLock is not a Critical Section; it is a lock on one. I don't use MFC but have my own Critical Section RAII wrapper similarly to CSingleLock. – Leo Davidson Dec 07 '10 at 10:08
0

That article suggests that a simple situation of using those primitives are fine, except that the implementation of them violates the semantics that should be expected of them.

basically, it suggests that if you use it as a non-recursive lock, where you take care to always ensure that the lock is valid (ie, not abandoned), then you should be fine.

The article does complain, however, that the limitations are inexcusable.

Arafangion
  • 11,517
  • 1
  • 40
  • 72