docs: tighten the RET_STALE_WARN comments

They restated what the code says and repeated the never-time-out
rationale at each site; keep the non-obvious why (no cancel because a
partially-applied library call is worse, race not cancelling the loser,
responded left unset on purpose) and drop the rest.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
Ivan FB 2026-07-14 16:11:00 +02:00
parent ccaf04c1ca
commit b40b245cc7
No known key found for this signature in database
GPG Key ID: DF0C67A04C543270
5 changed files with 30 additions and 42 deletions

View File

@ -43,9 +43,8 @@ type FFIContext*[T] = object
running: Atomic[bool] # To control when the threads are running
registeredRequests: ptr Table[cstring, FFIRequestProc]
staleWarnInterval*: Duration
# Cadence of the non-terminal RET_STALE_WARN progress callback; defaults to
# `StaleWarnInterval`. An internal runtime seam (tests tune it) — it is NOT
# exposed to the ffi dev, there is no per-proc override.
# RET_STALE_WARN cadence. An internal seam (tests tune it) — deliberately
# not exposed to the ffi dev, and there is no per-proc override.
var onFFIThread* {.threadvar.}: bool
# Re-entrant dispatch guard for `sendRequestToFFIThread`.
@ -58,10 +57,9 @@ const
FFIHeartbeatStaleThreshold* = 1.seconds
const StaleWarnIntervalMs* {.intdefine: "ffiStaleWarnIntervalMs".} = 5000
## Cadence at which an in-flight request re-notifies its caller with a
## non-terminal `RET_STALE_WARN`. Fires without limit — nim-ffi never times a
## handler out; the caller decides what to do. 5s mirrors Android's ANR input
## timeout. Override with `-d:ffiStaleWarnIntervalMs=<ms>`.
## `RET_STALE_WARN` cadence, fired without limit — nim-ffi never times a
## handler out. 5s mirrors Android's ANR input timeout. Override with
## `-d:ffiStaleWarnIntervalMs=<ms>`.
const StaleWarnInterval* = StaleWarnIntervalMs.milliseconds
type FFITeardownProc*[T] = proc(lib: ptr T): Future[void] {.async.}

View File

@ -50,13 +50,11 @@ proc awaitWithStaleWarnings(
interval: Duration,
reqId: string,
): Future[Result[seq[byte], string]] {.async.} =
## Awaits the handler, delivering a non-terminal RET_STALE_WARN to the caller
## every `interval` for as long as it keeps running, and returns the handler's
## real result. nim-ffi never times a handler out — a hard-cancel mid-call into
## the underlying library (Waku/libp2p) can leave it partially applied — so the
## caller is kept informed and decides for itself. The timer lives entirely in
## this frame, so nothing references the request once the handler resolves and
## the terminal callback frees it.
## Pings the caller with RET_STALE_WARN every `interval` while the handler
## runs, then returns its real result. Never cancels it: a hard-cancel mid-call
## into the underlying library (Waku/libp2p) can leave it partially applied, so
## the caller is kept informed and decides for itself. The timer lives entirely
## in this frame, so nothing references the request once the handler resolves.
let intervalMs = interval.milliseconds
if intervalMs <= 0:
# A non-positive / infinite interval opts out of progress pings entirely.
@ -92,13 +90,11 @@ proc processRequest[T](
else:
ctx[].registeredRequests[][reqIdCs](cast[pointer](request), ctx)
# CatchableError covers CancelledError from the shutdown drain; handleRes must
# still run, so the stale-warn loop and the handler await share one try — a
# cancel mid-loop must not skip the response-and-free below.
# CatchableError covers CancelledError from the shutdown drain. The warn loop
# and the handler share one try so that a cancel mid-loop still reaches the
# response-and-free below.
let res =
try:
# Emits RET_STALE_WARN every ctx.staleWarnInterval while the handler runs,
# then returns its real result.
await awaitWithStaleWarnings(retFut, request, ctx.staleWarnInterval, reqId)
except CatchableError as e:
Result[seq[byte], string].err(

View File

@ -252,12 +252,10 @@ proc fireCallback*(res: Result[seq[byte], string], request: ptr FFIThreadRequest
)
proc fireStaleWarn*(request: ptr FFIThreadRequest, elapsedMs: int64) =
## Non-terminal progress signal telling the caller its request is still in
## flight after `elapsedMs`. Unlike `fireCallback` it deliberately does NOT set
## `responded` and may fire many times — the one terminal RET_OK/RET_ERR is
## still owed. Skipped once a terminal response has gone out. Runs on the FFI
## thread, so the plain `responded` read needs no synchronization. The payload
## is the elapsed milliseconds as a decimal UTF-8 string.
## Tells the caller its request is still in flight after `elapsedMs` (sent as
## decimal UTF-8). Unlike `fireCallback` it deliberately leaves `responded`
## unset and may fire many times — the terminal RET_OK/RET_ERR is still owed.
## Runs on the FFI thread, so reading `responded` needs no synchronization.
if request[].responded:
return
foreignThreadGc:
@ -271,8 +269,8 @@ proc fireStaleWarn*(request: ptr FFIThreadRequest, elapsedMs: int64) =
proc handleRes*(res: Result[seq[byte], string], request: ptr FFIThreadRequest) =
## Terminal step of every request: delivers the response and frees the request
## exactly once. Any RET_STALE_WARN progress signals have already gone out; the
## `responded` guard in `fireStaleWarn` keeps this terminal answer the last one.
## exactly once. The `responded` guard in `fireStaleWarn` keeps this answer
## last, after any progress pings.
defer:
deleteRequest(request)
fireCallback(res, request)

View File

@ -7,22 +7,20 @@ import chronos
type FFICallBack* = proc(
callerRet: cint, msg: ptr cchar, len: csize_t, userData: pointer
) {.cdecl, gcsafe, raises: [].}
## Result-delivery callback. `callerRet` is one of the `RET_*` codes below.
## `RET_OK`/`RET_ERR` are *terminal*: they fire exactly once and end the
## request. `RET_STALE_WARN` is *non-terminal*: it may fire repeatedly while a
## handler is still running (see below) and is always followed by a terminal
## code. Consumers that only care about the final answer should ignore it.
## Result-delivery callback. `callerRet` is one of the `RET_*` codes below:
## `RET_OK`/`RET_ERR` fire exactly once and end the request, `RET_STALE_WARN`
## may fire repeatedly before them and should be ignored unless progress
## matters.
const RET_OK*: cint = 0
const RET_ERR*: cint = 1
const RET_MISSING_CALLBACK*: cint = 2
const RET_STALE_WARN*: cint = 3
## Non-terminal progress signal: the request is still in flight. Delivered
## every `StaleWarnInterval` (default 5s) for as long as the handler runs, with
## `msg` carrying the elapsed milliseconds as a decimal ASCII string. nim-ffi
## never times a handler out — it always ends with a terminal `RET_OK`/
## `RET_ERR`; `RET_STALE_WARN` just lets the caller decide whether to keep
## waiting.
## Non-terminal: the request is still in flight. Fires every
## `StaleWarnInterval` (default 5s) while the handler runs, `msg` carrying the
## elapsed milliseconds as decimal ASCII, and is always followed by a terminal
## code — nim-ffi never times a handler out, so the caller decides whether to
## keep waiting.
### End of exported types
################################################################################

View File

@ -27,10 +27,8 @@ proc deinitCallbackData(d: var CallbackData) =
proc testCallback(
retCode: cint, msg: ptr cchar, len: csize_t, userData: pointer
) {.cdecl, gcsafe, raises: [].} =
# RET_STALE_WARN is a progress ping, not an answer: a slow handler (or a slow
# CI runner) trips it before the terminal code arrives. Waking `waitCallback`
# here would hand the test a non-terminal retCode and let it tear the context
# down mid-flight. Tests that assert on the pings use `staleCallback`.
# A progress ping is not an answer: waking waitCallback here would report a
# non-terminal code as the result. Tests asserting on pings use staleCallback.
if retCode == RET_STALE_WARN:
return
let d = cast[ptr CallbackData](userData)