I've taken the liberty of applying the patch at the end of this post to thread.cpp. When I was doing some work with boost::thread, I had to remove the catch(...) in my local copy because testing was a nightmare. Since then, I've been getting the M before thread.cpp every time I did a cvs update. The discussion about this catch(...) clause didn't end with a definitive decision one way or the other, so... let me be the one who actually does something about it. :-) Index: thread.cpp =================================================================== RCS file: /cvsroot/boost/boost/libs/thread/src/thread.cpp,v retrieving revision 1.21 diff -u -r1.21 thread.cpp --- thread.cpp 14 Jul 2005 15:46:50 -0000 1.21 +++ thread.cpp 6 Jul 2006 13:39:48 -0000 @@ -105,7 +105,7 @@ static OSStatus thread_proxy(void* param) #endif { - try +// try { thread_param* p = static_cast<thread_param*>(param); boost::function0<void> threadfunc = p->m_threadfunc; @@ -115,12 +115,12 @@ on_thread_exit(); #endif } - catch (...) - { -#if defined(BOOST_HAS_WINTHREADS) - on_thread_exit(); -#endif - } +// catch (...) +// { +//#if defined(BOOST_HAS_WINTHREADS) +// on_thread_exit(); +//#endif +// } #if defined(BOOST_HAS_MPTASKS) ::boost::detail::thread_cleanup(); #endif
Peter Dimov escreveu:
Index: thread.cpp =================================================================== RCS file: /cvsroot/boost/boost/libs/thread/src/thread.cpp,v retrieving revision 1.21 diff -u -r1.21 thread.cpp --- thread.cpp 14 Jul 2005 15:46:50 -0000 1.21 +++ thread.cpp 6 Jul 2006 13:39:48 -0000 @@ -105,7 +105,7 @@ static OSStatus thread_proxy(void* param) #endif { - try +// try { thread_param* p = static_cast<thread_param*>(param); boost::function0<void> threadfunc = p->m_threadfunc; @@ -115,12 +115,12 @@ on_thread_exit(); #endif } - catch (...) - { -#if defined(BOOST_HAS_WINTHREADS) - on_thread_exit(); -#endif - } +// catch (...) +// { +//#if defined(BOOST_HAS_WINTHREADS) +// on_thread_exit(); +//#endif +// } #if defined(BOOST_HAS_MPTASKS) ::boost::detail::thread_cleanup(); #endif
Shouldn't there be some sentry object then, with that "on_thread_exit()" thing called by the destructor? -- Pedro Lamarão
Pedro Lamarão wrote:
Shouldn't there be some sentry object then, with that "on_thread_exit()" thing called by the destructor?
If you allow an exception to escape from your thread function, the default behavior is for the runtime to call terminate(), so a missing on_thread_exit is the least of your worries. :-)
"Peter Dimov" <pdimov@mmltd.net> writes:
Pedro Lamarão wrote:
Shouldn't there be some sentry object then, with that
"on_thread_exit()" thing called by the destructor?
If you allow an exception to escape from your thread function, the default
behavior is for the runtime to call terminate(), so a missing on_thread_exit
is the least of your worries. :-)
Well, let's not forget that terminate() is a form of orderly shutdown. -- Dave Abrahams Boost Consulting www.boost-consulting.com
David Abrahams wrote:
"Peter Dimov" <pdimov@mmltd.net> writes:
Pedro Lamarão wrote:
Shouldn't there be some sentry object then, with that "on_thread_exit()" thing called by the destructor?
If you allow an exception to escape from your thread function, the default behavior is for the runtime to call terminate(), so a missing on_thread_exit is the least of your worries. :-)
Well, let's not forget that terminate() is a form of orderly shutdown.
It's easier to move the on_thread_exit call to a destructor than to argue about the precise meaning of orderly shutdown, so that's what I've done. :-) Index: thread.cpp =================================================================== RCS file: /cvsroot/boost/boost/libs/thread/src/thread.cpp,v retrieving revision 1.22 diff -u -r1.22 thread.cpp --- thread.cpp 6 Jul 2006 13:45:13 -0000 1.22 +++ thread.cpp 6 Jul 2006 19:45:57 -0000 @@ -94,6 +94,18 @@ bool m_started; }; +#if defined(BOOST_HAS_WINTHREADS) + +struct on_thread_exit_guard +{ + ~on_thread_exit_guard() + { + on_thread_exit(); + } +}; + +#endif + } // unnamed namespace extern "C" { @@ -107,13 +119,16 @@ { // try { +#if defined(BOOST_HAS_WINTHREADS) + + on_thread_exit_guard guard; + +#endif + thread_param* p = static_cast<thread_param*>(param); boost::function0<void> threadfunc = p->m_threadfunc; p->started(); threadfunc(); -#if defined(BOOST_HAS_WINTHREADS) - on_thread_exit(); -#endif } // catch (...) // {
At 10:50 PM +0300 7/6/06, Peter Dimov wrote:
David Abrahams wrote:
"Peter Dimov" <pdimov@mmltd.net> writes:
Pedro Lamarão wrote:
Shouldn't there be some sentry object then, with that "on_thread_exit()" thing called by the destructor?
If you allow an exception to escape from your thread function, the default behavior is for the runtime to call terminate(), so a missing on_thread_exit is the least of your worries. :-)
Well, let's not forget that terminate() is a form of orderly shutdown.
It's easier to move the on_thread_exit call to a destructor than to argue about the precise meaning of orderly shutdown, so that's what I've done. :-)
Quoting from Roland Schwarz, 8/28/05: "consider removing catch(...)" The only thing the catch all clause is doing is call the (somewhat superfluous) on_thread_exit() function on the windows platform, which currently only is used for TSS cleanup. Not sure about MPTASKS.) Removing the catch all at this point wouldn't change anything. In particular this would not stop unescaped throws from silently disappearing. An ending thread also does not normally stop the entire process doesn't it? I think the catch all is an artifact from the time before the below DLL_THREAD_DETACH was discovered. On windows the on_thread_exit is called by the operating system in either case: case DLL_THREAD_DETACH: { on_thread_exit(); break; } Thus, assuming I understood Roland correctly, on_thread_exit() was being called twice on Windows in the case of unhandled exceptions in a thread, once in the catch-all under discussion, and then again in the above clause (whose location I haven't researched), the latter making the former unnecessary. The same reasoning might lead one to conclude that this clause makes the on_thread_exit() in the thread destructor that Peter is adding also unnecessary. I don't know anything about Windows at this level, so can't evaluate the correctness of this reasoning though. BTW, I also have no idea what the consequences of multiple calls to on_thread_exit() might be, but Peter might want to consider that before making the destructor change. [Oh, I see the destructor change is already in cvs.] One more question: is this (IMO) bug fix going to make it onto the 1.34 branch? Please?
Kim Barrett wrote:
Thus, assuming I understood Roland correctly, on_thread_exit() was being called twice on Windows in the case of unhandled exceptions in a thread, once in the catch-all under discussion, and then again in the above clause (whose location I haven't researched), the latter making the former unnecessary.
The original code did call on_thread_exit in the non-exceptional case, too.
At 2:24 PM +0300 7/7/06, Peter Dimov wrote:
Kim Barrett wrote:
Thus, assuming I understood Roland correctly, on_thread_exit() was being called twice on Windows in the case of unhandled exceptions ...
The original code did call on_thread_exit in the non-exceptional case, too.
[checking cvs] Ah, yes. So you've maintained the old behavior of always calling on_thread_exit() on the way out of thread_proxy(). The email from Roland that I quoted raises the question of whether that's actually needed or even correct, but that's a different issue from the elimination of the annoying catch-all.
Kim Barrett wrote:
[checking cvs] Ah, yes. So you've maintained the old behavior of always calling on_thread_exit() on the way out of thread_proxy(). The email from Roland that I quoted raises the question of whether that's actually needed or even correct, but that's a different issue from the elimination of the annoying catch-all.
Unfortunately I was held off for some time by other tasks, so I did not see this thread earlier. Explanation of the double call: In case the automatic cleanup works the call in the proxy is not really necessary. The cleanup in this case even is called when threads are used that have not been started by as boost:threads i.e. native threads. But this is compiler dependent, and might break when new versions pop up, or e.g on borland. In this case the on_thread_exit call becomes mandatory. I.e. tss still behaves correctly for threads that have been launched via the boost::thread API. So on the one hand I would like to see the original behaviour restored. I agree on the other hand, that it is not very sensible having threads with uncaught exceptions dying silently. But putting it into a guard however is not an ideal solution since it requires, that on_thread_exit must not throw, which is a severe restriction, because on_thread exit is responsible to run the atexit handlers which might even call user provided code. And this might make it harder to find the original source of error if they throw. So, which other solutions are possible: 1) omit the on_thread_exit in case of uncaught exceptions. (and do away with the try clause) 2) rethrow ? 3) emit a debugging message (at least this is why I would want the catch(...) removed) 4) Forcibly shut down the entire process While 1) leaves the issue to the crt library I am not sure if this will give rise to implementation dependent behavior. At least the standard says that terminate must be called when an exception handler cannot be found. This undoubtedly is the case, but on the other hand the standard currently isn't about threads at all. But I think one can reasonably expect that the crt vendor took care of this case. Clause 2) is almost the same than 1) but the on_exit_thread executed. Whether this is a good thing depends. I am inclined to not. Putting a debug message as in 3) only creates overhead. At least the crt already can be expected to take care in a sensible way. And 4) most likely will render the debugger less effective. So my opt. is for 1). Roland
Roland Schwarz wrote:
So, which other solutions are possible: 1) omit the on_thread_exit in case of uncaught exceptions. (and do away with the try clause) 2) rethrow ? 3) emit a debugging message (at least this is why I would want the catch(...) removed) 4) Forcibly shut down the entire process
...
So my opt. is for 1).
FWIW, I agree.
On Thursday 06 July 2006 15:45, Peter Dimov wrote:
- try +// try { thread_param* p = static_cast<thread_param*>(param); boost::function0<void> threadfunc = p->m_threadfunc; @@ -115,12 +115,12 @@ on_thread_exit(); #endif } - catch (...) - { -#if defined(BOOST_HAS_WINTHREADS) - on_thread_exit(); -#endif - } +// catch (...) +// { +//#if defined(BOOST_HAS_WINTHREADS) +// on_thread_exit(); +//#endif +// }
This is really bad. It deactivates some code instead of removing it without the least comment why it is still there. What is the next person maintaining supposed to think? No, instead of doing this you should add a comment that says that a first approach (and an earlier version) used to catch(...) but that this effectively prevents invocation of the debugger and is therefore a bad idea. An exception coming up here is a programming mistake anyway and hiding this silently here is bad because it restricts the user's possibilities. And they can still catch(...) in their code. Also, not even catching(...), calling on_thread_exit() and rethrowing is an alternative because at that time the stack is already lost and if an exception propagates up there it would invoke std::terminate(). It thus prevents jumping into the debugger as noted above. Referring to another posting in this thread, where you try to achieve the same via a local object's destructor, there is possibly a mistake in that: AFAIK, when an exception is thrown and not caught, terminate() is invoked without(!) unwinding the stack, or am I confusing something? cheers Uli
participants (7)
-
David Abrahams -
Kim Barrett -
Pedro Lamarão -
Peter Dimov -
Roland Schwarz -
Ulrich Eckhardt -
Yuval Ronen