Add ``tearDownForeignThreadGc`` function (#5369)

5 files changed, 132 insertions, 10 deletions

diff --git a/doc/backends.txt b/doc/backends.txt
index 5846cce9b..6446103ed 100644
--- a/doc/backends.txt
+++ b/doc/backends.txt
@@ -461,3 +461,11 @@ can then attach a GC to this thread via
 It is **not** safe to disable the garbage collector and enable it after the
 call from your background thread even if the code you are calling is short
 lived.
+
+Before the thread exits, you should tear down the thread's GC to prevent memory
+leaks by calling
+
+.. code-block:: nim
+
+  system.tearDownForeignThreadGc()
+
diff --git a/lib/system/gc_common.nim b/lib/system/gc_common.nim
index 269514ceb..6ab6bd920 100644
--- a/lib/system/gc_common.nim
+++ b/lib/system/gc_common.nim
@@ -128,13 +128,7 @@ iterator items(stack: ptr GcStack): ptr GcStack =
     yield s
     s = s.next
 
-# There will be problems with GC in foreign threads if `threads` option is off or TLS emulation is enabled
-const allowForeignThreadGc = compileOption("threads") and not compileOption("tlsEmulation")
-
-when allowForeignThreadGc:
-  var
-    localGcInitialized {.rtlThreadVar.}: bool
-
+when declared(threadType):
   proc setupForeignThreadGc*() {.gcsafe.} =
     ## Call this if you registered a callback that will be run from a thread not
     ## under your control. This has a cheap thread-local guard, so the GC for
@@ -143,16 +137,33 @@ when allowForeignThreadGc:
     ##
     ## This function is available only when ``--threads:on`` and ``--tlsEmulation:off``
     ## switches are used
-    if not localGcInitialized:
-      localGcInitialized = true
+    if threadType == ThreadType.None:
       initAllocator()
       var stackTop {.volatile.}: pointer
       setStackBottom(addr(stackTop))
       initGC()
+      threadType = ThreadType.ForeignThread
+
+  proc tearDownForeignThreadGc*() {.gcsafe.} =
+    ## Call this to tear down the GC, previously initialized by ``setupForeignThreadGc``.
+    ## If GC has not been previously initialized, or has already been torn down, the
+    ## call does nothing.
+    ##
+    ## This function is available only when ``--threads:on`` and ``--tlsEmulation:off``
+    ## switches are used
+    if threadType != ThreadType.ForeignThread:
+      return
+    when declared(deallocOsPages): deallocOsPages()
+    threadType = ThreadType.None
+    when declared(gch): zeroMem(addr gch, sizeof(gch))
+
 else:
   template setupForeignThreadGc*() =
     {.error: "setupForeignThreadGc is available only when ``--threads:on`` and ``--tlsEmulation:off`` are used".}
 
+  template tearDownForeignThreadGc*() =
+    {.error: "tearDownForeignThreadGc is available only when ``--threads:on`` and ``--tlsEmulation:off`` are used".}
+
 # ----------------- stack management --------------------------------------
 #  inspired from Smart Eiffel
 
diff --git a/lib/system/threads.nim b/lib/system/threads.nim
index e8b34bf2e..6e58638e9 100644
--- a/lib/system/threads.nim
+++ b/lib/system/threads.nim
@@ -285,7 +285,19 @@ when useStackMaskHack:
 when not defined(useNimRtl):
   when not useStackMaskHack:
     #when not defined(createNimRtl): initStackBottom()
-    when declared(initGC): initGC()
+    when declared(initGC):
+      initGC()
+      when not emulatedThreadVars:
+        type ThreadType {.pure.} = enum
+          None = 0,
+          NimThread = 1,
+          ForeignThread = 2
+        var
+          threadType {.rtlThreadVar.}: ThreadType
+
+        threadType = ThreadType.NimThread
+
+
 
   when emulatedThreadVars:
     if nimThreadVarsSize() > sizeof(ThreadLocalStorage):
@@ -442,6 +454,8 @@ proc threadProcWrapStackFrame[TArg](thrd: ptr Thread[TArg]) =
       # init the GC for refc/markandsweep
       setStackBottom(addr(p))
       initGC()
+      when declared(threadType):
+        threadType = ThreadType.NimThread
     when declared(registerThread):
       thrd.stackBottom = addr(thrd)
       registerThread(thrd)
diff --git a/tests/gc/foreign_thr.nim b/tests/gc/foreign_thr.nim
new file mode 100644
index 000000000..88ab95113
--- /dev/null
+++ b/tests/gc/foreign_thr.nim
@@ -0,0 +1,88 @@
+discard """
+  output: '''
+Hello from thread
+Hello from thread
+Hello from thread
+Hello from thread
+'''
+  cmd: "nim $target --hints:on --threads:on --tlsEmulation:off $options $file"
+"""
+# Copied from stdlib
+import strutils
+
+const
+  StackGuardSize = 4096
+  ThreadStackMask = 1024*256*sizeof(int)-1
+  ThreadStackSize = ThreadStackMask+1 - StackGuardSize
+
+type ThreadFunc = proc() {.thread.}
+
+when defined(posix):
+  import posix
+
+  proc runInForeignThread(f: ThreadFunc) =
+    proc wrapper(p: pointer): pointer {.noconv.} =
+      let thr = cast[ThreadFunc](p)
+      setupForeignThreadGc()
+      thr()
+      tearDownForeignThreadGc()
+      setupForeignThreadGc()
+      thr()
+      tearDownForeignThreadGc()
+      result = nil
+
+    var attrs {.noinit.}: PthreadAttr
+    doAssert pthread_attr_init(addr attrs) == 0
+    doAssert pthread_attr_setstacksize(addr attrs, ThreadStackSize) == 0
+    var tid: Pthread
+    doAssert pthread_create(addr tid, addr attrs, wrapper, f) == 0
+    doAssert pthread_join(tid, nil) == 0
+
+elif defined(windows):
+  import winlean
+  type
+    WinThreadProc = proc (x: pointer): int32 {.stdcall.}
+
+  proc createThread(lpThreadAttributes: pointer, dwStackSize: DWORD,
+                     lpStartAddress: WinThreadProc,
+                     lpParameter: pointer,
+                     dwCreationFlags: DWORD,
+                     lpThreadId: var DWORD): Handle {.
+    stdcall, dynlib: "kernel32", importc: "CreateThread".}
+
+  proc wrapper(p: pointer): int32 {.stdcall.} =
+    let thr = cast[ThreadFunc](p)
+    setupForeignThreadGc()
+    thr()
+    tearDownForeignThreadGc()
+    setupForeignThreadGc()
+    thr()
+    tearDownForeignThreadGc()
+    result = 0'i32
+
+  proc runInForeignThread(f: ThreadFunc) =
+    var dummyThreadId: DWORD
+    var h = createThread(nil, ThreadStackSize.int32, wrapper.WinThreadProc, cast[pointer](f), 0, dummyThreadId)
+    doAssert h != 0.Handle
+    doAssert waitForSingleObject(h, -1'i32) == 0.DWORD
+
+else:
+  {.fatal: "Unknown system".}
+
+proc runInNativeThread(f: ThreadFunc) =
+  proc wrapper(f: ThreadFunc) {.thread.} =
+    # These operations must be NOP
+    setupForeignThreadGc()
+    tearDownForeignThreadGc()
+    f()
+    f()
+  var thr: Thread[ThreadFunc]
+  createThread(thr, wrapper, f)
+  joinThread(thr)
+
+proc f {.thread.} =
+  var msg = "Hello " & "from thread"
+  echo msg
+
+runInForeignThread(f)
+runInNativeThread(f)
diff --git a/tests/testament/categories.nim b/tests/testament/categories.nim
index 2dc8e3318..0685dd73a 100644
--- a/tests/testament/categories.nim
+++ b/tests/testament/categories.nim
@@ -147,6 +147,7 @@ proc gcTests(r: var TResults, cat: Category, options: string) =
       testSpec r, makeTest("tests/gc" / filename, options &
                     " -d:release --gc:boehm", cat, actionRun)
 
+  testWithoutBoehm "foreign_thr"
   test "gcemscripten"
   test "growobjcrash"
   test "gcbench"