From ad816c475c305bf7f81fd945d1400e9cfe33d891 Mon Sep 17 00:00:00 2001 From: Gabriel Cruz Date: Mon, 6 Jul 2026 11:06:38 -0300 Subject: [PATCH] fix: pr comments --- ffi/codegen/meta.nim | 33 ++++++++++++++++++--------------- ffi/ffi_context.nim | 10 +++------- ffi/ffi_thread.nim | 8 +++++--- ffi/ffi_thread_request.nim | 26 +++++++++++--------------- ffi/internal/ffi_macro.nim | 26 ++++++++++++++------------ tests/unit/test_abi_format.nim | 8 ++++---- 6 files changed, 55 insertions(+), 56 deletions(-) diff --git a/ffi/codegen/meta.nim b/ffi/codegen/meta.nim index 6379b2f..ad97d1c 100644 --- a/ffi/codegen/meta.nim +++ b/ffi/codegen/meta.nim @@ -67,19 +67,22 @@ proc abiCodegenImplemented*(fmt: ABIFormat): bool = ## seam a future PR flips once the `c` dispatch path is wired. fmt == ABIFormat.Cbor -proc specKey*(spec: string): string = - ## Lowercased key of a `key = value` annotation spec (the text before `=`), - ## used to route a spec to its parser. `"timeout = 30000"` → `"timeout"`. - spec.split('=')[0].strip().toLowerAscii() +proc overrideKey*(override: string): string = + ## Lowercased key of a `key = value` pragma override (the text before `=`), + ## used to route it to its parser. `"timeout = 30000"` → `"timeout"`. + override.split('=')[0].strip().toLowerAscii() -proc parseTimeoutSpec*(spec: string): tuple[ok: bool, ms: int, err: string] = +proc parseTimeoutSpec*(override: string): tuple[ok: bool, ms: int, err: string] = ## Parse a `"timeout = "` override (whitespace/case tolerant). ## The value must be a positive integer number of milliseconds. On bad ## grammar or value, returns `ok = false` with a human-readable `err`. - let parts = spec.split('=') - if parts.len != 2 or specKey(spec) != "timeout": - return - (false, 0, "invalid timeout override '" & spec & "'; expected `timeout = `") + let parts = override.split('=') + if parts.len != 2 or overrideKey(override) != "timeout": + return ( + false, + 0, + "invalid timeout override: '" & override & "'; expected `timeout = `", + ) let raw = parts[1].strip() let ms = try: @@ -88,7 +91,7 @@ proc parseTimeoutSpec*(spec: string): tuple[ok: bool, ms: int, err: string] = return ( false, 0, - "invalid timeout value '" & raw & + "invalid timeout value: '" & raw & "'; expected a positive integer of milliseconds", ) if ms <= 0: @@ -106,28 +109,28 @@ proc parseABIFormatName*(name: string): tuple[ok: bool, fmt: ABIFormat] = else: (false, ABIFormat.Cbor) -proc parseAbiSpec*(spec: string): tuple[ok: bool, fmt: ABIFormat, err: string] = +proc parseAbiSpec*(override: string): tuple[ok: bool, fmt: ABIFormat, err: string] = ## Parse an `"abi = "` override (whitespace/case tolerant). On bad ## grammar or format, returns `ok = false` with a human-readable `err`. - let parts = spec.split('=') + let parts = override.split('=') if parts.len != 2: return ( false, ABIFormat.Cbor, - "invalid ABI override '" & spec & "'; expected `abi = c` or `abi = cbor`", + "invalid ABI override: '" & override & "'; expected `abi = c` or `abi = cbor`", ) if parts[0].strip().toLowerAscii() != "abi": return ( false, ABIFormat.Cbor, - "invalid ABI override '" & spec & "'; expected `abi = c` or `abi = cbor`", + "invalid ABI override: '" & override & "'; expected `abi = c` or `abi = cbor`", ) let (ok, fmt) = parseABIFormatName(parts[1]) if not ok: return ( false, ABIFormat.Cbor, - "unknown ABI format '" & parts[1].strip() & "'; valid values are `c` and `cbor`", + "unknown ABI format: '" & parts[1].strip() & "'; valid values are `c` and `cbor`", ) (true, fmt, "") diff --git a/ffi/ffi_context.nim b/ffi/ffi_context.nim index 8f80631..5f3407d 100644 --- a/ffi/ffi_context.nim +++ b/ffi/ffi_context.nim @@ -43,11 +43,8 @@ type FFIContext*[T] = object # Per-proc timeout overrides (ms). Points at the compile-time-filled global, # like registeredRequests, so the FFI thread reads it GC-safely via ctx. defaultRequestTimeout*: Duration - # Deadline applied to each handler unless a `{.ffi: "timeout = ".}` - # override raises it. On trip the caller is unblocked with a timeout err and - # the handler is left running (see processRequest). Set `InfiniteDuration` - # to opt out. Written on the owning thread before the first request; read on - # the FFI thread. + # Per-handler deadline unless a `{.ffi: "timeout = ".}` override raises + # it; `InfiniteDuration` opts out. See processRequest for the trip behavior. var onFFIThread* {.threadvar.}: bool # Re-entrant dispatch guard for `sendRequestToFFIThread`. @@ -59,8 +56,7 @@ const FFIHeartbeatStartDelay* = 10.seconds # grace window for library startup FFIHeartbeatStaleThreshold* = 1.seconds DefaultRequestTimeout* = 5.seconds - # A guess (issue #93): finite so a wedged handler can't hang a caller - # forever, generous enough to clear normal handlers. Overridable per proc. + # Finite fallback (issue #93) so a wedged handler can't hang a caller forever. include ./event_thread include ./ffi_thread diff --git a/ffi/ffi_thread.nim b/ffi/ffi_thread.nim index 9676918..9f6eac2 100644 --- a/ffi/ffi_thread.nim +++ b/ffi/ffi_thread.nim @@ -60,7 +60,8 @@ proc reportTimeoutIfTripped( ## deliberately do NOT cancel the handler: a hard-cancel mid-call into the ## underlying library (Waku/libp2p) can leave it partially applied, so we ## unblock the caller with a timeout err now and let the handler run to - ## completion. `respondOnce` keeps the two paths from answering twice. + ## completion. `fireCallback`'s once-only guard keeps the two paths from + ## answering twice. if deadline == InfiniteDuration: return # Handlers that already completed (e.g. a sync body) skip the timer entirely, @@ -77,10 +78,11 @@ proc reportTimeoutIfTripped( return warn "ffi request timed out; caller unblocked, handler left running", reqId = reqId, timeoutMs = deadline.milliseconds - request.respondOnce( + fireCallback( Result[seq[byte], string].err( "ffi request timed out after " & $deadline.milliseconds & "ms" - ) + ), + request, ) proc processRequest[T]( diff --git a/ffi/ffi_thread_request.nim b/ffi/ffi_thread_request.nim index cf3e6bf..f1a715e 100644 --- a/ffi/ffi_thread_request.nim +++ b/ffi/ffi_thread_request.nim @@ -129,9 +129,16 @@ proc deleteRequest*(request: ptr FFIThreadRequest) = c_free(cast[pointer](request[].reqId)) c_free(request) -proc fireCallback(res: Result[seq[byte], string], request: ptr FFIThreadRequest) = - ## Delivers one response to the foreign callback. Success payload is CBOR - ## bytes; error payload is the raw UTF-8 error string. +proc fireCallback*(res: Result[seq[byte], string], request: ptr FFIThreadRequest) = + ## Delivers the response to the foreign callback, at most once per request: + ## the timeout path and the handler-completion path both call it, but the + ## foreign side must be answered exactly once. Both run on the FFI thread, so + ## the plain `responded` flag needs no synchronization. Success payload is the + ## encoded response bytes; error payload is the raw UTF-8 error string. Does + ## NOT free the request; that stays with `handleRes`. + if request[].responded: + return + request[].responded = true if res.isErr(): foreignThreadGc: let msg = if res.error.len > 0: res.error else: EmptyErrorMarker @@ -156,23 +163,12 @@ proc fireCallback(res: Result[seq[byte], string], request: ptr FFIThreadRequest) RET_OK, cast[ptr cchar](addr sentinel), 1.csize_t, request[].userData ) -proc respondOnce*(request: ptr FFIThreadRequest, res: Result[seq[byte], string]) = - ## Fires the callback the first time it's called for `request` and no-ops - ## after — the timeout path and the handler-completion path both call it, but - ## the foreign side must be answered exactly once. Does NOT free the request: - ## freeing stays with `handleRes` so the handler always owns the buffer until - ## it finishes. - if request[].responded: - return - request[].responded = true - fireCallback(res, request) - proc handleRes*(res: Result[seq[byte], string], request: ptr FFIThreadRequest) = ## Terminal step of every request: delivers the response (unless a timeout ## already did) and frees the request exactly once. defer: deleteRequest(request) - respondOnce(request, res) + fireCallback(res, request) proc nilProcess*(reqId: cstring): Future[Result[seq[byte], string]] {.async.} = return err("This request type is not implemented: " & $reqId) diff --git a/ffi/internal/ffi_macro.nim b/ffi/internal/ffi_macro.nim index 66e341f..d263c64 100644 --- a/ffi/internal/ffi_macro.nim +++ b/ffi/internal/ffi_macro.nim @@ -22,12 +22,13 @@ proc resolveABIFormat(abiSpecs: seq[NimNode]): ABIFormat {.compileTime.} = ## Resolve one annotation's ABI from its optional `"abi = ..."` string specs ## (last wins), inheriting the library default when absent. var fmt = currentDefaultABIFormat - for spec in abiSpecs: - if spec.kind notin {nnkStrLit, nnkRStrLit, nnkTripleStrLit}: + for override in abiSpecs: + if override.kind notin {nnkStrLit, nnkRStrLit, nnkTripleStrLit}: error( - "FFI ABI override must be a string literal like \"abi = c\", got: " & spec.repr + "FFI ABI override must be a string literal like \"abi = c\", got: " & + override.repr ) - let parsed = parseAbiSpec($spec) + let parsed = parseAbiSpec($override) if not parsed.ok: error(parsed.err) fmt = parsed.fmt @@ -41,26 +42,27 @@ proc resolveFFISpecs( ## `timeoutMs == 0` means "no per-proc override" (use the context default). var abi = currentDefaultABIFormat var timeoutMs = 0 - for spec in specs: - if spec.kind notin {nnkStrLit, nnkRStrLit, nnkTripleStrLit}: + for override in specs: + if override.kind notin {nnkStrLit, nnkRStrLit, nnkTripleStrLit}: error( "FFI override must be a string literal like \"abi = c\" or " & - "\"timeout = 30000\", got: " & spec.repr + "\"timeout = 30000\", got: " & override.repr ) - case specKey($spec) + case overrideKey($override) of "abi": - let parsed = parseAbiSpec($spec) + let parsed = parseAbiSpec($override) if not parsed.ok: error(parsed.err) abi = parsed.fmt of "timeout": - let parsed = parseTimeoutSpec($spec) + let parsed = parseTimeoutSpec($override) if not parsed.ok: error(parsed.err) timeoutMs = parsed.ms else: error( - "unknown FFI override '" & $spec & "'; expected `abi = ...` or `timeout = ...`" + "unknown FFI override '" & $override & + "'; expected `abi = ...` or `timeout = ...`" ) (abi, timeoutMs) @@ -802,7 +804,7 @@ macro ffi*(args: varargs[untyped]): untyped = # a TypeDef; `cbor` rides the generic overloads. Both abis are valid here. if prc.kind == nnkTypeDef: if timeoutMs > 0: - error("`.ffi.` on a type takes no `timeout` override (it applies to procs)") + error("`.ffi.` on a type takes no `timeout` override (it only applies to procs)") gateFFITypeABIFormat(abiFormat, "`.ffi.` type") var cleanTypeDef = prc.copyNimTree() if cleanTypeDef[0].kind == nnkPragmaExpr: diff --git a/tests/unit/test_abi_format.nim b/tests/unit/test_abi_format.nim index d41df1f..f6cf8f2 100644 --- a/tests/unit/test_abi_format.nim +++ b/tests/unit/test_abi_format.nim @@ -99,10 +99,10 @@ suite "ABI format parsing": check "bson" in parseAbiSpec("abi = bson").err suite "handler-timeout spec parsing (issue #93)": - test "specKey extracts the lowercased, trimmed key": - check specKey("timeout = 30000") == "timeout" - check specKey(" ABI = c ") == "abi" - check specKey("bare") == "bare" + test "overrideKey extracts the lowercased, trimmed key": + check overrideKey("timeout = 30000") == "timeout" + check overrideKey(" ABI = c ") == "abi" + check overrideKey("bare") == "bare" test "parseTimeoutSpec accepts `timeout = `, flexible spacing": check parseTimeoutSpec("timeout = 30000") == (true, 30000, "")