From b6c17dc822960b626d76d814de90208c0a40a44e Mon Sep 17 00:00:00 2001 From: Gabriel Cruz <8129788+gmelodie@users.noreply.github.com> Date: Fri, 7 Aug 2026 12:00:55 -0300 Subject: [PATCH] fix(ffi): run the {.ffiDtor.} teardown on the recycle path (#147) --- .gitignore | 1 + CHANGELOG.md | 8 +++ ffi/event_thread.nim | 5 +- ffi/ffi_context.nim | 17 ++++-- ffi/ffi_thread.nim | 47 +++++++++++------ ffi/internal/ffi_macro.nim | 8 +-- tests/unit/test_ffi_teardown.nim | 79 ++++++++++++++++++++++++++-- tests/unit/test_ffi_teardown.nim.cfg | 2 + 8 files changed, 136 insertions(+), 31 deletions(-) create mode 100644 tests/unit/test_ffi_teardown.nim.cfg diff --git a/.gitignore b/.gitignore index 0cc0f12..508a6c9 100644 --- a/.gitignore +++ b/.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 diff --git a/CHANGELOG.md b/CHANGELOG.md index 0975823..581fe70 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/ffi/event_thread.nim b/ffi/event_thread.nim index af8376d..b0ae54e 100644 --- a/ffi/event_thread.nim +++ b/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() diff --git a/ffi/ffi_context.nim b/ffi/ffi_context.nim index 7dc1e99..5d5dd9d 100644 --- a/ffi/ffi_context.nim +++ b/ffi/ffi_context.nim @@ -73,10 +73,16 @@ const RecycleTimeoutMs* {.intdefine: "ffiRecycleTimeoutMs".} = 1500 ## again. Override with `-d:ffiRecycleTimeoutMs=`. 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=`. +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=`. +## 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=`. const ThreadExitTimeoutMs* {.intdefine: "ffiThreadExitTimeoutMs".} = 1500 const ThreadExitTimeout* = ThreadExitTimeoutMs.milliseconds diff --git a/ffi/ffi_thread.nim b/ffi/ffi_thread.nim index c8a9317..a7050ad 100644 --- a/ffi/ffi_thread.nim +++ b/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) diff --git a/ffi/internal/ffi_macro.nim b/ffi/internal/ffi_macro.nim index d30980c..7c2cd59 100644 --- a/ffi/internal/ffi_macro.nim +++ b/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] diff --git a/tests/unit/test_ffi_teardown.nim b/tests/unit/test_ffi_teardown.nim index a17a83d..9ef4e56 100644 --- a/tests/unit/test_ffi_teardown.nim +++ b/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() diff --git a/tests/unit/test_ffi_teardown.nim.cfg b/tests/unit/test_ffi_teardown.nim.cfg new file mode 100644 index 0000000..18d170e --- /dev/null +++ b/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