-
Notifications
You must be signed in to change notification settings - Fork 2
use fixed array of ctx to avoid consuming all fds #14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 8 commits
4da0ec3
a90453f
3224679
9469f71
66d48fd
7a0c04d
f44a112
7e257c2
b53f3ce
397150e
dbc5852
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -237,7 +237,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 | ||||||||||
|
|
@@ -281,6 +281,7 @@ proc ffiThreadBody[T](ctx: ptr FFIContext[T]) {.thread.} = | |||||||||
| waitFor ffiRun(ctx) | ||||||||||
|
|
||||||||||
| 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() | ||||||||||
|
|
@@ -316,10 +317,10 @@ proc cleanUpResources[T](ctx: ptr FFIContext[T]): Result[void, string] = | |||||||||
| ?ctx.threadExitSignal.close() | ||||||||||
| return ok() | ||||||||||
|
|
||||||||||
| 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 | ||||||||||
|
|
@@ -358,54 +359,116 @@ proc createFFIContext*[T](): Result[ptr FFIContext[T], 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) | ||||||||||
| return err("failed to create the watchdog thread: " & getCurrentExceptionMsg()) | ||||||||||
|
|
||||||||||
| registerCtx(cast[pointer](ctx)) | ||||||||||
| success = true | ||||||||||
| return ok(ctx) | ||||||||||
| return ok() | ||||||||||
|
|
||||||||||
| 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)) | ||||||||||
| 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 | ||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks but proc compareExchange[T: not Trivial](location: var Atomic[T]; expected: var T;
desired: T; order: MemoryOrder = moSequentiallyConsistent): bool |
||||||||||
| 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 | ||||||||||
|
|
||||||||||
| ctx.running.store(false) | ||||||||||
| 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) | ||||||||||
|
|
||||||||||
| let signaledOnTime = ctx.reqSignal.fireSync().valueOr: | ||||||||||
| 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 in destroyFFIContext: " & $error) | ||||||||||
| if not signaledOnTime: | ||||||||||
| return err("error signaling reqSignal in destroyFFIContext: " & $error) | ||||||||||
| if not reqSignaled: | ||||||||||
| ctx.onNotResponding() | ||||||||||
| return err("failed to signal reqSignal on time in destroyFFIContext") | ||||||||||
| let stopSignaled = ctx.stopSignal.fireSync().valueOr: | ||||||||||
| return err("error signaling stopSignal in destroyFFIContext: " & $error) | ||||||||||
| if not stopSignaled: | ||||||||||
| 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 | ||||||||||
| ## (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("destroyFFIContext: signalStop failed: " & $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: | ||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This line seems to be the only difference between the two proc newFn(ctx...) =
unregisterCtx(cast[pointer](ctx))
...
joinThread(ctx.ffiThread)
joinThread(ctx.watchdogThread)
# then ...
proc destroyFFIContext*[T](
pool: var FFIContextPool[T], ctx: ptr FFIContext[T]
): Result[void, string] =
newFn(ctx)
pool.releaseSlot(ctx)
return ok()
# and ...
proc destroyFFIContext[T](ctx: ptr FFIContext[T]): Result[void, string] =
newFn(ctx)
ctx.cleanUpResources().isOkOr:
return err("cleanUpResources failed: " & $error)
return ok()
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it was confusing because both had the same name but one call the other. |
||||||||||
| 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("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) | ||||||||||
| return ok() | ||||||||||
|
|
||||||||||
| template checkParams*(ctx: ptr FFIContext, callback: FFICallBack, userData: pointer) = | ||||||||||
|
|
||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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: | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TIL about |
||
| 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 | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: maybe this comment should be inside the definition of
genBindings? Here it may cause confusion as to what it refers toThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in 397150e