New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixing a couple of held locks during exception handling #2966
Conversation
- making sure exceptions thrown during construction of a local_priority_queue_os_executor get propagated properly
@hkaiser This fixes the issue, thanks! |
@@ -292,7 +292,7 @@ namespace hpx { namespace threads { namespace detail | |||
<< " failed with: " << e.what(); | |||
|
|||
// trigger the barrier | |||
pool_threads -= thread_num; | |||
pool_threads -= (thread_num + 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you make this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without this change the loop below tries to wait for one too many thread. thread_num
gets incremented only after the thread has successfully started, so any exception thrown during thread startup will see thread_num
not reflecting the current (failed) thread.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it be rather --pool_threads
then? thread_num + 1
itself might decrease the value too much, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the difference between
pool_threads -= (thread_num + 1);
and
--pool_threads;
pool_threads -= thread_num;
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant just --pool_threads
, not both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That will not work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, missed that the catch block is covering the loop as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I take that as an approval of this PR...
@chinz07 please verify whether this fixes your issue