Skip to content

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

Merged
gmelodie merged 2 commits into
masterfrom
fix/avoid-zombie-node-on-destroy
Aug 7, 2026
Merged

fix(ffi): run the {.ffiDtor.} teardown on the recycle path#147
gmelodie merged 2 commits into
masterfrom
fix/avoid-zombie-node-on-destroy

Conversation

@gmelodie

@gmelodie gmelodie commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator

Summary

A {.ffiDtor.} body never ran. The C destructor that buildFFIDtorProc emits calls recycleFFIContext (ffi/internal/ffi_macro.nim:1696), but the only site that awaited ffiTeardownHook was the thread-exit epilogue of the full-shutdown path, which needs ctx.running == false. Only destroyFFIContext sets that, and no generated wrapper calls it. So the hook site sat on a path nobody takes, and every dtor body shipped by a consumer was dead code.

The consequence is worse than a skipped callback. recycleContext frees the library with freeLib, which is memory-only, and the pooled worker survives a recycle by design. Anything the library spawned itself keeps running on that dispatcher, invisible to a host whose handle is gone — and the next createFFIContext can hand the same slot, and the same event loop, to a different owner.

recycleContext now awaits the hook after it drains the in-flight requests and before it frees the library. Both call sites share one runTeardown helper so the recycle path and the destroy path cannot drift apart.

Nothing in-tree caught this: both examples (examples/echo/echo.nim:57, examples/timer/timer.nim:154) have no-op dtor bodies, and tests/unit/test_ffi_teardown.nim only exercised destroyFFIContext.

Affected Areas

  • ffi/ffi_thread.nim — new runTeardown[T], called from recycleContext and from the thread-exit epilogue. The ordering is load-bearing: after the drain so no handler runs against a stopped library, before freeLib because the hook needs myLib, before clearListeners so events the teardown emits still reach the host. It runs only once libReady is set, because myLib otherwise points at the worker's zero-valued fallback, which is nil for a ref library type.
  • ffi/ffi_thread.nimrecycleContext now fires recycleDoneSignal and releases the slot from a defer. The function awaits arbitrary consumer code, and a raise out of that code must not strand the slot in Recycling.
  • ffi/event_thread.nim — the heartbeat check is skipped unless the lifecycle is Active. The worker ticks proveAlive only at the top of its loop, so a teardown parks the heartbeat for its whole duration; against a 1 s FFIHeartbeatStaleThreshold every healthy shutdown longer than a second would emit a NotRespondingEvent, and the matching onResponding could never arrive because clearListeners runs first. The eventQueueStuck check is untouched.
  • ffi/ffi_context.nim — new TeardownTimeoutMs (-d:ffiTeardownTimeoutMs, 10 s default) and RecycleWaitTimeout widened to cover it, so a synchronous recycleFFIContext does not give up while a legitimate teardown runs.
  • ffi/internal/ffi_macro.nim — the {.ffiDtor.} docstring now states where the body runs, how long the wrapper blocks its caller, and that the body must stay cancellable.
  • tests/unit/test_ffi_teardown.nim — a recycle suite covering the hook on recycleFFIContext, through the C-exported wrapper, on a slot proven to be the reused one, and past the timeout.
  • .gitignoretests/unit/test_* (the rule for extensionless compiled binaries) also swallowed tests/unit/test_ffi_teardown.nim.cfg. Added a !tests/unit/test_*.nim.cfg escape hatch next to the existing !tests/unit/test_*.nim.

Impact on Library Users

Behaviour change for any consumer that already ships a non-empty {.ffiDtor.} body: it went from dead code to live shutdown code, and it now runs on the ordinary destroy path. Read that body again before you take this bump.

The generated destructor now blocks its caller for as long as the teardown takes, up to RecycleWaitTimeout — 15 s at the default budgets, against 5 s before. Do not call it from a thread that must stay responsive.

Keep the dtor cancellable. At TeardownTimeout the hook is cancelled, but chronos withTimeout then waits for the cancellation to land, so a body that swallows CancelledError still holds the recycle. That is a slow-shutdown bug in the consumer, not something nim-ffi can bound from the outside.

ThreadExitTimeoutMs keeps its 1500 ms default. Raise it past ffiTeardownTimeoutMs if your dtor is slow on the destroyFFIContext path.

Risk Assessment

  • Backward compatible at the ABI and the API. No signature changes.
  • The slot is released on every exit path of recycleContext, including a raise out of the dtor, because the release sits in a defer.
  • TeardownTimeout is a cancellation request, not a hard ceiling. A dtor that ignores cancellation blocks the recycle, the caller then fails on RecycleWaitTimeout, and that slot does not return to the pool. Documented rather than papered over.
  • Suppressing the heartbeat check during a recycle means a worker that genuinely wedges inside a teardown no longer raises NotRespondingEvent. TeardownTimeout and RecycleWaitTimeout are the bounds that cover that window instead, and the alternative was a false alarm on every normal shutdown.
  • ThreadExitTimeout was deliberately left alone: tests/unit/test_ffi_context.nim:261 asserts that destroyFFIContext gives up in under 3 s when the worker loop is wedged, and raising the default breaks that fast-bail guarantee.
  • Not covered by a test: the libReady gate itself. Proving it needs a second library type with a failing constructor, and the reasoning rests on ffi_codegen_common.nim:27, which guards {.ffi.} calls the same way.

References

This is necessary but not sufficient for #4108. nim-ffi cannot cancel work a library spawned on its own, so liblogosdelivery must still register the dtor.

@gmelodie
gmelodie force-pushed the fix/avoid-zombie-node-on-destroy branch 3 times, most recently from db82a01 to aa29e68 Compare August 6, 2026 13:07
@gmelodie
gmelodie force-pushed the fix/avoid-zombie-node-on-destroy branch from aa29e68 to 8e394b9 Compare August 6, 2026 13:36
@gmelodie
gmelodie requested review from Ivansete-status and NagyZoltanPeter and removed request for Ivansete-status August 6, 2026 15:11
@gmelodie
gmelodie marked this pull request as ready for review August 6, 2026 15:11

@NagyZoltanPeter NagyZoltanPeter left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find it good and covers what we need.
Would it be possible to put it into v0.3.0.... rc version. We need to add such into logos-delivery IMO.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants