From b40b245cc73e31b771d8b1fa6752650b49a23707 Mon Sep 17 00:00:00 2001 From: Ivan FB Date: Tue, 14 Jul 2026 16:11:00 +0200 Subject: [PATCH] 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 --- ffi/ffi_context.nim | 12 +++++------- ffi/ffi_thread.nim | 20 ++++++++------------ ffi/ffi_thread_request.nim | 14 ++++++-------- ffi/ffi_types.nim | 20 +++++++++----------- tests/unit/test_ffi_context.nim | 6 ++---- 5 files changed, 30 insertions(+), 42 deletions(-) diff --git a/ffi/ffi_context.nim b/ffi/ffi_context.nim index 6aecdf3..7833188 100644 --- a/ffi/ffi_context.nim +++ b/ffi/ffi_context.nim @@ -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=`. + ## `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=`. const StaleWarnInterval* = StaleWarnIntervalMs.milliseconds type FFITeardownProc*[T] = proc(lib: ptr T): Future[void] {.async.} diff --git a/ffi/ffi_thread.nim b/ffi/ffi_thread.nim index bc0ca25..61076ef 100644 --- a/ffi/ffi_thread.nim +++ b/ffi/ffi_thread.nim @@ -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( diff --git a/ffi/ffi_thread_request.nim b/ffi/ffi_thread_request.nim index 640fa53..da723bb 100644 --- a/ffi/ffi_thread_request.nim +++ b/ffi/ffi_thread_request.nim @@ -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) diff --git a/ffi/ffi_types.nim b/ffi/ffi_types.nim index 074787f..b615fa7 100644 --- a/ffi/ffi_types.nim +++ b/ffi/ffi_types.nim @@ -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 ################################################################################ diff --git a/tests/unit/test_ffi_context.nim b/tests/unit/test_ffi_context.nim index 09304e9..b1080a1 100644 --- a/tests/unit/test_ffi_context.nim +++ b/tests/unit/test_ffi_context.nim @@ -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)