From ace29318866adee00144a15531d9d884f22e8a09 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ali=20=C3=87ehreli?= Date: Mon, 11 Dec 2017 03:36:45 -0800 Subject: [PATCH 1/4] Ensure Thread.remove() is called only when slock is held --- src/core/thread.d | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/core/thread.d b/src/core/thread.d index cbf719b05a..b53d648db2 100644 --- a/src/core/thread.d +++ b/src/core/thread.d @@ -2239,6 +2239,9 @@ version( Windows ) */ extern (C) void thread_detachThis() nothrow @nogc { + Thread.slock.lock_nothrow(); + scope(exit) Thread.slock.unlock_nothrow(); + if (auto t = Thread.getThis()) Thread.remove(t); } @@ -2257,6 +2260,9 @@ extern (C) void thread_detachThis() nothrow @nogc */ extern (C) void thread_detachByAddr( ThreadID addr ) { + Thread.slock.lock_nothrow(); + scope(exit) Thread.slock.unlock_nothrow(); + if( auto t = thread_findByAddr( addr ) ) Thread.remove( t ); } @@ -2265,6 +2271,9 @@ extern (C) void thread_detachByAddr( ThreadID addr ) /// ditto extern (C) void thread_detachInstance( Thread t ) nothrow @nogc { + Thread.slock.lock_nothrow(); + scope(exit) Thread.slock.unlock_nothrow(); + Thread.remove( t ); } From ce0e3062c3429d51ef9e7854269e6a443eaba159 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ali=20=C3=87ehreli?= Date: Mon, 11 Dec 2017 15:34:13 -0800 Subject: [PATCH 2/4] Issue 18063 - Do not pthread_detach non-D threads --- src/core/thread.d | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/src/core/thread.d b/src/core/thread.d index b53d648db2..d988e1f582 100644 --- a/src/core/thread.d +++ b/src/core/thread.d @@ -617,8 +617,11 @@ class Thread } else version( Posix ) { - pthread_detach( m_addr ); - m_addr = m_addr.init; + if (m_isOwned) + { + pthread_detach( m_addr ); + m_addr = m_addr.init; + } } version( Darwin ) { @@ -726,6 +729,7 @@ class Thread if( pthread_create( &m_addr, &attr, &thread_entryPoint, cast(void*) this ) != 0 ) onThreadError( "Error creating thread" ); } + m_isOwned = true; } version( Darwin ) { @@ -1536,6 +1540,7 @@ private: version( Posix ) { shared bool m_isRunning; + bool m_isOwned; } bool m_isDaemon; bool m_isInCriticalRegion; @@ -2111,8 +2116,10 @@ extern (C) bool thread_isMainThread() nothrow @nogc /** - * Registers the calling thread for use with the D Runtime. If this routine - * is called for a thread which is already registered, no action is performed. + * Registers the calling thread for use with the D Runtime. If this routine is + * called for a thread which is already registered, no action is performed. On + * Posix systems, the D Runtime does not take ownership of the thread; + * specifically, it does not call $(D pthread_detach) during cleanup. * * NOTE: This routine does not run thread-local static constructors when called. * If full functionality as a D thread is desired, the following function @@ -2145,6 +2152,7 @@ extern (C) Thread thread_attachThis() thisContext.tstack = thisContext.bstack; atomicStore!(MemoryOrder.raw)(thisThread.m_isRunning, true); + thisThread.m_isOwned = false; } thisThread.m_isDaemon = true; thisThread.m_tlsgcdata = rt_tlsgc_init(); From 20f76d3acd5f1870003aa37044fa09c1e5ef9eb5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ali=20=C3=87ehreli?= Date: Mon, 11 Dec 2017 03:41:54 -0800 Subject: [PATCH 3/4] Fix issue 18063 - thread_attachThis returns dangling pointer --- src/core/thread.d | 21 +++++ test/thread/Makefile | 7 +- test/thread/src/attach_detach.d | 160 ++++++++++++++++++++++++++++++++ 3 files changed, 187 insertions(+), 1 deletion(-) create mode 100644 test/thread/src/attach_detach.d diff --git a/src/core/thread.d b/src/core/thread.d index d988e1f582..45956a4041 100644 --- a/src/core/thread.d +++ b/src/core/thread.d @@ -2121,6 +2121,12 @@ extern (C) bool thread_isMainThread() nothrow @nogc * Posix systems, the D Runtime does not take ownership of the thread; * specifically, it does not call $(D pthread_detach) during cleanup. * + * Threads registered by this routine should normally be deregistered by $(D + * thread_detachThis). Although $(D thread_detachByAddr) and $(D + * thread_detachInstance) can be used as well, such threads cannot be registered + * again by $(D thread_attachThis) unless $(D thread_setThis) is called with the + * $(D null) value first. + * * NOTE: This routine does not run thread-local static constructors when called. * If full functionality as a D thread is desired, the following function * must be called after thread_attachThis: @@ -2251,7 +2257,10 @@ extern (C) void thread_detachThis() nothrow @nogc scope(exit) Thread.slock.unlock_nothrow(); if (auto t = Thread.getThis()) + { Thread.remove(t); + t.setThis(null); + } } @@ -2347,6 +2356,18 @@ extern (C) void thread_setThis(Thread t) nothrow @nogc Thread.setThis(t); } +unittest +{ + auto t = new Thread( + { + auto old = Thread.getThis(); + assert(old !is null); + thread_setThis(null); + assert(Thread.getThis() is null); + thread_setThis(old); + }).start; + t.join(); +} /** * Joins all non-daemon threads that are currently running. This is done by diff --git a/test/thread/Makefile b/test/thread/Makefile index ca27d1fb86..e4dc5570b7 100644 --- a/test/thread/Makefile +++ b/test/thread/Makefile @@ -1,6 +1,6 @@ include ../common.mak -TESTS:=fiber_guard_page +TESTS:=fiber_guard_page attach_detach .PHONY: all clean all: $(addprefix $(ROOT)/,$(addsuffix .done,$(TESTS))) @@ -11,6 +11,11 @@ $(ROOT)/fiber_guard_page.done: $(ROOT)/%.done : $(ROOT)/% $(QUIET)$(TIMELIMIT)$(ROOT)/$* $(RUN_ARGS); rc=$$?; [ $$rc -eq 139 ] || [ $$rc -eq 138 ] @touch $@ +$(ROOT)/attach_detach.done: $(ROOT)/%.done : $(ROOT)/% + @echo Testing $* + $(QUIET)$(TIMELIMIT)$(ROOT)/$* + @touch $@ + $(ROOT)/%: $(SRC)/%.d $(QUIET)$(DMD) $(DFLAGS) -of$@ $< diff --git a/test/thread/src/attach_detach.d b/test/thread/src/attach_detach.d new file mode 100644 index 0000000000..55345c0206 --- /dev/null +++ b/test/thread/src/attach_detach.d @@ -0,0 +1,160 @@ +version (Posix) +{ + +import core.thread; +import core.sys.posix.pthread; +import core.stdc.stdlib; +import core.stdc.time; + +// This program creates threads that are started outside of D runtime and +// stresses attaching and detaching those threads to the D runtime. + +struct MyThread +{ + pthread_t t; + bool stopRequested; +} + +enum totalThreads = 4; + +enum runTimeSeconds = 5; // Must be less than timelimit's +MyThread[totalThreads] threads; + +auto exerciseGC() { + int[] arr; + foreach (i; 0 .. 1000) + arr ~= i; + return arr; +} + +// This represents an API function of a non-D framework. Since we don't have any +// control on the lifetime of this thread, we have to attach upon entry and +// detach upon exit. +void api_foo() +{ + auto t = thread_attachThis(); + scope(exit) + { + // Pick a detachment method + final switch (rand() % 3) + { + case 0: + thread_detachThis(); + break; + case 1: + thread_detachByAddr(t.id); + // thread_setThis must be called by the detached thread; it happens + // to be the case in this test. + thread_setThis(null); + break; + case 2: + thread_detachInstance(t); + // thread_setThis must be called by the detached thread; it happens + // to be the case in this test. + thread_setThis(null); + break; + } + } + + assert_thread_is_attached(t.id); + cast(void)exerciseGC(); +} + +// Make calls to an api function and exit when requested +extern(C) void * thread_func(void * arg) +{ + MyThread *t = cast(MyThread*)arg; + + while (!t.stopRequested) + api_foo(); + + return arg; +} + +void start_thread(ref MyThread t) +{ + pthread_attr_t attr; + int err = pthread_attr_init(&attr); + assert(!err); + + t.stopRequested = false; + err = pthread_create(&t.t, &attr, &thread_func, cast(void*)&t); + assert(!err); + + err = pthread_attr_destroy(&attr); + assert(!err); +} + +void start_threads() +{ + foreach (ref t; threads) + start_thread(t); +} + +void stop_thread(ref MyThread t) +{ + t.stopRequested = true; + const err = pthread_join(t.t, null); + assert(!err); + + assert_thread_is_gone(t.t); +} + +void stop_threads() +{ + foreach (ref t; threads) + stop_thread(t); +} + +void assert_thread_is_attached(pthread_t tid) +{ + size_t found = 0; + foreach (t; Thread.getAll()) + if (tid == t.id) + { + ++found; + } + assert(found == 1); +} + +void assert_thread_is_gone(pthread_t tid) +{ + foreach (t; Thread.getAll()) + assert(tid != t.id); +} + +// Occasionally stop threads and start new ones +void watch_threads() +{ + const start = time(null); + + while ((time(null) - start) < runTimeSeconds) + { + foreach (ref t; threads) + { + const shouldStop = ((rand() % 100) == 0); + if (shouldStop) + { + stop_thread(t); + start_thread(t); + } + } + } +} + +void main() +{ + start_threads(); + watch_threads(); + stop_threads(); +} + +} // version (Posix) +else +{ + +void main() +{ +} + +} From cd6355e9bccb6ba27ed51cfcf8978ed7881f0f7e Mon Sep 17 00:00:00 2001 From: Andrei Alexandrescu Date: Tue, 12 Dec 2017 13:14:57 -0500 Subject: [PATCH 4/4] A few doc adjustments --- src/core/thread.d | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/core/thread.d b/src/core/thread.d index 45956a4041..2a5a58ac90 100644 --- a/src/core/thread.d +++ b/src/core/thread.d @@ -2119,19 +2119,19 @@ extern (C) bool thread_isMainThread() nothrow @nogc * Registers the calling thread for use with the D Runtime. If this routine is * called for a thread which is already registered, no action is performed. On * Posix systems, the D Runtime does not take ownership of the thread; - * specifically, it does not call $(D pthread_detach) during cleanup. + * specifically, it does not call `pthread_detach` during cleanup. * - * Threads registered by this routine should normally be deregistered by $(D - * thread_detachThis). Although $(D thread_detachByAddr) and $(D - * thread_detachInstance) can be used as well, such threads cannot be registered - * again by $(D thread_attachThis) unless $(D thread_setThis) is called with the - * $(D null) value first. + * Threads registered by this routine should normally be deregistered by + * `thread_detachThis`. Although `thread_detachByAddr` and + * `thread_detachInstance` can be used as well, such threads cannot be registered + * again by `thread_attachThis` unless `thread_setThis` is called with the + * `null` reference first. * * NOTE: This routine does not run thread-local static constructors when called. * If full functionality as a D thread is desired, the following function * must be called after thread_attachThis: * - * extern (C) void rt_moduleTlsCtor(); + * `extern (C) void rt_moduleTlsCtor();` */ extern (C) Thread thread_attachThis() {