Fix crash in destructor of detached thread#2334
Fix crash in destructor of detached thread#2334dlang-bot merged 2 commits intodlang:stablefrom mihails-strasuns:fix-detach-termination
Conversation
mihails-strasuns
requested review from
Burgos,
MartinNowak and
PetarKirov
as code owners
|
Thanks for your pull request and interest in making D better, @mihails-strasuns! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please see CONTRIBUTING.md for more information. If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment. Bugzilla references
Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub fetch digger
dub run digger -- build "stable + druntime#2334" |
|
Looks like this clashes with internal thread attaching/detaching. Will investigate more. |
|
Right, so regular |
|
Updated with a less intrusive version which adds additional flag. Will add test case after #2333 is merged as they share similar pattern. |
|
Blocking deployment of analytic tools across the firm. |
| @@ -1860,6 +1868,8 @@ private: | |||
| return; | |||
There was a problem hiding this comment.
Seems that this was used previously to see if the thread was detached. You can either replace this
with if (t.m_detached == false) or use this in the destructor directly.
|
@mihails-strasuns Can you add a test case? Also there's a typo in your commit message ("alread"). |
|
Yup, was waiting for the other PR to get merged. Will update in a moment. |
|
Updated |
jacob-carlborg
left a comment
jacob-carlborg
left a comment
There was a problem hiding this comment.
Should contain a changelog entry or a reference to a bugzilla issue. Otherwise it won't appear in the changelog.
|
@jacob-carlborg |
|
The commit message needs to start with "Fix issue XXX" where "XXX" is the bugzilla issue. The dlang-bot will post a message in the PR when it has been properly connected to the bugzilla issue. |
|
Such a choosy bot :X |
This is needed for parsing the git commit messages of all repos for a release. See also: https://github.com/dlang/dlang-bot#automated-references |
|
My remark was about how limited are supported formats of mention :) I doubt that supporting few more common patterns would result in lot of false positives. |
n8sh
dismissed
jacob-carlborg’s stale review
Added reference to issue number as requested.
src/core/thread.d
Outdated
| { | ||
| if ( m_addr == m_addr.init ) | ||
| bool no_context = m_addr == m_addr.init; | ||
| bool not_registered = !next && !prev; |
There was a problem hiding this comment.
If there's only one registered thread its next and prev would also be null, but it appears to me that we don't need to worry about this because it could only happen in the destructor of the main thread.
There was a problem hiding this comment.
That can potentially happen when being called for C - main thread can be detached too. I have adjusted the PR and also fixed remove method to take that into account.
Fix issue 19314 Expectation is that after thread is detached from druntime, it becomes responsibility of external environment to terminate it properly (for example, by calling `join` from C/C++ code). However, `Thread` object for that thread will still remain and will eventually be collected by GC. Destructor tries to call `pthread_detach` unconditionally but it is undefined behaviour if thread was already joined before (see `man pthread_detach`).
Previously `remove` implementation would early return if removed thread is the only element in the registered list (and thus has no prev/next references)
8 participants
Part of https://issues.dlang.org/show_bug.cgi?id=19288
Expectation is that after thread is detached from druntime, it becomes
responsibility of external environment to terminate it properly (for
example, by calling
joinfrom C/C++ code).However,
Threadobject for that thread will still remain and willeventually be collected by GC. Destructor tries to call
pthread_detachunconditionally but it is undefined behaviour if thread was alread
joined before (see
man pthread_detach).This fix invalidates thread object when it is detached so that
destructor can't conflict with external thread management code.