Fixes for typedthreads data race issues

lib/std/private/threadtypes.nim

@@ -1,4 +1,5 @@
 include system/inclrtl
+import std/atomics
 
 const hasSharedHeap* = defined(boehmgc) or defined(gogc) # don't share heaps; every thread has its own
 
@@ -163,13 +164,13 @@ const hasAllocStack* = defined(zephyr) # maybe freertos too?
 
 type
   Thread*[TArg] = object
-    core*: PGcThread
+    core*: Atomic[PGcThread]
     sys*: SysThread
     when TArg is void:
-      dataFn*: proc () {.nimcall, gcsafe.}
+      dataFn*: Atomic[proc () {.nimcall, gcsafe.}]
     else:
-      dataFn*: proc (m: TArg) {.nimcall, gcsafe.}
-      data*: TArg
+      dataFn*: Atomic[proc (m: TArg) {.nimcall, gcsafe.}]
+      data*: Atomic[TArg]
     when hasAllocStack:
       rawStack*: pointer
 

lib/std/typedthreads.nim

@@ -75,7 +75,7 @@ deinitLock(l)
 ```
 ]##
 
-
+import std/atomics
 import std/private/[threadtypes]
 export Thread
 
@@ -149,9 +149,9 @@ else:
     nimThreadProcWrapperBody(closure)
 {.pop.}
 
-proc running*[TArg](t: Thread[TArg]): bool {.inline.} =
+proc running*[TArg](t: var Thread[TArg]): bool {.inline.} =
   ## Returns true if `t` is running.
-  result = t.dataFn != nil
+  result = t.dataFn.load() != nil
 
 proc handle*[TArg](t: Thread[TArg]): SysThread {.inline.} =
   ## Returns the thread handle of `t`.
@@ -202,7 +202,7 @@ when false:
     else:
       discard pthread_cancel(t.sys)
     when declared(registerThread): unregisterThread(addr(t))
-    t.dataFn = nil
+    t.dataFn.store nil
     ## if thread `t` already exited, `t.core` will be `null`.
     if not isNil(t.core):
       deallocThreadStorage(t.core)
@@ -217,10 +217,10 @@ when hostOS == "windows":
     ## Entry point is the proc `tp`.
     ## `param` is passed to `tp`. `TArg` can be `void` if you
     ## don't need to pass any data to the thread.
-    t.core = cast[PGcThread](allocThreadStorage(sizeof(GcThread)))
+    t.core.store cast[PGcThread](allocThreadStorage(sizeof(GcThread)))
 
-    when TArg isnot void: t.data = param
-    t.dataFn = tp
+    when TArg isnot void: t.data.store param
+    t.dataFn.store tp
     when hasSharedHeap: t.core.stackSize = ThreadStackSize
     var dummyThreadId: int32 = 0'i32
     t.sys = createThread(nil, ThreadStackSize, threadProcWrapper[TArg],
@@ -242,10 +242,10 @@ elif defined(genode):
   proc createThread*[TArg](t: var Thread[TArg],
                            tp: proc (arg: TArg) {.thread, nimcall.},
                            param: TArg) =
-    t.core = cast[PGcThread](allocThreadStorage(sizeof(GcThread)))
+    t.core.store cast[PGcThread](allocThreadStorage(sizeof(GcThread)))
 
-    when TArg isnot void: t.data = param
-    t.dataFn = tp
+    when TArg isnot void: t.data.store param
+    t.dataFn.store tp
     when hasSharedHeap: t.stackSize = ThreadStackSize
     t.sys.initThread(
       runtimeEnv,
@@ -266,11 +266,14 @@ else:
     ## Entry point is the proc `tp`. `param` is passed to `tp`.
     ## `TArg` can be `void` if you
     ## don't need to pass any data to the thread.
-    t.core = cast[PGcThread](allocThreadStorage(sizeof(GcThread)))
-
-    when TArg isnot void: t.data = param
-    t.dataFn = tp
-    when hasSharedHeap: t.core.stackSize = ThreadStackSize
+    t.core.store cast[PGcThread](allocThreadStorage(sizeof(GcThread)))
+
+    when TArg isnot void: t.data.store param
+    t.dataFn.store tp
+    when hasSharedHeap: 
+      var stackSize: PGcThread = t.core.load()
+      stacksize.stackSize = ThreadStackSize
+      t.core.store stacksize
     var a {.noinit.}: Pthread_attr
     doAssert pthread_attr_init(a) == 0
     when hasAllocStack:

lib/system/threadimpl.nim

@@ -1,3 +1,5 @@
+import std/atomics
+
 var
   nimThreadDestructionHandlers* {.rtlThreadVar.}: seq[proc () {.closure, gcsafe, raises: [].}]
 when not defined(boehmgc) and not hasSharedHeap and not defined(gogc) and not defined(gcRegions):
@@ -50,10 +52,11 @@ when defined(boehmgc):
     boehmGC_register_my_thread(sb)
     try:
       let thrd = cast[ptr Thread[TArg]](thrd)
+      let dataFn = thrd.dataFn.load()
       when TArg is void:
-        thrd.dataFn()
+        dataFn()
       else:
-        thrd.dataFn(thrd.data)
+        dataFn(thrd.data.load())
     except:
       threadTrouble()
     finally:
@@ -62,15 +65,16 @@ when defined(boehmgc):
 else:
   proc threadProcWrapDispatch[TArg](thrd: ptr Thread[TArg]) {.raises: [].} =
     try:
+      let dataFn = thrd.dataFn.load()
       when TArg is void:
-        thrd.dataFn()
+        dataFn()
       else:
         when defined(nimV2):
-          thrd.dataFn(thrd.data)
+          dataFn(thrd.data.load())
         else:
           var x: TArg = default(TArg)
-          deepCopy(x, thrd.data)
-          thrd.dataFn(x)
+          deepCopy(x, thrd.data.load())
+          dataFn(x)
     except:
       threadTrouble()
     finally:
@@ -96,16 +100,16 @@ proc threadProcWrapStackFrame[TArg](thrd: ptr Thread[TArg]) {.raises: [].} =
 
 template nimThreadProcWrapperBody*(closure: untyped): untyped =
   var thrd = cast[ptr Thread[TArg]](closure)
-  var core = thrd.core
-  when declared(globalsSlot): threadVarSetValue(globalsSlot, thrd.core)
+  var core = thrd.core.load()
+  when declared(globalsSlot): threadVarSetValue(globalsSlot, thrd.core.load())
   threadProcWrapStackFrame(thrd)
   # Since an unhandled exception terminates the whole process (!), there is
-  # no need for a ``try finally`` here, nor would it be correct: The current
-  # exception is tried to be re-raised by the code-gen after the ``finally``!
+  # no need for a `try finally` here, nor would it be correct: The current
+  # exception is tried to be re-raised by the code-gen after the `finally`!
   # However this is doomed to fail, because we already unmapped every heap
   # page!
 
   # mark as not running anymore:
-  thrd.core = nil
-  thrd.dataFn = nil
-  deallocThreadStorage(cast[pointer](core))
+  thrd.core.store nil
+  thrd.dataFn.store nil
+  deallocThreadStorage(core)