feat(ffi): configurable per-request handler timeout with a finite default (#93)#108
Merged
Conversation
Ivansete-status
approved these changes
Jul 5, 2026
Ivansete-status
left a comment
Collaborator
There was a problem hiding this comment.
LGTM! Thanks for it! 🙌
Just added some nitpicks that I hope you find useful.
| requireLibraryDeclared("`.ffiRaw.`") | ||
| let prc = args[^1] | ||
| gateABIFormat(resolveABIFormat(args[0 ..^ 2]), "`.ffiRaw.` proc") | ||
| let (rawAbiFormat, rawTimeoutMs) = resolveFFISpecs(args[0 ..^ 2]) |
Collaborator
There was a problem hiding this comment.
Maybe needed to trim?
gmelodie
force-pushed
the
feat/config-request-timeout
branch
from
July 6, 2026 13:53
57936ad to
1c66cd0
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #93.
sendRequestToFFIThreadused to hand a foreign caller an unbounded wait, so a wedged or slow handler blocked that caller forever — the liveness watchdog detected the wedge but the in-flight request still hung. This wires a finite, configurable handler-completion timeout so the caller unblocks deterministically.What changed
Each
FFIContextnow carriesdefaultRequestTimeout(5s,ffi/ffi_context.nim). InprocessRequest(ffi/ffi_thread.nim) the handler is raced against its deadline viareportTimeoutIfTripped; on trip the caller is answered withffi request timed out after <n>msand a WARN is logged.The handler is deliberately not cancelled on timeout: a hard-cancel mid-call into the underlying library (Waku/libp2p) can leave it partially applied. We report the timeout and let the handler run to completion.
fireCallback(ffi/ffi_thread_request.nim) carries a once-onlyrespondedguard so the two response paths (timeout vs. completion) fire the foreign callback exactly once, and freeing stays withhandleResso the request buffer has a single owner.The deadline is overridable per proc with a
{.ffi: "timeout = <ms>".}spec, parsed like the existingabi = ...spec (meta.nimparseTimeoutSpec/overrideKey, wired throughffi/ffiRaw/ffiCtorinffi_macro.nim). It is runtime-only — recorded inrequestTimeoutsMsat module init and read via the context; codegen ignores it.Notes / scope
The issue's 4a (submit/accept timeout) is obsolete on the current MPSC ingress: submission is a non-blocking enqueue with no accept-ack, so there is nothing to time out on submit. This PR implements 4b, the high-value half.
The timeout race sits inside the same
tryas the handler await, preserving the existing "handleResmust still run" cancellation-safety invariant. Already-completed handlers (e.g. sync bodies) skip arming a timer to keep the per-request cost off the hot path; a shared per-context timer to eliminate it entirely for async handlers is a possible follow-up. Hard-cancel-on-timeout is left as an opt-in follow-up per the issue's cancellation-safety note — a permanently-wedged handler still unblocks its caller but does not reclaim its own request buffer until it finishes.Tests
tests/unit/test_ffi_context.nim: a handler past its per-proc deadline yields a timeout err and the callback fires exactly once (completion does not re-fire); a handler finishing under its deadline returns normally.tests/unit/test_abi_format.nim:parseTimeoutSpec/overrideKeyparsing (valid, malformed, non-positive), and that atimeoutoverride is recorded inrequestTimeoutsMswhile a plain proc has no entry.Verified with the host nim-2.2.6 toolchain (
nim check+ compile-run of the unit suites); all unit tests check clean on orc and refc, andnphis idempotent on the touched files.