Fix Issue 21784: Allow detached threads to be joined #3418

omerfirmak · 2021-03-29T12:03:56Z

This is useful when a thread terminates before the
caller is able to join(). With a reset m_addr, caller would segfault.

dlang-bot · 2021-03-29T12:04:12Z

Thanks for your pull request and interest in making D better, @omerfirmak! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
I have provided a detailed rationale explaining my changes
New or modified functions have Ddoc comments (with Params: and Returns:)

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

Auto-close	Bugzilla	Severity	Description
✓	21764	minor	checkaction=context doesn't work for empty tuples
✓	21784	enhancement	joining a detached thread results in segfault.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "stable + druntime#3418"

thewilsonator · 2021-03-29T12:15:28Z

Does this have a bugzilla issue number? Also a test would be nice.

omerfirmak · 2021-03-29T14:29:58Z

Does this have a bugzilla issue number?

I don't think there is one.

thewilsonator · 2021-03-30T01:05:47Z

I don't think there is one.

Please create one and title the commit message "Fix issue #####: Description of issue"

Geod24 · 2021-03-30T01:34:24Z

This is for the thread constructor issue ? Could you write a test too ? https://github.com/dlang/druntime/tree/master/test/thread/src

omerfirmak · 2021-03-30T10:05:20Z

All done

Geod24 · 2021-03-30T10:14:03Z

Now plz target stable and we're all good.

thewilsonator · 2021-03-30T10:15:31Z

ooh, stable good catch.

This is useful when a thread terminates before the caller is able to join().

omerfirmak · 2021-03-30T10:20:04Z

Ready

PetarKirov · 2021-03-31T04:34:37Z

src/core/thread/osthread.d

