fix(ffi): run the {.ffiDtor.} teardown on the recycle path (#147)

.gitignore

@@ -45,3 +45,4 @@ PLAN.md
 # Compiled test binaries (extensionless executables, also under tests/unit/)
 tests/unit/test_*
 !tests/unit/test_*.nim
+!tests/unit/test_*.nim.cfg

CHANGELOG.md

@@ -49,6 +49,14 @@ All notable changes to this project are documented in this file.
   router would silently give it the ctor ABI instead.
 
 ### Fixed
+- A `{.ffiDtor.}` body now runs when the context is recycled. Only the
+  thread-exit epilogue awaited the teardown hook, and the emitted C destructor
+  never takes that path, so a dtor body was dead code and the library kept every
+  loop it had spawned on the pooled worker. The recycle handler now awaits the
+  hook between the drain and `freeLib`, once a constructor has stored a library.
+  A body that overruns `-d:ffiTeardownTimeoutMs` (10 s) is cancelled, so keep the
+  dtor cancellable; the slot is released on every exit path. Consumers shipping a
+  non-empty dtor body should read it again: it is live now.
 - A `{.ffi.}` call against a `ref` library type whose `{.ffiCtor.}` never stored
   a library (it failed, or none ran) no longer crashes. Without a constructed
   library the FFI thread points `myLib` at a default-valued fallback; for an

ffi/event_thread.nim

@@ -116,7 +116,10 @@ proc eventRun[T](ctx: ptr FFIContext[T]) {.async.} =
       if not notifiedStuck and ctx.eventQueueStuck.load():
         onNotResponding(ctx)
         notifiedStuck = true
-      hb.check(ctx)
+      # A recycle parks the loop in the teardown on purpose; a stall there is
+      # not a fault, and clearListeners drops the onResponding that would follow.
+      if ctx.lifecycle.load() == CtxLifecycle.Active:
+        hb.check(ctx)
 
   # Catch anything enqueued between the last drain and the FFI thread's exit.
   ctx.drainEventQueue()

ffi/ffi_context.nim

@@ -73,10 +73,16 @@ const RecycleTimeoutMs* {.intdefine: "ffiRecycleTimeoutMs".} = 1500
   ## again. Override with `-d:ffiRecycleTimeoutMs=<ms>`.
 const RecycleTimeout* = RecycleTimeoutMs.milliseconds
 
+const TeardownTimeoutMs* {.intdefine: "ffiTeardownTimeoutMs".} = 10000
+  ## Cancels a `{.ffiDtor.}` teardown that overruns; a real library stop outlasts
+  ## a drain round. Override with `-d:ffiTeardownTimeoutMs=<ms>`.
+const TeardownTimeout* = TeardownTimeoutMs.milliseconds
+
 const
-  RecycleWaitTimeout* = 2 * RecycleTimeout + 2.seconds
-    ## Caller-side bound for synchronous recycle. It covers both drain rounds
-    ## plus slack, so it only fires when the worker itself is wedged.
+  RecycleWaitTimeout* = 2 * RecycleTimeout + TeardownTimeout + 2.seconds
+    ## Caller-side bound for synchronous recycle: both drain rounds, the teardown
+    ## hook and slack, so it only fires when the worker itself is wedged. The
+    ## generated C destructor blocks its caller this long — 15 s by default.
   EventThreadTickInterval* = 1.seconds
   FFIHeartbeatStartDelay* = 10.seconds
   FFIHeartbeatStaleThreshold* = 1.seconds
@@ -248,8 +254,9 @@ proc requestRecycle*[T](ctx: ptr FFIContext[T]): Result[void, string] =
     return err("requestRecycle: recycle did not complete in time")
   ok()
 
-## Per-thread exit wait before stopAndJoinThreads leaks ctx rather than hanging; async
-## `{.ffiDtor.}` teardown can outlast the default. Override `-d:ffiThreadExitTimeoutMs=<ms>`.
+## Per-thread exit wait before stopAndJoinThreads leaks ctx rather than hanging. Kept
+## short so a wedged worker fails fast; raise it past `ffiTeardownTimeoutMs` for a slow
+## `{.ffiDtor.}`. Override `-d:ffiThreadExitTimeoutMs=<ms>`.
 const ThreadExitTimeoutMs* {.intdefine: "ffiThreadExitTimeoutMs".} = 1500
 const ThreadExitTimeout* = ThreadExitTimeoutMs.milliseconds
 

ffi/ffi_thread.nim

@@ -126,12 +126,35 @@ proc rejectQueuedRequests[T](ctx: ptr FFIContext[T]) =
       error "rejecting a queued request raised", error = e.msg
     request = nextRequest
 
+proc runTeardown[T](ctx: ptr FFIContext[T]) {.async.} =
+  ## Awaits the library's `{.ffiDtor.}` body. `libReady` gates it: without a ctor
+  ## `myLib` is the zero-valued fallback, nil for a `ref` type.
+  let teardown = ffiTeardownHook[T]()
+  if teardown.isNil() or ctx.myLib.isNil() or not ctx.libReady.load():
+    return
+  try:
+    let done = await teardown(ctx.myLib).withTimeout(TeardownTimeout)
+    if not done:
+      error "library teardown cancelled at the timeout; releasing the library",
+        timeoutMs = TeardownTimeoutMs
+  except CatchableError as e:
+    error "library teardown raised", error = e.msg
+
 proc recycleContext[T](
     ctx: ptr FFIContext[T], ongoing: ptr seq[Future[void]]
 ) {.async.} =
-  ## Drain in-flight handlers, free the lib, clear listeners and release the
-  ## slot — all WITHOUT stopping the worker/event threads, so the next
-  ## createFFIContext reuses them (no fd churn). Then fire recycleDoneSignal.
+  ## Drain in-flight handlers, run the library teardown, free the lib, clear
+  ## listeners, then fire recycleDoneSignal and release the slot — all WITHOUT
+  ## stopping the worker/event threads, so the next createFFIContext reuses them
+  ## (no fd churn).
+  # Deferred: a raise out of the teardown must not strand the slot. Fire before
+  # the release, or a thread claiming the slot would take this as its own answer.
+  defer:
+    let fireRes = ctx.recycleDoneSignal.fireSync()
+    if fireRes.isErr():
+      error "failed to fire recycleDoneSignal", err = fireRes.error
+    ctx.releaseClaim()
+
   ongoing[].keepItIf(not it.finished())
   var drained = ongoing[].len == 0
   if not drained:
@@ -141,18 +164,14 @@ proc recycleContext[T](
       fut.cancelSoon()
     drained = await allFutures(ongoing[]).withTimeout(RecycleTimeout)
 
+  # Before freeLib: the hook still needs `myLib` and its listeners.
+  await runTeardown(ctx)
+
   freeLib(ctx)
   clearListeners(ctx[].eventRegistry)
   rejectQueuedRequests(ctx)
   ongoing[].setLen(0)
 
-  # Fire before the release: a thread that claims the freed slot first would
-  # otherwise take this fire as the answer to its own recycle.
-  let fireRes = ctx.recycleDoneSignal.fireSync()
-  if fireRes.isErr():
-    error "failed to fire recycleDoneSignal", err = fireRes.error
-  ctx.releaseClaim()
-
 var ffiEventQueueSignalPtr {.threadvar.}: ThreadSignalPtr
   # Stashed so the hook has no closure env.
 
@@ -252,12 +271,6 @@ proc ffiThreadBody[T](ctx: ptr FFIContext[T]) {.thread.} =
       except CatchableError as e:
         error "draining pending FFI requests on shutdown raised", error = e.msg
 
-    # Run the library's async {.ffiDtor.} shutdown before join if one exists and a request populated `myLib`; exceptions logged, never propagated.
-    let teardown = ffiTeardownHook[T]()
-    if not teardown.isNil() and not ctx.myLib.isNil():
-      try:
-        await teardown(ctx.myLib)
-      except CatchableError as e:
-        error "library teardown raised on shutdown", error = e.msg
+    await runTeardown(ctx)
 
   waitFor ffiRun(ctx)

ffi/internal/ffi_macro.nim

@@ -1739,9 +1739,11 @@ proc buildFFIDtorProc(prc: NimNode, abiFormat: ABIFormat): NimNode {.compileTime
 
 macro ffiDtor*(args: varargs[untyped]): untyped =
   ## C-exported FFIContext destructor. Sync (no return) or async (`Future[void]`);
-  ## a non-empty body becomes an async `ffiTeardownHook` the FFI thread awaits at
-  ## shutdown, so teardown runs on the worker thread. RET_ERR on null/invalid ctx.
-  ## `{.ffi.}` reaches the same path from the shape alone.
+  ## a non-empty body becomes an async `ffiTeardownHook` the FFI thread awaits on
+  ## the recycle and the shutdown path. The wrapper blocks its caller until the
+  ## body finishes, up to `RecycleWaitTimeout` (15 s by default). Keep the body
+  ## cancellable: one that ignores `TeardownTimeout` holds the recycle. RET_ERR
+  ## on null/invalid ctx. `{.ffi.}` reaches the same path from the shape alone.
   requireBeforeGenBindings("`.ffiDtor.`")
   requireLibraryDeclared("`.ffiDtor.`")
   let prc = args[^1]

tests/unit/test_ffi_teardown.nim

@@ -3,7 +3,8 @@ import unittest2
 import results
 import ffi
 
-# Exercises async {.ffiDtor.}: destroyFFIContext must block until teardown finishes.
+# Exercises async {.ffiDtor.} on the destroy and the recycle path.
+# test_ffi_teardown.nim.cfg cuts TeardownTimeout to 1s for every suite here.
 
 type TeardownLib = object
 
@@ -14,6 +15,7 @@ declareLibrary("teardownlib", TeardownLib)
 
 var gTeardownRan: Atomic[bool]
 var gTeardownThreadId: Atomic[int]
+var gTeardownHangs: Atomic[bool]
 
 type NoopConfig {.ffi.} = object
   dummy: int
@@ -24,8 +26,12 @@ proc teardownlib_create*(
   return ok(TeardownLib())
 
 proc teardownlib_destroy*(lib: TeardownLib): Future[void] {.ffiDtor.} =
-  ## Async teardown: sleeps, then records that it ran and on which thread.
-  await sleepAsync(200.milliseconds)
+  ## Records that it ran and on which thread. `gTeardownHangs` makes it sleep
+  ## past every timeout in play, well clear of any sanitizer slowdown.
+  if gTeardownHangs.load():
+    await sleepAsync(TeardownTimeout + 60.seconds)
+  else:
+    await sleepAsync(200.milliseconds)
   gTeardownThreadId.store(getThreadId())
   gTeardownRan.store(true)
 
@@ -41,14 +47,15 @@ proc encodedPtr(bytes: var seq[byte]): ptr byte =
     cast[ptr byte](addr bytes[0])
 
 proc createCtxWithLib(): ptr FFIContext[TeardownLib] =
-  ## Spins up a context via the ctor and waits until `myLib` is populated.
+  ## Spins up a context and waits on `libReady`, the flag the teardown gates on.
+  # Not `myLib`: the worker points that at its fallback before the ctor runs.
   var cfg = cborEncode(TeardownlibCreateCtorReq(config: NoopConfig(dummy: 0)))
   let ret = teardownlib_create(encodedPtr(cfg), cfg.len.csize_t, noopCallback, nil)
   if ret.isNil():
     return nil
   let ctx = cast[ptr FFIContext[TeardownLib]](ret)
   var tries = 0
-  while ctx[].myLib.isNil() and tries < 500:
+  while not ctx[].libReady.load() and tries < 500:
     os.sleep(5)
     inc tries
   ctx
@@ -75,3 +82,65 @@ suite "async {.ffiDtor.} teardown hook":
     check not ctx.isNil()
     check TeardownlibFFIPool.destroyFFIContext(ctx).isOk()
     check gTeardownRan.load()
+
+suite "{.ffiDtor.} teardown on the recycle path":
+  # Recycle is the path the generated C destroy wrapper takes.
+  test "recycle blocks until the async teardown body completes":
+    let ctx = createCtxWithLib()
+    check not ctx.isNil()
+    check not ctx[].myLib.isNil()
+
+    gTeardownRan.store(false)
+    gTeardownThreadId.store(0)
+    let callerTid = getThreadId()
+
+    check TeardownlibFFIPool.recycleFFIContext(ctx).isOk()
+
+    check gTeardownRan.load()
+    check gTeardownThreadId.load() != 0
+    check gTeardownThreadId.load() != callerTid
+
+  test "the C-exported destroy wrapper runs the teardown":
+    let ctx = createCtxWithLib()
+    check not ctx.isNil()
+
+    gTeardownRan.store(false)
+    check teardownlib_destroy(cast[pointer](ctx)) == RET_OK
+    check gTeardownRan.load()
+
+  test "a slot reused after teardown tears down again":
+    let first = createCtxWithLib()
+    check not first.isNil()
+    check TeardownlibFFIPool.recycleFFIContext(first).isOk()
+
+    gTeardownRan.store(false)
+    let second = createCtxWithLib()
+    check not second.isNil()
+    # Lowest free slot wins, so a fresh slot here would prove nothing.
+    check second == first
+    check not second[].myLib.isNil()
+    check TeardownlibFFIPool.recycleFFIContext(second).isOk()
+    check gTeardownRan.load()
+
+  test "a teardown past TeardownTimeout still releases the slot":
+    let ctx = createCtxWithLib()
+    check not ctx.isNil()
+
+    gTeardownRan.store(false)
+    gTeardownHangs.store(true)
+    let t0 = Moment.now()
+    check TeardownlibFFIPool.recycleFFIContext(ctx).isOk()
+    let elapsed = Moment.now() - t0
+    gTeardownHangs.store(false)
+
+    # The timeout ended the wait, not an early bail or the body finishing.
+    check elapsed >= TeardownTimeout
+    check elapsed < RecycleWaitTimeout
+    check not gTeardownRan.load()
+
+    let next = createCtxWithLib()
+    check not next.isNil()
+    check next == ctx
+    check not next[].myLib.isNil()
+    check TeardownlibFFIPool.recycleFFIContext(next).isOk()
+    check gTeardownRan.load()

tests/unit/test_ffi_teardown.nim.cfg

@@ -0,0 +1,2 @@
+# File-wide: at the 10s default the hang test costs 10s on every matrix job.
+-d:ffiTeardownTimeoutMs=1000