Revive 1989: Thread detach stability

src/core/thread/osthread.d

@@ -709,8 +709,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)
         {
@@ -822,6 +825,8 @@ class Thread
                 }
                 if ( pthread_attr_destroy( &attr ) != 0 )
                     onThreadError( "Error destroying thread attributes" );
+
+                m_isOwned = true;
             }
             version (Darwin)
             {
@@ -1631,6 +1636,7 @@ private:
     version (Posix)
     {
         shared bool     m_isRunning;
+        bool            m_isOwned;
     }
     bool                m_isDaemon;
     bool                m_isInCriticalRegion;
@@ -2239,14 +2245,22 @@ 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 `pthread_detach` during cleanup.
+ *
+ * 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()
 {
@@ -2275,6 +2289,7 @@ private Thread attachThread(Thread thisThread) @nogc
         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();
@@ -2369,8 +2384,14 @@ 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);
+        t.setThis(null);
+    }
 }
 
 
@@ -2387,6 +2408,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 );
 }
@@ -2395,6 +2419,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 );
 }
 
@@ -2460,6 +2487,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

test/thread/Makefile

@@ -1,6 +1,6 @@
 include ../common.mak
 
-TESTS:=fiber_guard_page external_threads tlsgc_sections test_import tlsstack
+TESTS:=fiber_guard_page external_threads tlsgc_sections test_import tlsstack attach_detach
 
 .PHONY: all clean
 all: $(addprefix $(ROOT)/,$(addsuffix .done,$(TESTS)))
@@ -31,6 +31,11 @@ $(ROOT)/tlsstack.done: $(ROOT)/%.done : $(ROOT)/%
 	$(QUIET)$(TIMELIMIT)$(ROOT)/$*
 	@touch $@
 
+$(ROOT)/attach_detach.done: $(ROOT)/%.done : $(ROOT)/%
+	@echo Testing $*
+	$(QUIET)$(TIMELIMIT)$(ROOT)/$*
+	@touch $@
+
 $(ROOT)/%: $(SRC)/%.d
 	$(QUIET)$(DMD) $(DFLAGS) -of$@ $<
 

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()
+{
+}
+
+}