@@ -299,7 +299,8 @@ class Thread : ThreadBase
        }
        else version (Posix)
        {
-            pthread_detach( m_addr );
+            if (m_addr != m_addr.init)


Do we need to apply the same fix for version (Windows) above?

I don't have a windows machine to test with but if CloseHandle crashes with m_hndl.init as it's argument, we probably should.

If test is correct, then you should 'seen failures on the Windows CI pipelines.

It is fine then, AFAICS it should be triggering a CloseHandle call with m_hndl.init

PR dlang#3418 allowed them on Posix; extend it to Windows too. The added test/thread/src/join_detach.d test now passes on Windows (tested by LDC).

PR #3418 allowed them on Posix; extend it to Windows too. The added test/thread/src/join_detach.d test now passes on Windows (tested by LDC).

MoonlightSentinel · 2021-04-23T10:07:55Z

test/thread/src/join_detach.d

+    th.start();
+    sem.wait();
+    while (th.isRunning()) {}
+    destroy(th); // force detach


This triggers a spurious CI failure, e.g.:

*** Error in `./generated/linux/release/32/join_detach': double free or corruption (fasttop): 0xf6b004b8 *** ======= Backtrace: ========= /lib/i386-linux-gnu/libc.so.6(+0x67377)[0xf7612377] /lib/i386-linux-gnu/libc.so.6(+0x6d2f7)[0xf76182f7] /lib/i386-linux-gnu/libc.so.6(+0x6dc31)[0xf7618c31] ./generated/linux/release/32/join_detach(_D4core8internal9container6common8xreallocFNbNiPvkZQe+0x1e)[0x807d17e] ./generated/linux/release/32/join_detach(_D2rt5tlsgc7destroyFNbNiPvZv+0x8c)[0x807a93c] ./generated/linux/release/32/join_detach(_D4core6thread8osthread6Thread6__dtorMFNbNiZv+0x19)[0x8077f21] ./generated/linux/release/32/join_detach(rt_finalize2+0x58)[0x807fe88] ./generated/linux/release/32/join_detach(rt_finalize+0x1e)[0x8079d4e] ./generated/linux/release/32/join_detach(_D6object__T7destroyVbi1TC4core6thread8osthread6ThreadZQBrFNbQBkZv+0x1b)[0x807797f] ./generated/linux/release/32/join_detach(_Dmain+0x74)[0x8077928] ./generated/linux/release/32/join_detach(_D2rt6dmain212_d_run_main2UAAakPUQgZiZ6runAllMFZ9__lambda2MFZv+0x21)[0x8078f9d] ./generated/linux/release/32/join_detach(_D2rt6dmain212_d_run_main2UAAakPUQgZiZ7tryExecMFMDFZvZv+0x15)[0x8078e39] ./generated/linux/release/32/join_detach(_D2rt6dmain212_d_run_main2UAAakPUQgZiZ6runAllMFZv+0x8a)[0x8078f1a] ./generated/linux/release/32/join_detach(_D2rt6dmain212_d_run_main2UAAakPUQgZiZ7tryExecMFMDFZvZv+0x15)[0x8078e39] ./generated/linux/release/32/join_detach(_d_run_main2+0x193)[0x8078dc3] ./generated/linux/release/32/join_detach(_d_run_main+0x95)[0x8078c15] ./generated/linux/release/32/join_detach(main+0x1f)[0x807795f] /lib/i386-linux-gnu/libc.so.6(__libc_start_main+0xf7)[0xf75c3637] ./generated/linux/release/32/join_detach[0x80777c1]

This seems to be a race condition between the main and spawned thread both destroying the TLS. The current implementation has some measures to prevent double destruction, but those are not used in this case (and also not thread safe AFAICT).

Hence the explicit note in the docs:

[...] As thread manipulation is a required facility for garbage collection, all user threads should derive from this class, and instances of this class should never be explicitly deleted. [...]

omerfirmak requested review from andralex and wilzbach as code owners March 29, 2021 12:03

omerfirmak force-pushed the detach_join branch from 98d40e0 to aa6c62d Compare March 30, 2021 10:05

dlang-bot added the Enhancement New functionality label Mar 30, 2021

omerfirmak changed the title ~~Allow detached threads to be joined~~ Fix Issue 21784: Allow detached threads to be joined Mar 30, 2021

thewilsonator approved these changes Mar 30, 2021

View reviewed changes

omerfirmak changed the base branch from master to stable March 30, 2021 10:17

omerfirmak requested review from Burgos, CyberShadow, ibuclaw, MartinNowak, rainers, schveiguy and WalterBright as code owners March 30, 2021 10:17

Fix Issue 21784: Allow detached threads to be joined

b301faf

This is useful when a thread terminates before the caller is able to join().

omerfirmak force-pushed the detach_join branch from aa6c62d to b301faf Compare March 30, 2021 10:19

Geod24 approved these changes Mar 30, 2021

View reviewed changes

Geod24 added the auto-merge label Mar 30, 2021

dlang-bot merged commit c5eb299 into dlang:stable Mar 30, 2021

PetarKirov reviewed Mar 31, 2021

View reviewed changes

kinke added a commit to kinke/druntime that referenced this pull request Apr 4, 2021

Allow detached threads to be joined on Windows too

ee7a7ab

PR dlang#3418 allowed them on Posix; extend it to Windows too. The added test/thread/src/join_detach.d test now passes on Windows (tested by LDC).

kinke mentioned this pull request Apr 4, 2021

[stable] Allow detached threads to be joined on Windows too #3429

Merged

Geod24 pushed a commit that referenced this pull request Apr 4, 2021

Allow detached threads to be joined on Windows too

89e2232

PR #3418 allowed them on Posix; extend it to Windows too. The added test/thread/src/join_detach.d test now passes on Windows (tested by LDC).

omerfirmak deleted the detach_join branch April 5, 2021 03:37

MoonlightSentinel reviewed Apr 23, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Issue 21784: Allow detached threads to be joined #3418

Fix Issue 21784: Allow detached threads to be joined #3418

omerfirmak commented Mar 29, 2021 •

edited

dlang-bot commented Mar 29, 2021 •

edited

thewilsonator commented Mar 29, 2021

omerfirmak commented Mar 29, 2021 •

edited

thewilsonator commented Mar 30, 2021

Geod24 commented Mar 30, 2021

omerfirmak commented Mar 30, 2021

Geod24 commented Mar 30, 2021

thewilsonator commented Mar 30, 2021

omerfirmak commented Mar 30, 2021

PetarKirov Mar 31, 2021

omerfirmak Apr 1, 2021 •

edited

ibuclaw Apr 1, 2021

omerfirmak Apr 1, 2021

MoonlightSentinel Apr 23, 2021

MoonlightSentinel Apr 23, 2021 •

edited

Fix Issue 21784: Allow detached threads to be joined #3418

Fix Issue 21784: Allow detached threads to be joined #3418

Conversation

omerfirmak commented Mar 29, 2021 • edited

dlang-bot commented Mar 29, 2021 • edited

Bugzilla references

Testing this PR locally

thewilsonator commented Mar 29, 2021

omerfirmak commented Mar 29, 2021 • edited

thewilsonator commented Mar 30, 2021

Geod24 commented Mar 30, 2021

omerfirmak commented Mar 30, 2021

Geod24 commented Mar 30, 2021

thewilsonator commented Mar 30, 2021

omerfirmak commented Mar 30, 2021

PetarKirov Mar 31, 2021

Choose a reason for hiding this comment

omerfirmak Apr 1, 2021 • edited

Choose a reason for hiding this comment

ibuclaw Apr 1, 2021

Choose a reason for hiding this comment

omerfirmak Apr 1, 2021

Choose a reason for hiding this comment

MoonlightSentinel Apr 23, 2021

Choose a reason for hiding this comment

MoonlightSentinel Apr 23, 2021 • edited

Choose a reason for hiding this comment

omerfirmak commented Mar 29, 2021 •

edited

dlang-bot commented Mar 29, 2021 •

edited

omerfirmak commented Mar 29, 2021 •

edited

omerfirmak Apr 1, 2021 •

edited

MoonlightSentinel Apr 23, 2021 •

edited