| Date: | May 21, 2004 / year-entry #201 |
| Tags: | code |
| Orig Link: | https://blogs.msdn.microsoft.com/oldnewthing/20040521-00/?p=39233 |
| Comments: | 33 |
| Summary: | Continuing from yesterday, here's another case where you have to watch your destructors. Yesterday's theme was destructors that run at the wrong time. Today, we're going to see destructors that don't run at all! Assume there's an ObjectLock class which takes a lock in its constructor and releases it in its destructor. DWORD ThreadProc(LPVOID p)... |
Continuing from yesterday, here's another case where you have to watch your destructors. Yesterday's theme was destructors that run at the wrong time. Today, we're going to see destructors that don't run at all! Assume there's an ObjectLock class which takes a lock in its constructor and releases it in its destructor.
DWORD ThreadProc(LPVOID p)
{
... do stuff ...
ObjectLock lock(p);
... do stuff ...
return 0;
}
Pretty standard stuff. The first batch of stuff is done without the lock, and the second batch is done inside the lock. When the function returns, the lock is automatically released. But suppose somebody adds a little code to this function like this:
DWORD ThreadProc(LPVOID p)
{
... do stuff ...
ObjectLock lock(p);
...
if (p->cancelled) ExitThread(1);
...
return 0;
}
The code change was just to add an early exit if the object was cancelled. But when does that ObjectLock destructor run?
It runs at the Result: You left an object locked permanently. You can imagine how variations on this code could lead to resource leaks or other problems. |
Comments (33)
Comments are closed. |
And one calls ExitThread in the middle of your code in what valid circumstances?
This is just a variation on the KillThread problem.
This is the very reason why ExitThread() and their variations (_endthread(), AfxEndThread() et al) must completely be avoided IMO. Don’t call them. Never. Simply exit from the thread proc.
There seem to me few if any good reasons to ever do that; just return from the threadproc.
I suppose that this is more of a problem if one is using pthreads; because pthreads don’t directly support a return value one must (if one wishes to report a value) use pthread_exit() instead.
Of course, since one wraps their thread functions in typesafe wrappers /anyway/ this isn’t that big a deal; one writes one’s threadprocs as if they could return a value, and only in the wrapper (which is a small piece of carefully controlled code) does one call the dangerous thread-terminating function.
pthreads includes functions or macros for registration of scoped cleanup functions which will be called on pthread_exit. Most implementations use a language-independent exception mechanism and throw an exception that can’t be caught except by the internal function that calls the thread start function. So pthread_exit generally will call destructors.
If you want to exit the thread and free the library simultaneously (e.g., this is a silent worker thread) then you have to use FreeLibraryAndExitThread to exit your thread.
And even if you are careful and call it only at the end of your function, you still have this problem.
DWORD ThreadProc(LPVOID p)
{
… do stuff …
ObjectLock lock(p);
…
FreeLibraryAndExitThread(hinst, 0);
}
I see a theme brewing here… The mixing of paradigms.
I mean… first, incorrectly mixing automatic and manual scoping in the ATL sample, and now mixing automatic scoping with scope-breaking APIs.
I get your point, and they are definitely easy-to-make mistakes to make that can catch the happy careless programmer unaware, but the fact of the matter is that, depending on the point of view, they are ultimately caused by either:
1- "Bad" compiler implementatons that don’t properly expose the target OS APIs in a manner according to the rules of the language, or…
2- "Bad" OS APIs that don’t take into account modern language paradims like scoping rules. But the OS can’t be designed to cater to all foreseeable programming languages, so the closer you work to the OS, the more likely you’ll find yourself in muddy waters. That’s what frameworks and abstraction layers tend to isolate you from, but there’s also…
3- "Bad" frameworks that conform to a particular paradigm only partially. For instance, if the ATL has the CComPtr to help manage individual COM resources, but doesn’t have the CCoInitialize to manage the process-global resource, then ATL can be seen to be blamed. Of course, I guess the designers of ATL didn’t see the COM subsystem as a resource that needs (admittedly simple) management, thus forcing programmers to mix paradigms, so then it’s a problem with…
4- "Bad" programmers that mix paradigms without being careful about exactly how they mix them.
I just aim to put the problem into perspective, here. Both of the examples you show would have been avoidable gotchas had there been smarter design decisions somewhere down the chain before getting to trustung the programmer to know what they are doing. IMHO.
"And even if you are careful and call it only at the end of your function, you still have this problem. "
But, no, you can’t, because you write an appropriate wrapper, so that your ThreadProcs actually work in a way appropriate to C++.
Agreed that this can be fixed with "more technology" and by "not mixing models". But do people know to use this "more technology"? Do they know not to "mix models"? These are bugs I see in the real world. That’s why I write about them.
I agree that these are "real world" problems, and I agree that the threading problem is a relatively harder one, but… I mean… do we, for instance, even mention the possibility of this being a problem in the ATL documentation?
You can’t prevent people from doing bad things, especially with extra-flexible languages like C++, but at least we should cover all the bases and be wordy about those we can’t cover for some reason or other.
This isn’t so much an ATL problem as it is a C++ problem. Destructors run at end of scope. Sometimes that’s not when you want them to run.
ATL would have a comment like, "The XYZ class contains a destructor. Consequently, you must take care that the destructor runs at an appropriate time. Bypassing the destructor or running the destructor at the wrong time may lead to programming errors." Unclear whether that actually helps any.
Besides, is it even possible to make a list of the things you can’t do?
Well, the destructor will not run but a lock will be released, the underlying Win32 API releases any synchronization objects a thread holds when that thread terminates.
Sorry, this is Win32 code, I thought it was managed. That makes my argument about synchronization objects even more valid :)
The only sync object that is auto-released on thread death is the mutex. The others are just plain leaked. (Events and semaphores don’t have owners. Critical sections are not kernel objects.)
This is not a C++ issue, this is a process API issue. The C++ spec does not deal with the situation where your program disappears because of a function call (nor should it). It’s not even a destructor issue; it’s a general resource-leak thing.
In UNIX it’s common to call exit() to kill the program immediately, but it is treated only as a shortcut and most software will optionally be able to do a normal "return from main()" exit for leak-checking purposes.
Or, how about this little gem:
{
ObjectLock(p);
… // code expecting to be protected by p
} // dev expects ObjectLock(p) to dtor here
The compiler won’t complain. It just happily constructs the ObjectLock and then destructs it on the same line. Most developers will not see this.
Some groups at Microsoft have banned this practice and have tools to discover when they are in product code.
The joys of using a language where everything is obvious. Not.
Btw, Raymond’s absolutely right about how easy it is to screw this stuff up.
Quite literally the day after I posted the article that started this whole thing, I was doing a routine code review of the code one of the developers in my group was about to check in.
And what do you know, he used global CComPtr’s.
#define CONCAT_(a,b) a##b
#define CONCAT(a,b) CONCAT_(a,b)
#define ObjectLock(x) ObjectLock CONCAT(oBj3KtL0k,LINE) (x)
ObjectLock(p);
No muss, no fuss…
Oh wait, except for the fact that Microsoft still hasn’t fixed the LINE bug in 7 years. But you can use COUNTER to work around that.
Hm, works for me. I cut/pasted the above four lines (five if you count the blank line) into "foo.cpp" and typed
cl /EP foo.cpp
and got out
ObjectLock oBj3KtL0k5 (p);
I’m talking about this: http://support.microsoft.com/default.aspx?scid=kb;[LN];Q199057
Personally, I don’t believe that "Edit and Continue" belongs in a C/C++ compiler. If you want BASIC you know where to find it.
"Personally, I don’t believe that "Edit and Continue" belongs in a C/C++ compiler. If you want BASIC you know where to find it. "
That’s one of the dumbest things I’ve read on this weblog.
What /possible/ justification is there for this opinion? Either E&C is useful–in which case it should be everywhere possible–or it’s not, in which case it should be nowhere. Why do BASIC authors deserve an occasionally useful feature but not C++?
[quote]Agreed that this can be fixed with "more technology" and by "not mixing models". But do people know to use this "more technology"? Do they know not to "mix models"? These are bugs I see in the real world. That’s why I write about them.[/quote]
Mort probably doesn’t, but I don’t much care. Mort will never get things right because he’s a lazy cretin with no interest in doing his job properly.
This is why Mort needs to be offshored and why MS should stop dumbing things down to make them Mort-friendly.
I must not use message board markup on weblogs.
I must not use message board markup on weblogs.
I must not use message board markup on weblogs.
I must not use message board markup on weblogs.
I must not use message board markup on weblogs.
"If you use the intel compiler, get used to not having edit and continue. I rarely, if ever use this feature, and didn’t miss it one bit when I started using icl8."
Every time I’ve tried using icl, what I’ve missed most is a code generator that generates code that actually works.
"Raymond has a point. You should understand your code well enough to not need Edit and Continue."
It’s not about "understanding". It’s about being able to fix inconsequential typos so that you can actually look at how the important part of the code works.
"If you understand what is going on, you can generally create a stream of code, and when a bug occurs, look it over and know what to fix, rather than "Break/Adjust This/Continue/Break/Adjust that/Continue etc…""
The entire reason you use a debugger is because there is a disparity between your understanding of what is going on, and what’s actually going on. If there weren’t there’d be no bug, and hence no use for a debugger. As such, talk of "if you understand what is going on" is a rather feeble attempt at misdirection; that you /don’t/ understand what is going on is a prerequisite.
The problem with E&C is that people want to use it for NON trivial modifications – and that turns into a nightmare for the compiler guys.
Edit and Continue clearly doesn’t play friendly with LINE since editing changes the line numbers, and then the entire file needs to be recompiled – so much for incremental compilation.
Re. unnamed local RAII object bug:
"Some groups at Microsoft have banned this practice and have tools to discover when they are in product code."
..thereby avoiding one potential problem by creating others, especially in the presence of exceptions.
I’m having a hard time seeing how that is considered a solution. Do they actually consider manual resource management to be less error prone (on the whole) than using RAII in C++?
re: unnamed local RAII object bug
Actually, the Microsoft group ban the usage outside of call statements where they are pretty useful and the lifetime of the object lasts until the return from the call.
"Edit and Continue clearly doesn’t play friendly with LINE since editing changes the line numbers, and then the entire file needs to be recompiled – so much for incremental compilation. "
But this is in general no great hardship.
__LINE__ seems to be used for two things; debug messages and for emulating hygienic macros.
The former usage is rendered useless when one has E&C (one’s using a debugger to avoid having to do printf() debugging, after all). The latter usage is not all that effective (if you use the macro twice on one line you create duplicate names) and better replaced by VC++’s (admittedly non-standard) counter facility.
PingBack from http://computinglife.wordpress.com/2007/06/23/how-do-you-chose-your-apis/