Seth asks how he can perform proper exception-handling cleanup if he doesn't know whether he needs to clean up a critical section.
I'm using SEH, and have some __try/__except blocks in which the code enters and leaves critical sections. If an exception occurs, I don't know if I'm currently in the CS or not. Even wrapping the code in __try/__finally wouldn't solve my problems.
Answer: You know whether you own the CRITICAL_SECTION because you entered it.
- Method 1: Deduce it from your instruction pointer.
- "If I'm at this line of code, then I must be in the critical section."
__try {
...
EnterCriticalSection(x);
__try { // if an exception happens here
... // make sure we leave the CS
} __finally { LeaveCriticalSection(x); }
...
} except (filter) {
...
}
Note that this technique is robust to nested calls to EnterCriticalSection. If you take the critical section again, then wrap the nested call in its own try/finally.
- Method 2: Deduce it from local state
- "I'll remember whether I entered the critical section."
int cEntered = 0;
__try {
...
EnterCriticalSection(x);
cEntered++;
...
cEntered--;
LeaveCriticalSection(x);
...
} except (filter) {
while (cEntered--)
LeaveCriticalSection(x);
...
}
Note that this technique is also robust to nested calls to EnterCriticalSection. If you take the critical section again, increment cEntered another time.
- Method 3: Track it in an object
- Wrap the CRITICAL_SECTION in another object.
This most closely matches what Seth is doing today.
class CritSec : CRITICAL_SECTION
{
public:
CritSec() : m_dwDepth(0), m_dwOwner(0)
{ InitializeCriticalSection(this); }
~CritSec() { DeleteCriticalSection(this); }
void Enter() { EnterCriticalSection(this);
m_dwDepth++;
m_dwOwner = GetCurrentThreadId(); }
void Leave() { if (!--m_dwDepth) m_dwOwner=0;
LeaveCriticalSection(this); }
bool Owned()
{ return GetCurrentThreadId() == m_dwOwner; }
private:
DWORD m_dwOwner;
DWORD m_dwDepth;
};
__try {
assert(!cs.Owned());
...
cs.Enter();
...
cs.Leave();
...
} except (filter) {
if (cs.Owned()) cs.Leave();
}
Notice that this code is not robust to nested critical sections (and correspondingly, Seth's code isn't either). If you take the critical section twice, the exception handler will only leave it once.
Note also that we assert that the critical section is not initially owned. If it happens to be owned already, then our cleanup code may attempt to leave a critical section that it did not enter. (Imagine if an exception occurs during the first "...".)
- Method 4: Track it in a smarter object
- Wrap the CRITICAL_SECTION in a smarter object.
Add the following method to the CritSec object above:
DWORD Depth() { return Owned() ? m_dwDepth : 0; }
Now you can be robust to nested critical sections:
DWORD dwDepth = cs.Depth();
__try {
...
cs.Enter();
...
cs.Leave();
...
} except (filter) {
while (cs.Depth() > dwDepth)
cs.Leave();
}
Note however that I am dubious of the entire endeavor that inspired the original question.
Cleaning up behind an exception thrown from within a critical section raises the issue of "How do you know what is safe to clean up?" You have a critical section because you are about to destabilize a data structure and you don't want others to see the data structure while it is unstable. But if you take an exception while owning the critical section - well your data structures are unstable at the point of the exception. Merely leaving the critical section will now leave your data structures in an inconsistent state, leading to harder-to-diagnose bugs later on. "How did my count get out of sync?"
More rants on exceptions in a future entry.
Exercise: Why don't we need to use synchronization to protect the uses of m_dwDepth and m_dwOwner?
Update 2004/Jan/16: Seth pointed out that I got the two branches of the ternary operator backwards in the Depth() function. Fixed.
Thanks very much for addressing this, Raymond. The last class looks like a good implementation.
My incorrect guess to the posted question is that since DWORD assignments involve only one assembler instruction, they are atomic and cannot be interrupted mid-execution. I’m assuming this is incorrect due to the existence of such functions as InterlockedIncrement().
This begs my next question: when do I need to protect data with a critical section? When multiple threads read the data? Or only when one or more threads write the data? Or only if it’s a complex data type such as a char buffer, where a read or write operation can be interrupted?
Thanks!…
You can make option more robust to nested critsecs if you change the exception handler to the following:
while (cs.Owned()) cs.Leave();
It seems like the answer to the exercise is a trick question. The m_dw* functions are inherently protected because you are doing them from within the critical section that the code already owns. The only exception is Owned() which, like Seth says, is atomic. But don’t let atomicity let you get sloppy, there are cache issues on MP systems you have to worry about.
The thing about "it’s only one asm instruction, so it can’t be interrupted" doesn’t hold on multiple-CPU boxes, though. A CS is not needed in the sample code because those member vars are accessed only between calls to EnterCriticalSection() and LeaveCriticalSection(), so you know that only one thread is in a position to be accessing those vars.
As for when to use a CS: If all you’re doing is reading, no CS is needed (assuming your underlying data store is thread-safe). As soon as you write to it, you need to lock it. If you have a copy of _Advanced Windows_ there’s a nice chunk of code in there called a SWMRG (single writer, multiple reader guard) that demonstrates many multithreaded concepts.
“Why don’t we need to use synchronization to protect the uses of m_dwDepth and m_dwOwner?”
It seems to me they are protected by *this CRITICAL_SECTION.
Mike Dunn: Actually, we read from m_dwOwner from outside the critical section, but we’ll see below why this is safe anyway.
Changes to m_dwOwner and m_dwDepth are protected by *this. m_dwOwner is read from outside the critical section, but that’s okay, because if you don’t own the critical section, then m_dwOwner is not you, and even if it changes, it will just change to something else that is still not you, so the test against GetCurrentThreadId will still fail. m_dwDepth is in turn protected by m_dwOwner.
Note that using "while (cs.Owner()) cs.Leave()" will cause problems if the critical section entered your code already owned; you’ll be leaving critical sections that you never entered. That’s what method 4 there to fix.
But I still don’t like any of this – there’s more to cleaning up behind an exception than just leaving critical sections. All you’re doing is delaying the crash to a point where you can’t debug it any more.
Knowing that you still don’t like any of this, I’m surprised you didn’t mention this popular method:
class LockSection
{
CRITICAL_SECTION *cs;
public:
LockSection(CRITICAL_SECTION *cs)
: cs(cs)
{
EnterCriticalSection(cs);
}
~LockSection()
{
LeaveCriticalSection(cs);
}
};
__try
{
LockSection lock(&mycs);
//stuff
}
except
{
// critical section left from stack unwinding
}
// critical section left from going out of scope.
Yes, that’s a common method, and it’s fine if you like it. You do run into problems if you want to exit the critical section at a point other than the end of the block that declared the LockSection object. E.g., if you want to exit the critical section, do something, then re-enter the critical section.
There are legitimate reasons why you would want to throw exceptions from within a critical section. Raymond brought up the case where a thread lock the CS in order to prevent others from seeing its modifications in progress (write lock), but he didn’t mention the case where a thread locks to prevent others from modifying the data structure (read lock). An example would be cloning objects off a shared list and encountering an allocation error in the middle. Throwing out of a read lock is perfectly safe and I find it frequently occurs in multithreaded, exception-based code.
If you really want to get fancy, add an operator int() const { return 0; } method, and then this macro:
#define synchronized(lock) switch(LockSection locksection=(lock))default:
This then allows you to emulate Java’s synchronized statement, which IMO makes for much more readable code. It even works without braces around the controlled code. The downside is that this particular incarnation crashes every version of Visual C++ since at least 6.0, requiring an if() workaround.
You can simply open a new scope if you want to leave the critical section earlier. Example:
// do stuff before entering CS
{
LockSection lock(&cs);
// stuff inside CS
}
// stuff after leaving CS
Joe,
Your code produces warning C4509 and error C2712 – Cannot use __try in functions that require object unwinding. Do you compile your apps with /GX- to overcome this?
The "new scope" trick has two problems, one aesthetic and one technical.
The aesthetic problem is that "magic happens" at the close-brace and it isn’t obvious. Somebody might later decide to remove the "redundant" braces and break your code, unaware that twenty lines into the block, you created a LockSection object. This can be fixed with some coding conventions (for example, always comment the close-brace with "// exit lock()").
The technical problem is that this still restricts you critical section use that matches the ambient block structure. Consider:
cs.Enter();
for (p = m_first; p; p = p->m_next) {
p->prepare();
cs.Leave();
result = Callback(p);
cs.Enter();
if (p->revalidate()) {
use(result);
}
}
cs.Leave();
The entry/exit of the critical section here does not match the block structure of the surrounding code.
"Who would be so crazy as to write code like this?"
Answer: Anybody who supports callbacks. If you call out to uncontrolled code while holding a critical section, you risk deadlocking. You must leave the critical section, call the callback, then re-enter the critical section and revalidate all active variables.
Heh, to be honest I haven’t done much C++ exception programming under windows lately, and I’ve specifically never done the above (I’ve used it plenty for multiple exit points in a function) so I don’t know how to work around it. Sorry.
My guess is you have to use a different exception method? From that message it doesn’t seem like the compiler can handle it. It could be that it requires RTTI to function and you have it turned off.
Joe’s solution is about what i am using myself.
But since this is a C++ solution, you have to use C++ exception handling.
Instead of __try etc. use try and catch.
Back when I was coding in C++ I used the LockSection method a lot. I think it’s the most natural approach in C++. If you want to leave and reenter critical section at arbitrary places you can add Leave() and Enter() methods to the lock class. Or you could create an UnlockSection class that would unlock in constructor and lock in destructor.<p>As for callbacks, another approach is to lock, clone the list of listeners into a local variable, unlock then invoke the callbacks using the copy.
"lock, clone the list of listeners into a local variable, unlock then invoke the callbacks using the copy"
Yes, you can do this. And even in this case, you need to able to unlock temporarily while inside a nested lock.
cs.Lock();
while (condition) {
.. blah blah();
.. if (need to make callback) {
.. .. list = copy_callback_list();
.. .. cs.Unlock(); // << nested unlock
.. .. callback_each_entry(list);
.. .. free(list);
.. .. cs.Lock(); // <<re-lock
.. }
}
cs.Unlock();
Be careful if you unlock the critical section. I did that in some code (to call out to a callback, exactly as suggested). If the callback returned an error, the code exited the routine without reacquiring the lock. The destructor was stupid and assumed that it owned the lock, so it simply called LeaveCriticalSection. So the execution sequence (from the point of view of the critical section) was
EnterCriticalSection
LeaveCriticalSection
LeaveCriticalSection
Needless to say, this was not good for the health of my application.