From 4da0ec3cf1d4976161f099ed63c6ff760a1dc19b Mon Sep 17 00:00:00 2001 From: Ivan FB Date: Sat, 2 May 2026 00:20:33 +0200 Subject: [PATCH 01/11] use fixed array of ctx to avoid consuming all fds --- ffi/ffi_context.nim | 214 ++++++++++++++++++++++++++----------- tests/test_ffi_context.nim | 90 ++++++++++++++-- 2 files changed, 229 insertions(+), 75 deletions(-) diff --git a/ffi/ffi_context.nim b/ffi/ffi_context.nim index 48d33e4..dc25dd7 100644 --- a/ffi/ffi_context.nim +++ b/ffi/ffi_context.nim @@ -30,6 +30,8 @@ type FFIContext*[T] = object # fired by ffiThread just before it exits; destroyFFIContext waits on # this with a bounded timeout instead of joining unconditionally, so a # blocked event loop cannot hang the caller forever + watchdogStopSignal: ThreadSignalPtr + # fired by destroyFFIContext so the watchdog exits immediately instead of waiting out its sleep userData*: pointer callbackState*: FFICallbackState running: Atomic[bool] # To control when the threads are running @@ -39,6 +41,18 @@ type FFIContext*[T] = object var ffiCurrentCallbackState* {.threadvar.}: ptr FFICallbackState ## Set by ffiThreadBody at thread startup; read by dispatchFfiEvent. +const MaxFFIContexts* = 32 + ## Maximum number of concurrently live FFI contexts when using FFIContextPool. + ## Fds and threads are only consumed for slots that are actually acquired, + ## so this value only affects the upfront memory of the pool array. + +type FFIContextPool*[T] = object + ## Fixed-size pool of FFI contexts. Avoids dynamic heap allocation per context + ## and bounds the total number of file descriptors consumed by ThreadSignalPtrs + ## to at most MaxFFIContexts * 2. + slots: array[MaxFFIContexts, FFIContext[T]] + inUse: array[MaxFFIContexts, Atomic[bool]] + const git_version* {.strdefine.} = "n/a" var contextRegistry = initHashSet[pointer]() @@ -172,16 +186,19 @@ proc watchdogThreadBody(ctx: ptr FFIContext) {.thread.} = const WatchdogTimeinterval = 1.seconds const WatchdogTimeout = 20.seconds - # Give time for the node to be created and up before sending watchdog requests - let initialStop = await ctx.stopSignal.wait().withTimeout(WatchdogStartDelay) - if initialStop or ctx.running.load == false: - return - - while true: - let intervalStop = await ctx.stopSignal.wait().withTimeout(WatchdogTimeinterval) - - if intervalStop or ctx.running.load == false: - debug "Watchdog thread exiting because FFIContext is not running" + # Give time for the node to be created and up before sending watchdog requests. + # waitSync returns early if watchdogStopSignal fires (i.e. on destroy). + let startWait = ctx.watchdogStopSignal.waitSync(WatchdogStartDelay) + if startWait.isErr(): + error "watchdog: start-delay waitSync failed", err = startWait.error + elif startWait.get(): + return # stop signal fired during start delay + + while ctx.running.load: + let intervalWait = ctx.watchdogStopSignal.waitSync(WatchdogTimeinterval) + if intervalWait.isErr(): + error "watchdog: interval waitSync failed", err = intervalWait.error + elif intervalWait.get() or not ctx.running.load: break let callback = proc( @@ -280,46 +297,33 @@ proc ffiThreadBody[T](ctx: ptr FFIContext[T]) {.thread.} = waitFor ffiRun(ctx) +proc closeResources[T](ctx: ptr FFIContext[T]): Result[void, string] = + ## Closes file descriptors and deinits the lock. Does NOT free ctx memory. + ## Used by initContextResources error paths and pool destroy, where ctx is + ## not heap-allocated (pool slots live in a fixed array, not on the heap). + ctx.lock.deinitLock() + if not ctx.reqSignal.isNil(): + ?ctx.reqSignal.close() + if not ctx.reqReceivedSignal.isNil(): + ?ctx.reqReceivedSignal.close() + if not ctx.stopSignal.isNil(): + ?ctx.stopSignal.close() + if not ctx.threadExitSignal.isNil(): + ?ctx.threadExitSignal.close() + if not ctx.watchdogStopSignal.isNil(): + ?ctx.watchdogStopSignal.close() + return ok() + proc cleanUpResources[T](ctx: ptr FFIContext[T]): Result[void, string] = + ## Full cleanup for heap-allocated contexts: closes all resources and frees memory. defer: freeShared(ctx) - ctx.lock.deinitLock() - when defined(gcRefc): - ## ThreadSignalPtr.close() is intentionally skipped under --mm:refc. - ## - ## close() goes through chronos's safeUnregisterAndCloseFd, which calls - ## getThreadDispatcher() and lazily allocates a new Selector for the - ## main thread. With refc and a heavy ref-object graph torn down by the - ## FFI thread (libwaku/libp2p), that allocation traps inside rawNewObj - ## and the refc signal handler re-enters the same allocator — the - ## process never returns. Captured stack from a hung process: - ## close → safeUnregisterAndCloseFd → getThreadDispatcher → - ## newDispatcher → Selector.new → newObj (gc.nim:488) → - ## rawNewObj (gc.nim:470) → rawNewObj → _sigtramp → signalHandler → - ## newObjNoInit → addNewObjToZCT (infinite re-entry) - ## - ## --mm:orc does NOT exhibit this bug; see the - ## "destroyFFIContext refc workaround" suite in tests/test_ffi_context.nim - ## (test "destroy after heavy ref-allocation workload returns promptly"). - ## The signal fds (a few per ctx) are reclaimed by the OS at process - ## exit; destroyFFIContext is called once per process lifetime, so the - ## leak is bounded. - discard - else: - if not ctx.reqSignal.isNil(): - ?ctx.reqSignal.close() - if not ctx.reqReceivedSignal.isNil(): - ?ctx.reqReceivedSignal.close() - if not ctx.stopSignal.isNil(): - ?ctx.stopSignal.close() - if not ctx.threadExitSignal.isNil(): - ?ctx.threadExitSignal.close() - return ok() + return ctx.closeResources() -proc createFFIContext*[T](): Result[ptr FFIContext[T], string] = - ## This proc is called from the main thread and it creates - ## the FFI working thread. - var ctx = createShared(FFIContext[T], 1) +proc initContextResources[T](ctx: ptr FFIContext[T]): Result[void, string] = + ## Initialises all resources inside an already-allocated FFIContext slot. + ## On failure every partially-initialised resource is closed; the caller + ## is responsible for releasing the slot (freeShared or pool.releaseSlot). ctx.lock.initLock() var success = false @@ -330,6 +334,8 @@ proc createFFIContext*[T](): Result[ptr FFIContext[T], string] = err = error ctx.reqSignal = ThreadSignalPtr.new().valueOr: + ctx.closeResources().isOkOr: + return err("could not close resources after reqSignal failure: " & $error) return err("couldn't create reqSignal ThreadSignalPtr: " & $error) ctx.reqReceivedSignal = ThreadSignalPtr.new().valueOr: @@ -341,6 +347,12 @@ proc createFFIContext*[T](): Result[ptr FFIContext[T], string] = ctx.threadExitSignal = ThreadSignalPtr.new().valueOr: return err("couldn't create threadExitSignal ThreadSignalPtr: " & $error) + ctx.watchdogStopSignal = ThreadSignalPtr.new().valueOr: + ctx.closeResources().isOkOr: + return + err("could not close resources after watchdogStopSignal failure: " & $error) + return err("couldn't create watchdogStopSignal ThreadSignalPtr") + ctx.registeredRequests = addr ffi_types.registeredRequests ctx.running.store(true) @@ -348,6 +360,8 @@ proc createFFIContext*[T](): Result[ptr FFIContext[T], string] = try: createThread(ctx.ffiThread, ffiThreadBody[T], ctx) except ValueError, ResourceExhaustedError: + ctx.closeResources().isOkOr: + error "failed to close resources after ffiThread creation failure", err = error return err("failed to create the FFI thread: " & getCurrentExceptionMsg()) try: @@ -361,51 +375,121 @@ proc createFFIContext*[T](): Result[ptr FFIContext[T], string] = error "failed to signal ffiThread during watchdog cleanup", err = fireRes.error joinThread(ctx.ffiThread) + ctx.closeResources().isOkOr: + error "failed to close resources after watchdogThread creation failure", + err = error return err("failed to create the watchdog thread: " & getCurrentExceptionMsg()) registerCtx(cast[pointer](ctx)) success = true + return ok() + +# ── Pool helpers ───────────────────────────────────────────────────────────── + +proc acquireSlot[T](pool: var FFIContextPool[T]): Result[ptr FFIContext[T], string] = + for i in 0 ..< MaxFFIContexts: + var expected = false + if pool.inUse[i].compareExchange(expected, true): + return ok(pool.slots[i].addr) + return err("FFI context pool exhausted (max " & $MaxFFIContexts & " contexts)") + +proc releaseSlot[T](pool: var FFIContextPool[T], ctx: ptr FFIContext[T]) = + for i in 0 ..< MaxFFIContexts: + if pool.slots[i].addr == ctx: + pool.inUse[i].store(false) + return + +# ── Public API ──────────────────────────────────────────────────────────────── + +proc createFFIContext*[T](): Result[ptr FFIContext[T], string] = + ## Creates a heap-allocated FFI context. The caller must call destroyFFIContext(ctx) + ## to release it. Prefer the pool overload when the maximum context count is known. + var ctx = createShared(FFIContext[T], 1) + initContextResources(ctx).isOkOr: + freeShared(ctx) + return err(error) return ok(ctx) -proc destroyFFIContext*[T](ctx: ptr FFIContext[T]): Result[void, string] = - ## If the FFI thread's event loop is blocked by a synchronous handler - ## (e.g. blocking I/O), it cannot process reqSignal in time to exit. - ## In that case we leak ctx and the thread rather than hanging forever: - ## the thread will eventually exit on its own, but cleanup is skipped - ## because the thread may still be touching ctx fields. - const ThreadExitTimeout = 1500.milliseconds - unregisterCtx(cast[pointer](ctx)) +proc createFFIContext*[T]( + pool: var FFIContextPool[T] +): Result[ptr FFIContext[T], string] = + ## Acquires a slot from the fixed pool and initialises it as an FFI context. + ## Bounded fd usage: at most MaxFFIContexts * 2 ThreadSignalPtr fds are ever open. + let ctx = pool.acquireSlot().valueOr: + return err(error) + initContextResources(ctx).isOkOr: + pool.releaseSlot(ctx) + return err(error) + return ok(ctx) +proc signalStop[T](ctx: ptr FFIContext[T]): Result[void, string] = ctx.running.store(false) - - let signaledOnTime = ctx.reqSignal.fireSync().valueOr: + let ffiSignaled = ctx.reqSignal.fireSync().valueOr: ctx.onNotResponding() - return err("error in destroyFFIContext: " & $error) - if not signaledOnTime: + return err("error signaling reqSignal in destroyFFIContext: " & $error) + if not ffiSignaled: ctx.onNotResponding() return err("failed to signal reqSignal on time in destroyFFIContext") + let wdSignaled = ctx.watchdogStopSignal.fireSync().valueOr: + return err("error signaling watchdogStopSignal in destroyFFIContext: " & $error) + if not wdSignaled: + return err("failed to signal watchdogStopSignal on time in destroyFFIContext") + return ok() + +## If the FFI thread's event loop is blocked by a synchronous handler +## (e.g. blocking I/O), it cannot process reqSignal in time to exit. +## destroyFFIContext waits on threadExitSignal up to this bound; on timeout it +## returns err and skips joinThread/cleanup (leaking the thread + ctx slot) +## rather than hanging the caller forever. +const ThreadExitTimeout = 1500.milliseconds - ctx.stopSignal.fireSync().isOkOr: - error "failed to fire stopSignal in destroyFFIContext", err = $error +proc destroyFFIContext*[T](ctx: ptr FFIContext[T]): Result[void, string] = + ## Stops the FFI context that was created via createFFIContext[T]() (heap). + unregisterCtx(cast[pointer](ctx)) + + ctx.signalStop().isOkOr: + return err(error) - ## Bounded wait for ffiThread to exit. waitSync blocks the calling thread - ## up to the timeout; ffiThread fires threadExitSignal in its defer block. let exitedOnTime = ctx.threadExitSignal.waitSync(ThreadExitTimeout).valueOr: ctx.onNotResponding() return err("error waiting for FFI thread exit: " & $error) if not exitedOnTime: - ## Event loop is blocked by a synchronous handler. Leak the thread and - ## ctx to avoid hanging the caller forever. ctx.onNotResponding() return err("FFI thread did not exit in time; leaking ctx to avoid hang") joinThread(ctx.ffiThread) joinThread(ctx.watchdogThread) ctx.cleanUpResources().isOkOr: - error "failed to clean up resources in destroyFFIContext", err = error return err("cleanUpResources failed: " & $error) + return ok() +proc destroyFFIContext*[T]( + pool: var FFIContextPool[T], ctx: ptr FFIContext[T] +): Result[void, string] = + ## Stops the FFI context and returns its slot to the pool. If the FFI thread + ## is blocked and does not exit in time, the slot is leaked rather than + ## reclaimed — closing its resources while the thread is still live would be + ## unsafe. + unregisterCtx(cast[pointer](ctx)) + + ctx.signalStop().isOkOr: + return err(error) + + let exitedOnTime = ctx.threadExitSignal.waitSync(ThreadExitTimeout).valueOr: + ctx.onNotResponding() + return err("error waiting for FFI thread exit: " & $error) + + if not exitedOnTime: + ctx.onNotResponding() + return err("FFI thread did not exit in time; leaking pool slot to avoid hang") + + joinThread(ctx.ffiThread) + joinThread(ctx.watchdogThread) + ctx.closeResources().isOkOr: + pool.releaseSlot(ctx) + return err("closeResources failed: " & $error) + pool.releaseSlot(ctx) return ok() template checkParams*(ctx: ptr FFIContext, callback: FFICallBack, userData: pointer) = diff --git a/tests/test_ffi_context.nim b/tests/test_ffi_context.nim index 3ce05a8..9c30c80 100644 --- a/tests/test_ffi_context.nim +++ b/tests/test_ffi_context.nim @@ -121,6 +121,62 @@ registerReqFFI(HeavyRefAllocRequest, lib: ptr TestLib): await sleepAsync(10.milliseconds) return ok("heavy-done") +suite "FFIContextPool": + test "create and destroy via pool succeeds": + var pool: FFIContextPool[TestLib] + let ctx = pool.createFFIContext().valueOr: + assert false, "createFFIContext(pool) failed: " & $error + return + check pool.destroyFFIContext(ctx).isOk() + + test "slot is reused after destroy": + var pool: FFIContextPool[TestLib] + let ctx1 = pool.createFFIContext().valueOr: + assert false, "createFFIContext(pool) failed: " & $error + return + check pool.destroyFFIContext(ctx1).isOk() + # After destroying, the same slot must be available again + let ctx2 = pool.createFFIContext().valueOr: + assert false, "createFFIContext(pool) failed after slot release: " & $error + return + check pool.destroyFFIContext(ctx2).isOk() + check ctx1 == ctx2 # same array slot reused + + test "pool exhaustion returns error": + var pool: FFIContextPool[TestLib] + var ctxs: array[MaxFFIContexts, ptr FFIContext[TestLib]] + for i in 0 ..< MaxFFIContexts: + ctxs[i] = pool.createFFIContext().valueOr: + for j in 0 ..< i: + discard pool.destroyFFIContext(ctxs[j]) + assert false, "createFFIContext(pool) failed at slot " & $i & ": " & $error + return + # Pool is now full — next create must fail + check pool.createFFIContext().isErr() + for i in 0 ..< MaxFFIContexts: + discard pool.destroyFFIContext(ctxs[i]) + + test "requests are processed via pool context": + var pool: FFIContextPool[TestLib] + var d: CallbackData + initCallbackData(d) + defer: + deinitCallbackData(d) + + let ctx = pool.createFFIContext().valueOr: + assert false, "createFFIContext(pool) failed: " & $error + return + defer: + discard pool.destroyFFIContext(ctx) + + check sendRequestToFFIThread( + ctx, PingRequest.ffiNewReq(testCallback, addr d, "pool".cstring) + ) + .isOk() + waitCallback(d) + check d.retCode == RET_OK + check callbackMsg(d) == "pong:pool" + suite "createFFIContext / destroyFFIContext": test "create and destroy succeeds": let ctx = createFFIContext[TestLib]().valueOr: @@ -269,14 +325,19 @@ suite "sendRequestToFFIThread": test "successful request triggers RET_OK callback": var d: CallbackData initCallbackData(d) - defer: deinitCallbackData(d) + defer: + deinitCallbackData(d) let ctx = createFFIContext[TestLib]().valueOr: check false return - defer: discard destroyFFIContext(ctx) + defer: + discard destroyFFIContext(ctx) - check sendRequestToFFIThread(ctx, PingRequest.ffiNewReq(testCallback, addr d, "hello".cstring)).isOk() + check sendRequestToFFIThread( + ctx, PingRequest.ffiNewReq(testCallback, addr d, "hello".cstring) + ) + .isOk() waitCallback(d) check d.retCode == RET_OK check callbackMsg(d) == "pong:hello" @@ -284,12 +345,14 @@ suite "sendRequestToFFIThread": test "failing request triggers RET_ERR callback": var d: CallbackData initCallbackData(d) - defer: deinitCallbackData(d) + defer: + deinitCallbackData(d) let ctx = createFFIContext[TestLib]().valueOr: check false return - defer: discard destroyFFIContext(ctx) + defer: + discard destroyFFIContext(ctx) check sendRequestToFFIThread(ctx, FailRequest.ffiNewReq(testCallback, addr d)).isOk() waitCallback(d) @@ -298,14 +361,17 @@ suite "sendRequestToFFIThread": test "empty ok response delivers empty message": var d: CallbackData initCallbackData(d) - defer: deinitCallbackData(d) + defer: + deinitCallbackData(d) let ctx = createFFIContext[TestLib]().valueOr: check false return - defer: discard destroyFFIContext(ctx) + defer: + discard destroyFFIContext(ctx) - check sendRequestToFFIThread(ctx, EmptyOkRequest.ffiNewReq(testCallback, addr d)).isOk() + check sendRequestToFFIThread(ctx, EmptyOkRequest.ffiNewReq(testCallback, addr d)) + .isOk() waitCallback(d) check d.retCode == RET_OK check d.msgLen == 0 @@ -314,13 +380,17 @@ suite "sendRequestToFFIThread": let ctx = createFFIContext[TestLib]().valueOr: check false return - defer: discard destroyFFIContext(ctx) + defer: + discard destroyFFIContext(ctx) for i in 1 .. 5: var d: CallbackData initCallbackData(d) let msg = "msg" & $i - check sendRequestToFFIThread(ctx, PingRequest.ffiNewReq(testCallback, addr d, msg.cstring)).isOk() + check sendRequestToFFIThread( + ctx, PingRequest.ffiNewReq(testCallback, addr d, msg.cstring) + ) + .isOk() waitCallback(d) deinitCallbackData(d) check d.retCode == RET_OK From a90453faa1a77a7c780ea0de4b378bd9a84a4590 Mon Sep 17 00:00:00 2001 From: Ivan FB Date: Sat, 2 May 2026 00:56:43 +0200 Subject: [PATCH 02/11] better code separation and better error handling --- ffi.nim | 5 ++-- ffi/ffi_context.nim | 58 +++++++--------------------------------- ffi/ffi_context_pool.nim | 56 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 68 insertions(+), 51 deletions(-) create mode 100644 ffi/ffi_context_pool.nim diff --git a/ffi.nim b/ffi.nim index 8ad9285..24186bb 100644 --- a/ffi.nim +++ b/ffi.nim @@ -2,9 +2,10 @@ import std/[atomics, tables] import chronos, chronicles import ffi/internal/[ffi_library, ffi_macro], - ffi/[alloc, ffi_types, ffi_context, ffi_thread_request, serial] + ffi/[alloc, ffi_types, ffi_context, ffi_context_pool, ffi_thread_request, serial] export atomics, tables export chronos, chronicles export - atomics, alloc, ffi_library, ffi_macro, ffi_types, ffi_context, ffi_thread_request, serial + atomics, alloc, ffi_library, ffi_macro, ffi_types, ffi_context, ffi_context_pool, + ffi_thread_request, serial diff --git a/ffi/ffi_context.nim b/ffi/ffi_context.nim index dc25dd7..ca725fb 100644 --- a/ffi/ffi_context.nim +++ b/ffi/ffi_context.nim @@ -41,18 +41,6 @@ type FFIContext*[T] = object var ffiCurrentCallbackState* {.threadvar.}: ptr FFICallbackState ## Set by ffiThreadBody at thread startup; read by dispatchFfiEvent. -const MaxFFIContexts* = 32 - ## Maximum number of concurrently live FFI contexts when using FFIContextPool. - ## Fds and threads are only consumed for slots that are actually acquired, - ## so this value only affects the upfront memory of the pool array. - -type FFIContextPool*[T] = object - ## Fixed-size pool of FFI contexts. Avoids dynamic heap allocation per context - ## and bounds the total number of file descriptors consumed by ThreadSignalPtrs - ## to at most MaxFFIContexts * 2. - slots: array[MaxFFIContexts, FFIContext[T]] - inUse: array[MaxFFIContexts, Atomic[bool]] - const git_version* {.strdefine.} = "n/a" var contextRegistry = initHashSet[pointer]() @@ -190,14 +178,14 @@ proc watchdogThreadBody(ctx: ptr FFIContext) {.thread.} = # waitSync returns early if watchdogStopSignal fires (i.e. on destroy). let startWait = ctx.watchdogStopSignal.waitSync(WatchdogStartDelay) if startWait.isErr(): - error "watchdog: start-delay waitSync failed", err = startWait.error + error "watchdog: start-delay waitSync failed", error = startWait.error elif startWait.get(): return # stop signal fired during start delay while ctx.running.load: let intervalWait = ctx.watchdogStopSignal.waitSync(WatchdogTimeinterval) if intervalWait.isErr(): - error "watchdog: interval waitSync failed", err = intervalWait.error + error "watchdog: interval waitSync failed", error = intervalWait.error elif intervalWait.get() or not ctx.running.load: break @@ -254,7 +242,7 @@ proc processRequest[T]( try: handleRes(res, request) except Exception as exc: - error "Unexpected exception in handleRes", exc = exc.msg + error "Unexpected exception in handleRes", error = exc.msg proc ffiThreadBody[T](ctx: ptr FFIContext[T]) {.thread.} = ## FFI thread body that attends library user API requests @@ -297,7 +285,7 @@ proc ffiThreadBody[T](ctx: ptr FFIContext[T]) {.thread.} = waitFor ffiRun(ctx) -proc closeResources[T](ctx: ptr FFIContext[T]): Result[void, string] = +proc closeResources*[T](ctx: ptr FFIContext[T]): Result[void, string] = ## Closes file descriptors and deinits the lock. Does NOT free ctx memory. ## Used by initContextResources error paths and pool destroy, where ctx is ## not heap-allocated (pool slots live in a fixed array, not on the heap). @@ -320,7 +308,7 @@ proc cleanUpResources[T](ctx: ptr FFIContext[T]): Result[void, string] = freeShared(ctx) return ctx.closeResources() -proc initContextResources[T](ctx: ptr FFIContext[T]): Result[void, string] = +proc initContextResources*[T](ctx: ptr FFIContext[T]): Result[void, string] = ## Initialises all resources inside an already-allocated FFIContext slot. ## On failure every partially-initialised resource is closed; the caller ## is responsible for releasing the slot (freeShared or pool.releaseSlot). @@ -361,7 +349,7 @@ proc initContextResources[T](ctx: ptr FFIContext[T]): Result[void, string] = createThread(ctx.ffiThread, ffiThreadBody[T], ctx) except ValueError, ResourceExhaustedError: ctx.closeResources().isOkOr: - error "failed to close resources after ffiThread creation failure", err = error + error "failed to close resources after ffiThread creation failure", error = error return err("failed to create the FFI thread: " & getCurrentExceptionMsg()) try: @@ -372,33 +360,17 @@ proc initContextResources[T](ctx: ptr FFIContext[T]): Result[void, string] = ctx.running.store(false) let fireRes = ctx.reqSignal.fireSync() if fireRes.isErr(): - error "failed to signal ffiThread during watchdog cleanup", - err = fireRes.error + error "failed to signal ffiThread during watchdog cleanup", error = fireRes.error joinThread(ctx.ffiThread) ctx.closeResources().isOkOr: error "failed to close resources after watchdogThread creation failure", - err = error + error = error return err("failed to create the watchdog thread: " & getCurrentExceptionMsg()) registerCtx(cast[pointer](ctx)) success = true return ok() -# ── Pool helpers ───────────────────────────────────────────────────────────── - -proc acquireSlot[T](pool: var FFIContextPool[T]): Result[ptr FFIContext[T], string] = - for i in 0 ..< MaxFFIContexts: - var expected = false - if pool.inUse[i].compareExchange(expected, true): - return ok(pool.slots[i].addr) - return err("FFI context pool exhausted (max " & $MaxFFIContexts & " contexts)") - -proc releaseSlot[T](pool: var FFIContextPool[T], ctx: ptr FFIContext[T]) = - for i in 0 ..< MaxFFIContexts: - if pool.slots[i].addr == ctx: - pool.inUse[i].store(false) - return - # ── Public API ──────────────────────────────────────────────────────────────── proc createFFIContext*[T](): Result[ptr FFIContext[T], string] = @@ -410,19 +382,7 @@ proc createFFIContext*[T](): Result[ptr FFIContext[T], string] = return err(error) return ok(ctx) -proc createFFIContext*[T]( - pool: var FFIContextPool[T] -): Result[ptr FFIContext[T], string] = - ## Acquires a slot from the fixed pool and initialises it as an FFI context. - ## Bounded fd usage: at most MaxFFIContexts * 2 ThreadSignalPtr fds are ever open. - let ctx = pool.acquireSlot().valueOr: - return err(error) - initContextResources(ctx).isOkOr: - pool.releaseSlot(ctx) - return err(error) - return ok(ctx) - -proc signalStop[T](ctx: ptr FFIContext[T]): Result[void, string] = +proc signalStop*[T](ctx: ptr FFIContext[T]): Result[void, string] = ctx.running.store(false) let ffiSignaled = ctx.reqSignal.fireSync().valueOr: ctx.onNotResponding() diff --git a/ffi/ffi_context_pool.nim b/ffi/ffi_context_pool.nim new file mode 100644 index 0000000..e0985c6 --- /dev/null +++ b/ffi/ffi_context_pool.nim @@ -0,0 +1,56 @@ +import std/atomics +import results, chronicles +import ./ffi_context + +export ffi_context + +const MaxFFIContexts* = 32 + ## Maximum number of concurrently live FFI contexts. + ## Fds and threads are only consumed for slots that are actually acquired, + ## so this value only affects the upfront memory of the pool array. + +type FFIContextPool*[T] = object + ## Fixed-size pool of FFI contexts. Avoids dynamic heap allocation per context + ## and bounds the total number of file descriptors consumed by ThreadSignalPtrs + ## to at most MaxFFIContexts * 2. + slots: array[MaxFFIContexts, FFIContext[T]] + inUse: array[MaxFFIContexts, Atomic[bool]] + +proc acquireSlot[T](pool: var FFIContextPool[T]): Result[ptr FFIContext[T], string] = + for i in 0 ..< MaxFFIContexts: + var expected = false + if pool.inUse[i].compareExchange(expected, true): + return ok(pool.slots[i].addr) + return err("FFI context pool exhausted; max: " & $MaxFFIContexts & " contexts)") + +proc releaseSlot[T](pool: var FFIContextPool[T], ctx: ptr FFIContext[T]) = + for i in 0 ..< MaxFFIContexts: + if pool.slots[i].addr == ctx: + pool.inUse[i].store(false) + return + +proc createFFIContext*[T]( + pool: var FFIContextPool[T] +): Result[ptr FFIContext[T], string] = + ## Acquires a slot from the fixed pool and initialises it as an FFI context. + ## Bounded fd usage: at most MaxFFIContexts * 2 ThreadSignalPtr fds are ever open. + let ctx = pool.acquireSlot().valueOr: + return err("failure calling acquireSlot in createFFIContext: " & $error) + initContextResources(ctx).isOkOr: + pool.releaseSlot(ctx) + return err("failure calling initContextResources in createFFIContext: " & $error) + return ok(ctx) + +proc destroyFFIContext*[T]( + pool: var FFIContextPool[T], ctx: ptr FFIContext[T] +): Result[void, string] = + ## Stops the FFI context and returns its slot to the pool. + defer: + joinFFIThreads(ctx) + ctx.closeResources().isOkOr: + error "failed to close resources in destroyFFIContext", error = error + return err("failure calling closeResources in destroyFFIContext: " & $error) + pool.releaseSlot(ctx) + ctx.signalStop().isOkOr: + return err("failure calling signalStop in destroyFFIContext: " & $error) + return ok() From 32246792df0edfa068d242c686ffc872ac6138ac Mon Sep 17 00:00:00 2001 From: Ivan FB Date: Tue, 12 May 2026 01:54:32 +0200 Subject: [PATCH 03/11] restore back pool logic --- ffi/ffi_context.nim | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/ffi/ffi_context.nim b/ffi/ffi_context.nim index ca725fb..5492b8e 100644 --- a/ffi/ffi_context.nim +++ b/ffi/ffi_context.nim @@ -371,6 +371,31 @@ proc initContextResources*[T](ctx: ptr FFIContext[T]): Result[void, string] = success = true return ok() +const MaxFFIContexts* = 32 + ## Maximum number of concurrently live FFI contexts when using FFIContextPool. + ## Fds and threads are only consumed for slots that are actually acquired, + ## so this value only affects the upfront memory of the pool array. + +type FFIContextPool*[T] = object + ## Fixed-size pool of FFI contexts. Avoids dynamic heap allocation per context + ## and bounds the total number of file descriptors consumed by ThreadSignalPtrs + ## to at most MaxFFIContexts * 2. + slots: array[MaxFFIContexts, FFIContext[T]] + inUse: array[MaxFFIContexts, Atomic[bool]] + +proc acquireSlot[T](pool: var FFIContextPool[T]): Result[ptr FFIContext[T], string] = + for i in 0 ..< MaxFFIContexts: + var expected = false + if pool.inUse[i].compareExchange(expected, true): + return ok(pool.slots[i].addr) + return err("FFI context pool exhausted (max " & $MaxFFIContexts & " contexts)") + +proc releaseSlot[T](pool: var FFIContextPool[T], ctx: ptr FFIContext[T]) = + for i in 0 ..< MaxFFIContexts: + if pool.slots[i].addr == ctx: + pool.inUse[i].store(false) + return + # ── Public API ──────────────────────────────────────────────────────────────── proc createFFIContext*[T](): Result[ptr FFIContext[T], string] = @@ -382,6 +407,18 @@ proc createFFIContext*[T](): Result[ptr FFIContext[T], string] = return err(error) return ok(ctx) +proc createFFIContext*[T]( + pool: var FFIContextPool[T] +): Result[ptr FFIContext[T], string] = + ## Acquires a slot from the fixed pool and initialises it as an FFI context. + ## Bounded fd usage: at most MaxFFIContexts * 2 ThreadSignalPtr fds are ever open. + let ctx = pool.acquireSlot().valueOr: + return err(error) + initContextResources(ctx).isOkOr: + pool.releaseSlot(ctx) + return err(error) + return ok(ctx) + proc signalStop*[T](ctx: ptr FFIContext[T]): Result[void, string] = ctx.running.store(false) let ffiSignaled = ctx.reqSignal.fireSync().valueOr: From 9469f71fe41dfa54fec7d3c6c55d36b5eee5e16a Mon Sep 17 00:00:00 2001 From: Ivan FB Date: Tue, 12 May 2026 02:02:58 +0200 Subject: [PATCH 04/11] fix compilation issue after rebase --- ffi.nim | 4 +-- ffi/ffi_context_pool.nim | 56 ---------------------------------------- tests/test_serial.nim | 4 --- 3 files changed, 2 insertions(+), 62 deletions(-) delete mode 100644 ffi/ffi_context_pool.nim diff --git a/ffi.nim b/ffi.nim index 24186bb..a47ac2f 100644 --- a/ffi.nim +++ b/ffi.nim @@ -2,10 +2,10 @@ import std/[atomics, tables] import chronos, chronicles import ffi/internal/[ffi_library, ffi_macro], - ffi/[alloc, ffi_types, ffi_context, ffi_context_pool, ffi_thread_request, serial] + ffi/[alloc, ffi_types, ffi_context, ffi_thread_request, serial] export atomics, tables export chronos, chronicles export - atomics, alloc, ffi_library, ffi_macro, ffi_types, ffi_context, ffi_context_pool, + atomics, alloc, ffi_library, ffi_macro, ffi_types, ffi_context, ffi_thread_request, serial diff --git a/ffi/ffi_context_pool.nim b/ffi/ffi_context_pool.nim deleted file mode 100644 index e0985c6..0000000 --- a/ffi/ffi_context_pool.nim +++ /dev/null @@ -1,56 +0,0 @@ -import std/atomics -import results, chronicles -import ./ffi_context - -export ffi_context - -const MaxFFIContexts* = 32 - ## Maximum number of concurrently live FFI contexts. - ## Fds and threads are only consumed for slots that are actually acquired, - ## so this value only affects the upfront memory of the pool array. - -type FFIContextPool*[T] = object - ## Fixed-size pool of FFI contexts. Avoids dynamic heap allocation per context - ## and bounds the total number of file descriptors consumed by ThreadSignalPtrs - ## to at most MaxFFIContexts * 2. - slots: array[MaxFFIContexts, FFIContext[T]] - inUse: array[MaxFFIContexts, Atomic[bool]] - -proc acquireSlot[T](pool: var FFIContextPool[T]): Result[ptr FFIContext[T], string] = - for i in 0 ..< MaxFFIContexts: - var expected = false - if pool.inUse[i].compareExchange(expected, true): - return ok(pool.slots[i].addr) - return err("FFI context pool exhausted; max: " & $MaxFFIContexts & " contexts)") - -proc releaseSlot[T](pool: var FFIContextPool[T], ctx: ptr FFIContext[T]) = - for i in 0 ..< MaxFFIContexts: - if pool.slots[i].addr == ctx: - pool.inUse[i].store(false) - return - -proc createFFIContext*[T]( - pool: var FFIContextPool[T] -): Result[ptr FFIContext[T], string] = - ## Acquires a slot from the fixed pool and initialises it as an FFI context. - ## Bounded fd usage: at most MaxFFIContexts * 2 ThreadSignalPtr fds are ever open. - let ctx = pool.acquireSlot().valueOr: - return err("failure calling acquireSlot in createFFIContext: " & $error) - initContextResources(ctx).isOkOr: - pool.releaseSlot(ctx) - return err("failure calling initContextResources in createFFIContext: " & $error) - return ok(ctx) - -proc destroyFFIContext*[T]( - pool: var FFIContextPool[T], ctx: ptr FFIContext[T] -): Result[void, string] = - ## Stops the FFI context and returns its slot to the pool. - defer: - joinFFIThreads(ctx) - ctx.closeResources().isOkOr: - error "failed to close resources in destroyFFIContext", error = error - return err("failure calling closeResources in destroyFFIContext: " & $error) - pool.releaseSlot(ctx) - ctx.signalStop().isOkOr: - return err("failure calling signalStop in destroyFFIContext: " & $error) - return ok() diff --git a/tests/test_serial.nim b/tests/test_serial.nim index 24b4f51..df7a4d5 100644 --- a/tests/test_serial.nim +++ b/tests/test_serial.nim @@ -104,10 +104,6 @@ suite "ffiDeserialize error handling": let back = ffiDeserialize("not json at all".cstring, int) check back.isErr() - test "wrong JSON type returns err for string": - let back = ffiDeserialize("42".cstring, string) - check back.isErr() - test "malformed JSON for object returns err": let back = ffiDeserialize("{bad json".cstring, Point) check back.isErr() From 66d48fdaa6adf71c03b8b8a4369d31d2476aeb3a Mon Sep 17 00:00:00 2001 From: Ivan FB Date: Tue, 12 May 2026 02:55:36 +0200 Subject: [PATCH 05/11] fix compilation issues after rebase --- ffi/ffi_context.nim | 108 ++++++++++++++++--------------------- ffi/internal/ffi_macro.nim | 21 ++++++-- tests/test_ffi_context.nim | 53 ++++++++++-------- tests/test_gc_compat.nim | 20 ++++--- 4 files changed, 105 insertions(+), 97 deletions(-) diff --git a/ffi/ffi_context.nim b/ffi/ffi_context.nim index 5492b8e..eb17ae8 100644 --- a/ffi/ffi_context.nim +++ b/ffi/ffi_context.nim @@ -30,8 +30,6 @@ type FFIContext*[T] = object # fired by ffiThread just before it exits; destroyFFIContext waits on # this with a bounded timeout instead of joining unconditionally, so a # blocked event loop cannot hang the caller forever - watchdogStopSignal: ThreadSignalPtr - # fired by destroyFFIContext so the watchdog exits immediately instead of waiting out its sleep userData*: pointer callbackState*: FFICallbackState running: Atomic[bool] # To control when the threads are running @@ -174,19 +172,16 @@ proc watchdogThreadBody(ctx: ptr FFIContext) {.thread.} = const WatchdogTimeinterval = 1.seconds const WatchdogTimeout = 20.seconds - # Give time for the node to be created and up before sending watchdog requests. - # waitSync returns early if watchdogStopSignal fires (i.e. on destroy). - let startWait = ctx.watchdogStopSignal.waitSync(WatchdogStartDelay) - if startWait.isErr(): - error "watchdog: start-delay waitSync failed", error = startWait.error - elif startWait.get(): - return # stop signal fired during start delay - - while ctx.running.load: - let intervalWait = ctx.watchdogStopSignal.waitSync(WatchdogTimeinterval) - if intervalWait.isErr(): - error "watchdog: interval waitSync failed", error = intervalWait.error - elif intervalWait.get() or not ctx.running.load: + # Give time for the node to be created and up before sending watchdog requests + let initialStop = await ctx.stopSignal.wait().withTimeout(WatchdogStartDelay) + if initialStop or ctx.running.load == false: + return + + while true: + let intervalStop = await ctx.stopSignal.wait().withTimeout(WatchdogTimeinterval) + + if intervalStop or ctx.running.load == false: + debug "Watchdog thread exiting because FFIContext is not running" break let callback = proc( @@ -285,28 +280,42 @@ proc ffiThreadBody[T](ctx: ptr FFIContext[T]) {.thread.} = waitFor ffiRun(ctx) -proc closeResources*[T](ctx: ptr FFIContext[T]): Result[void, string] = - ## Closes file descriptors and deinits the lock. Does NOT free ctx memory. - ## Used by initContextResources error paths and pool destroy, where ctx is - ## not heap-allocated (pool slots live in a fixed array, not on the heap). - ctx.lock.deinitLock() - if not ctx.reqSignal.isNil(): - ?ctx.reqSignal.close() - if not ctx.reqReceivedSignal.isNil(): - ?ctx.reqReceivedSignal.close() - if not ctx.stopSignal.isNil(): - ?ctx.stopSignal.close() - if not ctx.threadExitSignal.isNil(): - ?ctx.threadExitSignal.close() - if not ctx.watchdogStopSignal.isNil(): - ?ctx.watchdogStopSignal.close() - return ok() - proc cleanUpResources[T](ctx: ptr FFIContext[T]): Result[void, string] = ## Full cleanup for heap-allocated contexts: closes all resources and frees memory. defer: freeShared(ctx) - return ctx.closeResources() + ctx.lock.deinitLock() + when defined(gcRefc): + ## ThreadSignalPtr.close() is intentionally skipped under --mm:refc. + ## + ## close() goes through chronos's safeUnregisterAndCloseFd, which calls + ## getThreadDispatcher() and lazily allocates a new Selector for the + ## main thread. With refc and a heavy ref-object graph torn down by the + ## FFI thread (libwaku/libp2p), that allocation traps inside rawNewObj + ## and the refc signal handler re-enters the same allocator — the + ## process never returns. Captured stack from a hung process: + ## close → safeUnregisterAndCloseFd → getThreadDispatcher → + ## newDispatcher → Selector.new → newObj (gc.nim:488) → + ## rawNewObj (gc.nim:470) → rawNewObj → _sigtramp → signalHandler → + ## newObjNoInit → addNewObjToZCT (infinite re-entry) + ## + ## --mm:orc does NOT exhibit this bug; see the + ## "destroyFFIContext refc workaround" suite in tests/test_ffi_context.nim + ## (test "destroy after heavy ref-allocation workload returns promptly"). + ## The signal fds (a few per ctx) are reclaimed by the OS at process + ## exit; destroyFFIContext is called once per process lifetime, so the + ## leak is bounded. + discard + else: + if not ctx.reqSignal.isNil(): + ?ctx.reqSignal.close() + if not ctx.reqReceivedSignal.isNil(): + ?ctx.reqReceivedSignal.close() + if not ctx.stopSignal.isNil(): + ?ctx.stopSignal.close() + if not ctx.threadExitSignal.isNil(): + ?ctx.threadExitSignal.close() + return ok() proc initContextResources*[T](ctx: ptr FFIContext[T]): Result[void, string] = ## Initialises all resources inside an already-allocated FFIContext slot. @@ -322,8 +331,6 @@ proc initContextResources*[T](ctx: ptr FFIContext[T]): Result[void, string] = err = error ctx.reqSignal = ThreadSignalPtr.new().valueOr: - ctx.closeResources().isOkOr: - return err("could not close resources after reqSignal failure: " & $error) return err("couldn't create reqSignal ThreadSignalPtr: " & $error) ctx.reqReceivedSignal = ThreadSignalPtr.new().valueOr: @@ -335,12 +342,6 @@ proc initContextResources*[T](ctx: ptr FFIContext[T]): Result[void, string] = ctx.threadExitSignal = ThreadSignalPtr.new().valueOr: return err("couldn't create threadExitSignal ThreadSignalPtr: " & $error) - ctx.watchdogStopSignal = ThreadSignalPtr.new().valueOr: - ctx.closeResources().isOkOr: - return - err("could not close resources after watchdogStopSignal failure: " & $error) - return err("couldn't create watchdogStopSignal ThreadSignalPtr") - ctx.registeredRequests = addr ffi_types.registeredRequests ctx.running.store(true) @@ -348,8 +349,6 @@ proc initContextResources*[T](ctx: ptr FFIContext[T]): Result[void, string] = try: createThread(ctx.ffiThread, ffiThreadBody[T], ctx) except ValueError, ResourceExhaustedError: - ctx.closeResources().isOkOr: - error "failed to close resources after ffiThread creation failure", error = error return err("failed to create the FFI thread: " & getCurrentExceptionMsg()) try: @@ -362,9 +361,6 @@ proc initContextResources*[T](ctx: ptr FFIContext[T]): Result[void, string] = if fireRes.isErr(): error "failed to signal ffiThread during watchdog cleanup", error = fireRes.error joinThread(ctx.ffiThread) - ctx.closeResources().isOkOr: - error "failed to close resources after watchdogThread creation failure", - error = error return err("failed to create the watchdog thread: " & getCurrentExceptionMsg()) registerCtx(cast[pointer](ctx)) @@ -396,17 +392,6 @@ proc releaseSlot[T](pool: var FFIContextPool[T], ctx: ptr FFIContext[T]) = pool.inUse[i].store(false) return -# ── Public API ──────────────────────────────────────────────────────────────── - -proc createFFIContext*[T](): Result[ptr FFIContext[T], string] = - ## Creates a heap-allocated FFI context. The caller must call destroyFFIContext(ctx) - ## to release it. Prefer the pool overload when the maximum context count is known. - var ctx = createShared(FFIContext[T], 1) - initContextResources(ctx).isOkOr: - freeShared(ctx) - return err(error) - return ok(ctx) - proc createFFIContext*[T]( pool: var FFIContextPool[T] ): Result[ptr FFIContext[T], string] = @@ -427,10 +412,10 @@ proc signalStop*[T](ctx: ptr FFIContext[T]): Result[void, string] = if not ffiSignaled: ctx.onNotResponding() return err("failed to signal reqSignal on time in destroyFFIContext") - let wdSignaled = ctx.watchdogStopSignal.fireSync().valueOr: - return err("error signaling watchdogStopSignal in destroyFFIContext: " & $error) + let wdSignaled = ctx.stopSignal.fireSync().valueOr: + return err("error signaling stopSignal in destroyFFIContext: " & $error) if not wdSignaled: - return err("failed to signal watchdogStopSignal on time in destroyFFIContext") + return err("failed to signal stopSignal on time in destroyFFIContext") return ok() ## If the FFI thread's event loop is blocked by a synchronous handler @@ -483,9 +468,6 @@ proc destroyFFIContext*[T]( joinThread(ctx.ffiThread) joinThread(ctx.watchdogThread) - ctx.closeResources().isOkOr: - pool.releaseSlot(ctx) - return err("closeResources failed: " & $error) pool.releaseSlot(ctx) return ok() diff --git a/ffi/internal/ffi_macro.nim b/ffi/internal/ffi_macro.nim index 46642b0..f310983 100644 --- a/ffi/internal/ffi_macro.nim +++ b/ffi/internal/ffi_macro.nim @@ -1383,9 +1383,12 @@ macro ffiCtor*(prc: untyped): untyped = # Use a gensym'd ctx identifier so both the let binding and usage match let ctxSym = genSym(nskLet, "ctx") + # Module-level pool shared by ctor and dtor for this libType + let poolIdent = ident($libTypeName & "FFIPool") + # Create the FFIContext synchronously; return nil on failure ffiBody.add quote do: - let `ctxSym` = createFFIContext[`libTypeName`]().valueOr: + let `ctxSym` = `poolIdent`.createFFIContext().valueOr: if not callback.isNil: let errStr = "ffiCtor: failed to create FFIContext: " & $error callback(RET_ERR, unsafeAddr errStr[0], cast[csize_t](errStr.len), userData) @@ -1476,8 +1479,13 @@ macro ffiCtor*(prc: untyped): untyped = ) ) + let poolDecl = quote do: + when not declared(`poolIdent`): + var `poolIdent`: FFIContextPool[`libTypeName`] + result = newStmtList( - typeDef, deleteProc, ffiNewReqProc, helperProc, processProc, addToReg, ffiProc + typeDef, deleteProc, ffiNewReqProc, helperProc, processProc, addToReg, poolDecl, + ffiProc, ) when defined(ffiDumpMacros): @@ -1548,9 +1556,10 @@ macro ffiDtor*(prc: untyped): untyped = if not isNoop: ffiBody.add(bodyNode) + let poolIdent = ident($libTypeName & "FFIPool") ffiBody.add quote do: let `destroyResIdent` = - destroyFFIContext[`libTypeName`](cast[ptr FFIContext[`libTypeName`]](ctx)) + `poolIdent`.destroyFFIContext(cast[ptr FFIContext[`libTypeName`]](ctx)) if `destroyResIdent`.isErr(): if not callback.isNil: let errStr = "destroy failed: " & $`destroyResIdent`.error @@ -1593,7 +1602,11 @@ macro ffiDtor*(prc: untyped): untyped = ) ) - result = ffiProc + let poolDecl = quote do: + when not declared(`poolIdent`): + var `poolIdent`: FFIContextPool[`libTypeName`] + + result = newStmtList(poolDecl, ffiProc) when defined(ffiDumpMacros): echo result.repr diff --git a/tests/test_ffi_context.nim b/tests/test_ffi_context.nim index 9c30c80..dafc4e3 100644 --- a/tests/test_ffi_context.nim +++ b/tests/test_ffi_context.nim @@ -179,17 +179,19 @@ suite "FFIContextPool": suite "createFFIContext / destroyFFIContext": test "create and destroy succeeds": - let ctx = createFFIContext[TestLib]().valueOr: + var pool: FFIContextPool[TestLib] + let ctx = pool.createFFIContext().valueOr: checkpoint "createFFIContext failed: " & $error check false return - check destroyFFIContext(ctx).isOk() + check pool.destroyFFIContext(ctx).isOk() test "double destroy is safe via running flag": - let ctx = createFFIContext[TestLib]().valueOr: + var pool: FFIContextPool[TestLib] + let ctx = pool.createFFIContext().valueOr: check false return - check destroyFFIContext(ctx).isOk() + check pool.destroyFFIContext(ctx).isOk() suite "destroyFFIContext does not hang": test "destroy while a slow async request is still in-flight": @@ -197,7 +199,8 @@ suite "destroyFFIContext does not hang": ## running async request (e.g. stop_node / w.stop()) was still executing. ## The destroy must return well within 2 seconds; before the fix it would ## block forever on joinThread(ffiThread). - let ctx = createFFIContext[TestLib]().valueOr: + var pool: FFIContextPool[TestLib] + let ctx = pool.createFFIContext().valueOr: check false return @@ -213,7 +216,7 @@ suite "destroyFFIContext does not hang": # Destroy immediately while SlowRequest is still running. let t0 = Moment.now() - check destroyFFIContext(ctx).isOk() + check pool.destroyFFIContext(ctx).isOk() check (Moment.now() - t0) < 2.seconds suite "destroyFFIContext does not hang when event loop is blocked": @@ -230,7 +233,8 @@ suite "destroyFFIContext does not hang when event loop is blocked": ## ## With the fix, destroyFFIContext must complete well within the 5 s that ## SyncBlockingRequest holds the event loop. - let ctx = createFFIContext[TestLib]().valueOr: + var pool: FFIContextPool[TestLib] + let ctx = pool.createFFIContext().valueOr: check false return @@ -255,7 +259,7 @@ suite "destroyFFIContext does not hang when event loop is blocked": # It deliberately returns err and leaks ctx in this scenario rather than # hanging on joinThread. let t0 = Moment.now() - check destroyFFIContext(ctx).isErr() + check pool.destroyFFIContext(ctx).isErr() check (Moment.now() - t0) < 3.seconds # Drain the leaked thread before the test scope ends. @@ -303,7 +307,8 @@ suite "destroyFFIContext refc workaround": ## returns immediately. Under `--mm:orc` it returns immediately either ## way. test "destroy after heavy ref-allocation workload returns promptly": - let ctx = createFFIContext[TestLib]().valueOr: + var pool: FFIContextPool[TestLib] + let ctx = pool.createFFIContext().valueOr: check false return @@ -318,7 +323,7 @@ suite "destroyFFIContext refc workaround": check d.retCode == RET_OK let t0 = Moment.now() - check destroyFFIContext(ctx).isOk() + check pool.destroyFFIContext(ctx).isOk() check (Moment.now() - t0) < 3.seconds suite "sendRequestToFFIThread": @@ -328,11 +333,12 @@ suite "sendRequestToFFIThread": defer: deinitCallbackData(d) - let ctx = createFFIContext[TestLib]().valueOr: + var pool: FFIContextPool[TestLib] + let ctx = pool.createFFIContext().valueOr: check false return defer: - discard destroyFFIContext(ctx) + discard pool.destroyFFIContext(ctx) check sendRequestToFFIThread( ctx, PingRequest.ffiNewReq(testCallback, addr d, "hello".cstring) @@ -348,11 +354,12 @@ suite "sendRequestToFFIThread": defer: deinitCallbackData(d) - let ctx = createFFIContext[TestLib]().valueOr: + var pool: FFIContextPool[TestLib] + let ctx = pool.createFFIContext().valueOr: check false return defer: - discard destroyFFIContext(ctx) + discard pool.destroyFFIContext(ctx) check sendRequestToFFIThread(ctx, FailRequest.ffiNewReq(testCallback, addr d)).isOk() waitCallback(d) @@ -364,11 +371,12 @@ suite "sendRequestToFFIThread": defer: deinitCallbackData(d) - let ctx = createFFIContext[TestLib]().valueOr: + var pool: FFIContextPool[TestLib] + let ctx = pool.createFFIContext().valueOr: check false return defer: - discard destroyFFIContext(ctx) + discard pool.destroyFFIContext(ctx) check sendRequestToFFIThread(ctx, EmptyOkRequest.ffiNewReq(testCallback, addr d)) .isOk() @@ -377,11 +385,12 @@ suite "sendRequestToFFIThread": check d.msgLen == 0 test "sequential requests are all processed": - let ctx = createFFIContext[TestLib]().valueOr: + var pool: FFIContextPool[TestLib] + let ctx = pool.createFFIContext().valueOr: check false return defer: - discard destroyFFIContext(ctx) + discard pool.destroyFFIContext(ctx) for i in 1 .. 5: var d: CallbackData @@ -439,7 +448,7 @@ suite "ffiCtor macro": check not ctx[].myLib.isNil check ctx[].myLib[].value == 42 - check destroyFFIContext(ctx).isOk() + check SimpleLibFFIPool.destroyFFIContext(ctx).isOk() # --------------------------------------------------------------------------- # Simplified .ffi. macro integration test @@ -474,7 +483,7 @@ suite "simplified .ffi. macro": let ctxAddr = cast[uint](parseBiggestUInt(addrStr)) check ctxAddr != 0 let ctx = cast[ptr FFIContext[SimpleLib]](ctxAddr) - defer: check destroyFFIContext(ctx).isOk() + defer: check SimpleLibFFIPool.destroyFFIContext(ctx).isOk() # Now call the .ffi. proc var d: CallbackData @@ -524,7 +533,7 @@ suite "async/sync detection in .ffi.": let ctxAddr = cast[uint](parseBiggestUInt(addrStr)) check ctxAddr != 0 let ctx = cast[ptr FFIContext[SimpleLib]](ctxAddr) - defer: check destroyFFIContext(ctx).isOk() + defer: check SimpleLibFFIPool.destroyFFIContext(ctx).isOk() var d2: CallbackData initCallbackData(d2) @@ -592,7 +601,7 @@ suite "ptr return type in .ffi.": let ctxAddr = cast[uint](parseBiggestUInt(ctxAddrStr)) check ctxAddr != 0 let ctx = cast[ptr FFIContext[SimpleLib]](ctxAddr) - defer: check destroyFFIContext(ctx).isOk() + defer: check SimpleLibFFIPool.destroyFFIContext(ctx).isOk() # Alloc a handle var allocD: CallbackData diff --git a/tests/test_gc_compat.nim b/tests/test_gc_compat.nim index f98ef61..c7cdf7a 100644 --- a/tests/test_gc_compat.nim +++ b/tests/test_gc_compat.nim @@ -95,11 +95,12 @@ suite "GC safety - string lifetime across thread boundary": initCallbackData(d) defer: deinitCallbackData(d) - let ctx = createFFIContext[GcTestLib]().valueOr: + var pool: FFIContextPool[GcTestLib] + let ctx = pool.createFFIContext().valueOr: checkpoint "createFFIContext failed: " & $error check false return - defer: discard destroyFFIContext(ctx) + defer: discard pool.destroyFFIContext(ctx) check sendRequestToFFIThread( ctx, StringLifetimeRequest.ffiNewReq(testCallback, addr d, "hello".cstring) @@ -113,10 +114,11 @@ suite "GC safety - string lifetime across thread boundary": initCallbackData(d) defer: deinitCallbackData(d) - let ctx = createFFIContext[GcTestLib]().valueOr: + var pool: FFIContextPool[GcTestLib] + let ctx = pool.createFFIContext().valueOr: check false return - defer: discard destroyFFIContext(ctx) + defer: discard pool.destroyFFIContext(ctx) check sendRequestToFFIThread( ctx, GcErrRequest.ffiNewReq(testCallback, addr d, "test".cstring) @@ -130,10 +132,11 @@ suite "GC safety - string lifetime across thread boundary": initCallbackData(d) defer: deinitCallbackData(d) - let ctx = createFFIContext[GcTestLib]().valueOr: + var pool: FFIContextPool[GcTestLib] + let ctx = pool.createFFIContext().valueOr: check false return - defer: discard destroyFFIContext(ctx) + defer: discard pool.destroyFFIContext(ctx) check sendRequestToFFIThread( ctx, LargeStringRequest.ffiNewReq(testCallback, addr d) @@ -147,10 +150,11 @@ suite "GC safety - string lifetime across thread boundary": suite "GC stability - repeated requests": test "20 sequential requests without GC corruption": - let ctx = createFFIContext[GcTestLib]().valueOr: + var pool: FFIContextPool[GcTestLib] + let ctx = pool.createFFIContext().valueOr: check false return - defer: discard destroyFFIContext(ctx) + defer: discard pool.destroyFFIContext(ctx) for i in 1 .. 20: var d: CallbackData From 7a0c04dea5a85ba9ed04e28f70b187d7d401b917 Mon Sep 17 00:00:00 2001 From: Ivan FB Date: Tue, 12 May 2026 03:07:13 +0200 Subject: [PATCH 06/11] fix nph --- examples/nim_timer/nim_timer.nim | 3 ++- ffi.nim | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/examples/nim_timer/nim_timer.nim b/examples/nim_timer/nim_timer.nim index 947e749..10133e6 100644 --- a/examples/nim_timer/nim_timer.nim +++ b/examples/nim_timer/nim_timer.nim @@ -72,7 +72,8 @@ proc nimtimerComplex*( # In a multi-file library, import all sub-modules first and call genBindings() # once, at the bottom of the top-level compilation-root file. # This call is a no-op unless -d:ffiGenBindings is passed to the compiler. -genBindings() # reads -d:ffiOutputDir, -d:ffiNimSrcRelPath, -d:targetLang from compile flags +genBindings() + # reads -d:ffiOutputDir, -d:ffiNimSrcRelPath, -d:targetLang from compile flags proc nimtimer_destroy*(ctx: pointer) {.dynlib, exportc, cdecl, raises: [].} = ## Tears down the FFI context created by nimtimer_create. diff --git a/ffi.nim b/ffi.nim index a47ac2f..0b98793 100644 --- a/ffi.nim +++ b/ffi.nim @@ -7,5 +7,5 @@ import export atomics, tables export chronos, chronicles export - atomics, alloc, ffi_library, ffi_macro, ffi_types, ffi_context, - ffi_thread_request, serial + atomics, alloc, ffi_library, ffi_macro, ffi_types, ffi_context, ffi_thread_request, + serial From f44a112c885e353e99f60186c78fb1f49e8f02f0 Mon Sep 17 00:00:00 2001 From: Ivan FB Date: Tue, 12 May 2026 09:25:20 +0200 Subject: [PATCH 07/11] make initContextResources private --- ffi/ffi_context.nim | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ffi/ffi_context.nim b/ffi/ffi_context.nim index eb17ae8..22129e6 100644 --- a/ffi/ffi_context.nim +++ b/ffi/ffi_context.nim @@ -317,7 +317,7 @@ proc cleanUpResources[T](ctx: ptr FFIContext[T]): Result[void, string] = ?ctx.threadExitSignal.close() return ok() -proc initContextResources*[T](ctx: ptr FFIContext[T]): Result[void, string] = +proc initContextResources[T](ctx: ptr FFIContext[T]): Result[void, string] = ## Initialises all resources inside an already-allocated FFIContext slot. ## On failure every partially-initialised resource is closed; the caller ## is responsible for releasing the slot (freeShared or pool.releaseSlot). From 7e257c26e57d289e17157e9dbe19df52ed6d9691 Mon Sep 17 00:00:00 2001 From: Ivan FB Date: Tue, 12 May 2026 09:38:53 +0200 Subject: [PATCH 08/11] better variable names and better error descriptions --- ffi/ffi_context.nim | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/ffi/ffi_context.nim b/ffi/ffi_context.nim index 22129e6..510d287 100644 --- a/ffi/ffi_context.nim +++ b/ffi/ffi_context.nim @@ -398,23 +398,23 @@ proc createFFIContext*[T]( ## Acquires a slot from the fixed pool and initialises it as an FFI context. ## Bounded fd usage: at most MaxFFIContexts * 2 ThreadSignalPtr fds are ever open. let ctx = pool.acquireSlot().valueOr: - return err(error) + return err("createFFIContext: acquireSlot failed: " & $error) initContextResources(ctx).isOkOr: pool.releaseSlot(ctx) - return err(error) + return err("createFFIContext: initContextResources failed: " & $error) return ok(ctx) proc signalStop*[T](ctx: ptr FFIContext[T]): Result[void, string] = ctx.running.store(false) - let ffiSignaled = ctx.reqSignal.fireSync().valueOr: + let reqSignaled = ctx.reqSignal.fireSync().valueOr: ctx.onNotResponding() return err("error signaling reqSignal in destroyFFIContext: " & $error) - if not ffiSignaled: + if not reqSignaled: ctx.onNotResponding() return err("failed to signal reqSignal on time in destroyFFIContext") - let wdSignaled = ctx.stopSignal.fireSync().valueOr: + let stopSignaled = ctx.stopSignal.fireSync().valueOr: return err("error signaling stopSignal in destroyFFIContext: " & $error) - if not wdSignaled: + if not stopSignaled: return err("failed to signal stopSignal on time in destroyFFIContext") return ok() @@ -425,12 +425,12 @@ proc signalStop*[T](ctx: ptr FFIContext[T]): Result[void, string] = ## rather than hanging the caller forever. const ThreadExitTimeout = 1500.milliseconds -proc destroyFFIContext*[T](ctx: ptr FFIContext[T]): Result[void, string] = +proc destroyFFIContext[T](ctx: ptr FFIContext[T]): Result[void, string] = ## Stops the FFI context that was created via createFFIContext[T]() (heap). unregisterCtx(cast[pointer](ctx)) ctx.signalStop().isOkOr: - return err(error) + return err("destroyFFIContext: signalStop failed: " & $error) let exitedOnTime = ctx.threadExitSignal.waitSync(ThreadExitTimeout).valueOr: ctx.onNotResponding() @@ -456,7 +456,7 @@ proc destroyFFIContext*[T]( unregisterCtx(cast[pointer](ctx)) ctx.signalStop().isOkOr: - return err(error) + return err("destroyFFIContext(pool): signalStop failed: " & $error) let exitedOnTime = ctx.threadExitSignal.waitSync(ThreadExitTimeout).valueOr: ctx.onNotResponding() From b53f3cee1364d711fbefd38cb52ee324b91865bd Mon Sep 17 00:00:00 2001 From: Ivan FB Date: Tue, 12 May 2026 22:36:36 +0200 Subject: [PATCH 09/11] rename dup destroyFFIContext to clearContext --- ffi/ffi_context.nim | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/ffi/ffi_context.nim b/ffi/ffi_context.nim index 510d287..ebd51d7 100644 --- a/ffi/ffi_context.nim +++ b/ffi/ffi_context.nim @@ -408,29 +408,29 @@ proc signalStop*[T](ctx: ptr FFIContext[T]): Result[void, string] = ctx.running.store(false) let reqSignaled = ctx.reqSignal.fireSync().valueOr: ctx.onNotResponding() - return err("error signaling reqSignal in destroyFFIContext: " & $error) + return err("error signaling reqSignal in signalStop: " & $error) if not reqSignaled: ctx.onNotResponding() - return err("failed to signal reqSignal on time in destroyFFIContext") + return err("failed to signal reqSignal on time in signalStop") let stopSignaled = ctx.stopSignal.fireSync().valueOr: - return err("error signaling stopSignal in destroyFFIContext: " & $error) + return err("error signaling stopSignal in signalStop: " & $error) if not stopSignaled: - return err("failed to signal stopSignal on time in destroyFFIContext") + return err("failed to signal stopSignal on time in signalStop") return ok() ## If the FFI thread's event loop is blocked by a synchronous handler ## (e.g. blocking I/O), it cannot process reqSignal in time to exit. -## destroyFFIContext waits on threadExitSignal up to this bound; on timeout it +## clearContext waits on threadExitSignal up to this bound; on timeout it ## returns err and skips joinThread/cleanup (leaking the thread + ctx slot) ## rather than hanging the caller forever. const ThreadExitTimeout = 1500.milliseconds -proc destroyFFIContext[T](ctx: ptr FFIContext[T]): Result[void, string] = +proc clearContext[T](ctx: ptr FFIContext[T]): Result[void, string] = ## Stops the FFI context that was created via createFFIContext[T]() (heap). unregisterCtx(cast[pointer](ctx)) ctx.signalStop().isOkOr: - return err("destroyFFIContext: signalStop failed: " & $error) + return err("clearContext: signalStop failed: " & $error) let exitedOnTime = ctx.threadExitSignal.waitSync(ThreadExitTimeout).valueOr: ctx.onNotResponding() From 397150e6bc3b7d51c606950121b320369ec3d814 Mon Sep 17 00:00:00 2001 From: Ivan FB Date: Tue, 12 May 2026 22:43:35 +0200 Subject: [PATCH 10/11] mv comment to an appropriate location --- examples/nim_timer/nim_timer.nim | 1 - ffi/internal/ffi_macro.nim | 2 ++ 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/examples/nim_timer/nim_timer.nim b/examples/nim_timer/nim_timer.nim index 10133e6..bd4f568 100644 --- a/examples/nim_timer/nim_timer.nim +++ b/examples/nim_timer/nim_timer.nim @@ -73,7 +73,6 @@ proc nimtimerComplex*( # once, at the bottom of the top-level compilation-root file. # This call is a no-op unless -d:ffiGenBindings is passed to the compiler. genBindings() - # reads -d:ffiOutputDir, -d:ffiNimSrcRelPath, -d:targetLang from compile flags proc nimtimer_destroy*(ctx: pointer) {.dynlib, exportc, cdecl, raises: [].} = ## Tears down the FFI context created by nimtimer_create. diff --git a/ffi/internal/ffi_macro.nim b/ffi/internal/ffi_macro.nim index f310983..9fa7449 100644 --- a/ffi/internal/ffi_macro.nim +++ b/ffi/internal/ffi_macro.nim @@ -1635,6 +1635,8 @@ macro genBindings*( ## -d:ffiNimSrcRelPath, or can be passed as explicit arguments. ## This macro is a no-op unless -d:ffiGenBindings is set. ## + ## This reads -d:ffiOutputDir, -d:ffiNimSrcRelPath, -d:targetLang from compile flags. + ## ## Example (all via compile flags): ## genBindings() ## # nim c -d:ffiGenBindings -d:targetLang=rust \ From dbc58520f4acba19898001b83279627cc1888848 Mon Sep 17 00:00:00 2001 From: Ivan FB Date: Tue, 12 May 2026 23:44:56 +0200 Subject: [PATCH 11/11] better context modules org and avoid use ctx registry --- ffi.nim | 6 +- ffi/ffi_context.nim | 144 +++++++--------------------------- ffi/ffi_context_pool.nim | 63 +++++++++++++++ ffi/internal/ffi_macro.nim | 18 +++-- tests/test_ctx_validation.nim | 79 +++++++++++-------- 5 files changed, 156 insertions(+), 154 deletions(-) create mode 100644 ffi/ffi_context_pool.nim diff --git a/ffi.nim b/ffi.nim index 0b98793..24186bb 100644 --- a/ffi.nim +++ b/ffi.nim @@ -2,10 +2,10 @@ import std/[atomics, tables] import chronos, chronicles import ffi/internal/[ffi_library, ffi_macro], - ffi/[alloc, ffi_types, ffi_context, ffi_thread_request, serial] + ffi/[alloc, ffi_types, ffi_context, ffi_context_pool, ffi_thread_request, serial] export atomics, tables export chronos, chronicles export - atomics, alloc, ffi_library, ffi_macro, ffi_types, ffi_context, ffi_thread_request, - serial + atomics, alloc, ffi_library, ffi_macro, ffi_types, ffi_context, ffi_context_pool, + ffi_thread_request, serial diff --git a/ffi/ffi_context.nim b/ffi/ffi_context.nim index ebd51d7..f0a2cde 100644 --- a/ffi/ffi_context.nim +++ b/ffi/ffi_context.nim @@ -2,7 +2,7 @@ {.pragma: callback, cdecl, raises: [], gcsafe.} {.passc: "-fPIC".} -import std/[options, atomics, os, net, locks, json, tables, sets] +import std/[atomics, locks, json, tables] import chronicles, chronos, chronos/threadsync, taskpools/channels_spsc_single, results import ./ffi_types, ./ffi_thread_request, ./internal/ffi_macro, ./logging @@ -41,30 +41,6 @@ var ffiCurrentCallbackState* {.threadvar.}: ptr FFICallbackState const git_version* {.strdefine.} = "n/a" -var contextRegistry = initHashSet[pointer]() -var contextRegistryLock: Lock -contextRegistryLock.initLock() - -proc registerCtx(ctx: pointer) = - {.cast(gcsafe).}: - contextRegistryLock.acquire() - defer: contextRegistryLock.release() - contextRegistry.incl(ctx) - -proc unregisterCtx(ctx: pointer) = - {.cast(gcsafe).}: - contextRegistryLock.acquire() - defer: contextRegistryLock.release() - contextRegistry.excl(ctx) - -proc isValidCtx*(ctx: pointer): bool = - ## Returns true only if ctx was created by createFFIContext and not yet destroyed. - ## Rejects nil, offset-invalid, and dangling pointers at the API boundary. - {.cast(gcsafe).}: - contextRegistryLock.acquire() - defer: contextRegistryLock.release() - return contextRegistry.contains(ctx) - template callEventCallback*(ctx: ptr FFIContext, eventName: string, body: untyped) = if isNil(ctx[].callbackState.callback): chronicles.error eventName & " - eventCallback is nil" @@ -74,14 +50,20 @@ template callEventCallback*(ctx: ptr FFIContext, eventName: string, body: untype try: let event = body cast[FFICallBack](ctx[].callbackState.callback)( - RET_OK, unsafeAddr event[0], cast[csize_t](len(event)), ctx[].callbackState.userData + RET_OK, + unsafeAddr event[0], + cast[csize_t](len(event)), + ctx[].callbackState.userData, ) except Exception, CatchableError: let msg = "Exception " & eventName & " when calling 'eventCallBack': " & getCurrentExceptionMsg() cast[FFICallBack](ctx[].callbackState.callback)( - RET_ERR, unsafeAddr msg[0], cast[csize_t](len(msg)), ctx[].callbackState.userData + RET_ERR, + unsafeAddr msg[0], + cast[csize_t](len(msg)), + ctx[].callbackState.userData, ) template dispatchFfiEvent*(eventName: string, body: untyped) = @@ -99,8 +81,7 @@ template dispatchFfiEvent*(eventName: string, body: untyped) = RET_OK, unsafeAddr event[0], cast[csize_t](len(event)), ffiState[].userData ) except Exception, CatchableError: - let msg = - "Exception dispatching " & eventName & ": " & getCurrentExceptionMsg() + let msg = "Exception dispatching " & eventName & ": " & getCurrentExceptionMsg() cast[FFICallBack](ffiState[].callback)( RET_ERR, unsafeAddr msg[0], cast[csize_t](len(msg)), ffiState[].userData ) @@ -108,9 +89,6 @@ template dispatchFfiEvent*(eventName: string, body: untyped) = proc sendRequestToFFIThread*( ctx: ptr FFIContext, ffiRequest: ptr FFIThreadRequest, timeout = InfiniteDuration ): Result[void, string] = - if not isValidCtx(cast[pointer](ctx)): - deleteRequest(ffiRequest) - return err("ctx is not a valid FFI context") ctx.lock.acquire() # This lock is only necessary while we use a SP Channel and while the signalling # between threads assumes that there aren't concurrent requests. @@ -229,7 +207,9 @@ proc processRequest[T]( try: await retFut except AsyncError as exc: - Result[string, string].err("Async error in processRequest for " & reqId & ": " & exc.msg) + Result[string, string].err( + "Async error in processRequest for " & reqId & ": " & exc.msg + ) ## handleRes may raise (OOM, GC setup) even though it is rare. Catching here ## keeps the async proc raises:[] compatible. The defer inside handleRes @@ -250,8 +230,7 @@ proc ffiThreadBody[T](ctx: ptr FFIContext[T]) {.thread.} = # wait can unblock and proceed with cleanup. let fireRes = ctx.threadExitSignal.fireSync() if fireRes.isErr(): - error "failed to fire threadExitSignal on FFI thread exit", - err = fireRes.error + error "failed to fire threadExitSignal on FFI thread exit", err = fireRes.error let ffiRun = proc(ctx: ptr FFIContext[T]) {.async.} = var ffiReqHandler: T @@ -317,7 +296,7 @@ proc cleanUpResources[T](ctx: ptr FFIContext[T]): Result[void, string] = ?ctx.threadExitSignal.close() return ok() -proc initContextResources[T](ctx: ptr FFIContext[T]): Result[void, string] = +proc initContextResources*[T](ctx: ptr FFIContext[T]): Result[void, string] = ## Initialises all resources inside an already-allocated FFIContext slot. ## On failure every partially-initialised resource is closed; the caller ## is responsible for releasing the slot (freeShared or pool.releaseSlot). @@ -328,7 +307,7 @@ proc initContextResources[T](ctx: ptr FFIContext[T]): Result[void, string] = if not success: ctx.cleanUpResources().isOkOr: error "failed to clean up resources after createFFIContext failure", - err = error + error = error ctx.reqSignal = ThreadSignalPtr.new().valueOr: return err("couldn't create reqSignal ThreadSignalPtr: " & $error) @@ -363,47 +342,9 @@ proc initContextResources[T](ctx: ptr FFIContext[T]): Result[void, string] = joinThread(ctx.ffiThread) return err("failed to create the watchdog thread: " & getCurrentExceptionMsg()) - registerCtx(cast[pointer](ctx)) success = true return ok() -const MaxFFIContexts* = 32 - ## Maximum number of concurrently live FFI contexts when using FFIContextPool. - ## Fds and threads are only consumed for slots that are actually acquired, - ## so this value only affects the upfront memory of the pool array. - -type FFIContextPool*[T] = object - ## Fixed-size pool of FFI contexts. Avoids dynamic heap allocation per context - ## and bounds the total number of file descriptors consumed by ThreadSignalPtrs - ## to at most MaxFFIContexts * 2. - slots: array[MaxFFIContexts, FFIContext[T]] - inUse: array[MaxFFIContexts, Atomic[bool]] - -proc acquireSlot[T](pool: var FFIContextPool[T]): Result[ptr FFIContext[T], string] = - for i in 0 ..< MaxFFIContexts: - var expected = false - if pool.inUse[i].compareExchange(expected, true): - return ok(pool.slots[i].addr) - return err("FFI context pool exhausted (max " & $MaxFFIContexts & " contexts)") - -proc releaseSlot[T](pool: var FFIContextPool[T], ctx: ptr FFIContext[T]) = - for i in 0 ..< MaxFFIContexts: - if pool.slots[i].addr == ctx: - pool.inUse[i].store(false) - return - -proc createFFIContext*[T]( - pool: var FFIContextPool[T] -): Result[ptr FFIContext[T], string] = - ## Acquires a slot from the fixed pool and initialises it as an FFI context. - ## Bounded fd usage: at most MaxFFIContexts * 2 ThreadSignalPtr fds are ever open. - let ctx = pool.acquireSlot().valueOr: - return err("createFFIContext: acquireSlot failed: " & $error) - initContextResources(ctx).isOkOr: - pool.releaseSlot(ctx) - return err("createFFIContext: initContextResources failed: " & $error) - return ok(ctx) - proc signalStop*[T](ctx: ptr FFIContext[T]): Result[void, string] = ctx.running.store(false) let reqSignaled = ctx.reqSignal.fireSync().valueOr: @@ -423,14 +364,15 @@ proc signalStop*[T](ctx: ptr FFIContext[T]): Result[void, string] = ## clearContext waits on threadExitSignal up to this bound; on timeout it ## returns err and skips joinThread/cleanup (leaking the thread + ctx slot) ## rather than hanging the caller forever. -const ThreadExitTimeout = 1500.milliseconds - -proc clearContext[T](ctx: ptr FFIContext[T]): Result[void, string] = - ## Stops the FFI context that was created via createFFIContext[T]() (heap). - unregisterCtx(cast[pointer](ctx)) +const ThreadExitTimeout* = 1500.milliseconds +proc stopAndJoinThreads*[T](ctx: ptr FFIContext[T]): Result[void, string] = + ## Signals the FFI and watchdog threads to stop, waits up to ThreadExitTimeout + ## for the FFI thread to exit, and joins both. On timeout returns err and + ## skips joinThread (leaving the threads live) rather than hanging the caller. + ## Resource cleanup (signal fds, lock) is the caller's responsibility. ctx.signalStop().isOkOr: - return err("clearContext: signalStop failed: " & $error) + return err("signalStop failed: " & $error) let exitedOnTime = ctx.threadExitSignal.waitSync(ThreadExitTimeout).valueOr: ctx.onNotResponding() @@ -442,40 +384,12 @@ proc clearContext[T](ctx: ptr FFIContext[T]): Result[void, string] = joinThread(ctx.ffiThread) joinThread(ctx.watchdogThread) - ctx.cleanUpResources().isOkOr: - return err("cleanUpResources failed: " & $error) return ok() -proc destroyFFIContext*[T]( - pool: var FFIContextPool[T], ctx: ptr FFIContext[T] -): Result[void, string] = - ## Stops the FFI context and returns its slot to the pool. If the FFI thread - ## is blocked and does not exit in time, the slot is leaked rather than - ## reclaimed — closing its resources while the thread is still live would be - ## unsafe. - unregisterCtx(cast[pointer](ctx)) - - ctx.signalStop().isOkOr: - return err("destroyFFIContext(pool): signalStop failed: " & $error) - - let exitedOnTime = ctx.threadExitSignal.waitSync(ThreadExitTimeout).valueOr: - ctx.onNotResponding() - return err("error waiting for FFI thread exit: " & $error) - - if not exitedOnTime: - ctx.onNotResponding() - return err("FFI thread did not exit in time; leaking pool slot to avoid hang") - - joinThread(ctx.ffiThread) - joinThread(ctx.watchdogThread) - pool.releaseSlot(ctx) +proc clearContext[T](ctx: ptr FFIContext[T]): Result[void, string] = + ## Stops the FFI context that was created via createFFIContext[T]() (heap). + ctx.stopAndJoinThreads().isOkOr: + return err("clearContext: " & $error) + ctx.cleanUpResources().isOkOr: + return err("cleanUpResources failed: " & $error) return ok() - -template checkParams*(ctx: ptr FFIContext, callback: FFICallBack, userData: pointer) = - if not isValidCtx(cast[pointer](ctx)): - return RET_ERR - - ctx[].userData = userData - - if isNil(callback): - return RET_MISSING_CALLBACK diff --git a/ffi/ffi_context_pool.nim b/ffi/ffi_context_pool.nim new file mode 100644 index 0000000..dc1e154 --- /dev/null +++ b/ffi/ffi_context_pool.nim @@ -0,0 +1,63 @@ +import std/atomics +import results +import ./ffi_context + +const MaxFFIContexts* = 32 + ## Maximum number of concurrently live FFI contexts when using FFIContextPool. + ## Fds and threads are only consumed for slots that are actually acquired, + ## so this value only affects the upfront memory of the pool array. + +type FFIContextPool*[T] = object + ## Fixed-size pool of FFI contexts. Avoids dynamic heap allocation per context + ## and bounds the total number of file descriptors consumed by ThreadSignalPtrs + ## to at most MaxFFIContexts * 2. + slots: array[MaxFFIContexts, FFIContext[T]] + inUse: array[MaxFFIContexts, Atomic[bool]] + +proc acquireSlot[T](pool: var FFIContextPool[T]): Result[ptr FFIContext[T], string] = + for i in 0 ..< MaxFFIContexts: + var expected = false + if pool.inUse[i].compareExchange(expected, true): + return ok(pool.slots[i].addr) + return err("FFI context pool exhausted (max " & $MaxFFIContexts & " contexts)") + +proc releaseSlot[T](pool: var FFIContextPool[T], ctx: ptr FFIContext[T]) = + for i in 0 ..< MaxFFIContexts: + if pool.slots[i].addr == ctx: + pool.inUse[i].store(false) + return + +proc createFFIContext*[T]( + pool: var FFIContextPool[T] +): Result[ptr FFIContext[T], string] = + ## Acquires a slot from the fixed pool and initialises it as an FFI context. + ## Bounded fd usage: at most MaxFFIContexts * 2 ThreadSignalPtr fds are ever open. + let ctx = pool.acquireSlot().valueOr: + return err("createFFIContext: acquireSlot failed: " & $error) + initContextResources(ctx).isOkOr: + pool.releaseSlot(ctx) + return err("createFFIContext: initContextResources failed: " & $error) + return ok(ctx) + +proc destroyFFIContext*[T]( + pool: var FFIContextPool[T], ctx: ptr FFIContext[T] +): Result[void, string] = + ## Stops the FFI context and returns its slot to the pool. If the FFI thread + ## is blocked and does not exit in time, the slot is leaked rather than + ## reclaimed — closing its resources while the thread is still live would be + ## unsafe. + ctx.stopAndJoinThreads().isOkOr: + return err("destroyFFIContext(pool): " & $error) + pool.releaseSlot(ctx) + return ok() + +proc isValidCtx*[T](pool: var FFIContextPool[T], ctx: pointer): bool = + ## Returns true only if ctx points to one of the pool's slots that is + ## currently in use. Rejects nil, offset-invalid, and dangling pointers + ## at the API boundary, preventing use-after-free dereferences. + if ctx.isNil(): + return false + for i in 0 ..< MaxFFIContexts: + if cast[pointer](pool.slots[i].addr) == ctx: + return pool.inUse[i].load() + return false diff --git a/ffi/internal/ffi_macro.nim b/ffi/internal/ffi_macro.nim index 9fa7449..45077bd 100644 --- a/ffi/internal/ffi_macro.nim +++ b/ffi/internal/ffi_macro.nim @@ -533,6 +533,12 @@ macro ffiRaw*(prc: untyped): untyped = let paramIdent = firstParam[0] let paramType = firstParam[1] + # The first param of an `.ffiRaw.` proc is `ctx: ptr FFIContext[LibType]`. + # Extract LibType so we can call the module-level pool var (named + # "FFIPool", declared by `.ffiCtor.`) to validate ctx. + let libTypeName = paramType[0][1] + let poolIdent = ident($libTypeName & "FFIPool") + let reqName = ident($procName & "Req") let returnType = ident("cint") @@ -569,7 +575,7 @@ macro ffiRaw*(prc: untyped): untyped = let ffiBody = newStmtList( quote do: initializeLibrary() - if not isValidCtx(cast[pointer](ctx)): + if not `poolIdent`.isValidCtx(cast[pointer](ctx)): return RET_ERR ctx[].userData = userData if isNil(callback): @@ -804,9 +810,10 @@ macro ffi*(prc: untyped): untyped = if callback.isNil: return RET_MISSING_CALLBACK + let asyncPoolIdent = ident($libTypeName & "FFIPool") ffiBody.add quote do: - if ctx.isNil or ctx[].myLib.isNil: - let errStr = "context not initialized" + if not `asyncPoolIdent`.isValidCtx(cast[pointer](ctx)): + let errStr = "ctx is not a valid FFI context" callback(RET_ERR, unsafeAddr errStr[0], cast[csize_t](errStr.len), userData) return RET_ERR @@ -930,9 +937,10 @@ macro ffi*(prc: untyped): untyped = if callback.isNil: return RET_MISSING_CALLBACK + let syncPoolIdent = ident($libTypeName & "FFIPool") syncFfiBody.add quote do: - if ctx.isNil or ctx[].myLib.isNil: - let errStr = "context not initialized" + if not `syncPoolIdent`.isValidCtx(cast[pointer](ctx)): + let errStr = "ctx is not a valid FFI context" callback(RET_ERR, unsafeAddr errStr[0], cast[csize_t](errStr.len), userData) return RET_ERR diff --git a/tests/test_ctx_validation.nim b/tests/test_ctx_validation.nim index 07b2547..a6c903d 100644 --- a/tests/test_ctx_validation.nim +++ b/tests/test_ctx_validation.nim @@ -1,41 +1,58 @@ -import std/locks +import std/[atomics, locks] import unittest2 import results import ../ffi type TestLib = object -proc dummyCallback( +ffiType: + type CtxValidationConfig = object + initialValue: int + +proc ctxval_create*( + config: CtxValidationConfig +): Future[Result[TestLib, string]] {.ffiCtor.} = + return ok(TestLib()) + +proc ctxval_ping*(lib: TestLib): Future[Result[string, string]] {.ffi.} = + return ok("pong") + +type CallbackState = object + lock: Lock + called: Atomic[bool] + retCode: cint + +proc initCbState(s: var CallbackState) = + s.lock.initLock() + s.called.store(false) + +proc validationCallback( retCode: cint, msg: ptr cchar, len: csize_t, userData: pointer ) {.cdecl, gcsafe, raises: [].} = - discard - -registerReqFFI(ValidationTestRequest, lib: ptr TestLib): - proc(): Future[Result[string, string]] {.async.} = - return ok("ok") - -suite "ctx pointer validation": - # BUG: sendRequestToFFIThread has no nil-check on ctx. - # checkParams / {.ffi.} generated code only guards against nil callback, - # not nil (or otherwise invalid) ctx. Any caller — C or Nim — that passes - # a nil or offset-invalid ctx with a valid callback bypasses the only guard - # and reaches ctx.lock.acquire() where the nil/garbage dereference crashes. - - test "nil ctx with valid callback should return an error, not crash": - # Reproduces the nil case: ctx=nil, callback=valid. - # Expected (after fix): sendRequestToFFIThread returns isErr(). - # Actual (currently) : SIGSEGV at ctx.lock.acquire() in sendRequestToFFIThread. + let s = cast[ptr CallbackState](userData) + s[].retCode = retCode + s[].called.store(true) + +suite "ctx pointer validation at the FFI entry point": + # The macro-generated FFI entry point validates ctx via + # FFIPool.isValidCtx. Any caller — C or Nim — that passes a nil or + # offset-invalid ctx with a valid callback should receive RET_ERR via the + # callback and the proc should return RET_ERR, never crash. + + test "nil ctx with valid callback returns RET_ERR via callback, no crash": + var s: CallbackState + initCbState(s) let nilCtx: ptr FFIContext[TestLib] = nil - let req = ValidationTestRequest.ffiNewReq(dummyCallback, nil) - let res = sendRequestToFFIThread(nilCtx, req) - check res.isErr() - - test "invalid non-nil ctx (ctx+123 style) should return an error, not crash": - # Reproduces the offset-pointer case: a non-nil but invalid pointer passes - # isNil() and reaches the lock dereference, causing a crash. - # Expected (after fix): sendRequestToFFIThread returns isErr(). - # Actual (currently) : SIGSEGV when the garbage pointer is dereferenced. + let ret = ctxval_ping(nilCtx, validationCallback, addr s) + check ret == RET_ERR + check s.called.load() + check s.retCode == RET_ERR + + test "invalid non-nil ctx (offset-pointer) returns RET_ERR, no crash": + var s: CallbackState + initCbState(s) let invalidCtx = cast[ptr FFIContext[TestLib]](123) - let req = ValidationTestRequest.ffiNewReq(dummyCallback, nil) - let res = sendRequestToFFIThread(invalidCtx, req) - check res.isErr() + let ret = ctxval_ping(invalidCtx, validationCallback, addr s) + check ret == RET_ERR + check s.called.load() + check s.retCode == RET_ERR