diff --git a/CHANGELOG.md b/CHANGELOG.md index c18d679..2afe662 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -42,16 +42,17 @@ All notable changes to this project are documented in this file. `nimble genbindings_c` / `genbindings_c_echo` / `check_bindings_c` / `test_c_e2e` tasks, a `tests/e2e/c` ctest harness, and a `tests/unit/test_c_codegen.nim` unit suite. -- Configurable per-request handler timeout with a finite default: each - `FFIContext` now carries a `defaultRequestTimeout` (5s) applied to every - handler, replacing the previous unbounded wait so a wedged handler can no - longer hang a foreign caller forever. On trip the caller is unblocked with an - `ffi request timed out after ms` err; the handler is left running (not - cancelled, since a hard-cancel mid-call into the underlying library can leave - it partial), and the callback still fires exactly once. Override per proc with - a `"timeout = "` spec (e.g. `{.ffi: "timeout = 30000".}`), parsed like the - `abi = ...` spec; runtime-only, codegen ignores it - ([#93](https://github.com/logos-messaging/nim-ffi/issues/93)). +- Non-terminal `RET_STALE_WARN` (3) progress callback in place of a handler + timeout: nim-ffi never times a handler out (a hard-cancel mid-call into the + underlying library can leave it half-applied). Instead, while a request is + still in flight its result callback receives a `RET_STALE_WARN` every 5s + (Android's ANR interval; override with `-d:ffiStaleWarnIntervalMs=`), with + the payload carrying the elapsed milliseconds as a decimal string. The request + always ends with exactly one terminal `RET_OK` / `RET_ERR`; the dev decides + what to do with a slow one. Replaces the never-released per-proc + `{.ffi: "timeout = ".}` override and the `defaultRequestTimeout` context + field ([#126](https://github.com/logos-messaging/nim-ffi/issues/126), + supersedes [#93](https://github.com/logos-messaging/nim-ffi/issues/93)). - Per-interaction ABI-format annotations: `declareLibrary` now takes an optional `defaultABIFormat` (`"cbor"` default, or `"c"`) that every `{.ffi.}` / `{.ffiCtor.}` / `{.ffiDtor.}` / `{.ffiRaw.}` / `{.ffiEvent.}` diff --git a/README.md b/README.md index 87e4cf3..5aab5e3 100644 --- a/README.md +++ b/README.md @@ -91,26 +91,33 @@ Every `{.ffi.}` / `{.ffiCtor.}` proc must have an explicit `return ok(...)` without awaiting). The `Result`'s error string is delivered to the foreign caller as the failure message. -### Request timeouts +### The result callback contract -Every handler runs under a deadline. The default is `DefaultRequestTimeout` -(5s, `ffi/ffi_context.nim`), applied to every proc so a wedged handler can't -hang a foreign caller forever. On trip the caller is unblocked with an `ffi -request timed out after ms` error; the handler is **not** cancelled — a -hard cancel mid-call into the underlying library can leave it half-applied — so -it keeps running, and the caller's callback still fires exactly once. +Each request carries a result callback. It receives one of these status codes +(`ret` / `err_code`): -Raise or lower the deadline per proc with a `"timeout = "` spec, parsed -like the `abi = ...` spec below: +| Code | Value | Terminal? | Meaning | +| --- | --- | --- | --- | +| `RET_OK` | 0 | yes | Success; the payload carries the encoded result. | +| `RET_ERR` | 1 | yes | Failure; the payload carries the UTF-8 error string. | +| `RET_MISSING_CALLBACK` | 2 | — | No callback was passed; the request path reports this itself. | +| `RET_STALE_WARN` | 3 | **no** | Progress ping — the handler is still running. | -```nim -proc slowOp*( - c: Counter, req: BumpRequest -): Future[Result[BumpResponse, string]] {.ffi: "timeout = 30000".} = - ... -``` +**nim-ffi never times a handler out.** A slow request runs to its natural +`RET_OK` / `RET_ERR`; it is never cancelled (a hard-cancel mid-call into the +underlying library can leave it half-applied). Instead, while a handler is still +in flight the callback receives a **non-terminal** `RET_STALE_WARN` every 5s +(Android's ANR interval; override at build time with +`-d:ffiStaleWarnIntervalMs=`), with the payload carrying the elapsed +milliseconds as a decimal string. The dev decides what to do with a slow request +— keep waiting, surface a spinner, tear the context down — nim-ffi does not +decide for them. -The timeout is runtime-only; binding codegen ignores it. +`RET_STALE_WARN` may fire any number of times and is **always** followed by +exactly one terminal `RET_OK` / `RET_ERR`. A caller that only wants the final +answer must ignore it (do not treat a non-zero code as an error without checking +for `RET_STALE_WARN` first). The generated higher-level typed wrappers currently +ignore it; the progress signal is delivered at the raw result-callback boundary. ### Events diff --git a/examples/echo/c_abi_bindings/echo.h b/examples/echo/c_abi_bindings/echo.h index 8b9bfdd..d6810cf 100644 --- a/examples/echo/c_abi_bindings/echo.h +++ b/examples/echo/c_abi_bindings/echo.h @@ -9,6 +9,10 @@ #define NIMFFI_RET_OK 0 #define NIMFFI_RET_ERR 1 #define NIMFFI_RET_MISSING_CALLBACK 2 +/* Non-terminal: the request is still running. Fires every ~5s with `msg` + carrying the elapsed milliseconds as decimal text; always followed by a + terminal RET_OK/RET_ERR. Ignore it unless you want progress. */ +#define NIMFFI_RET_STALE_WARN 3 /* Flat wire structs — the C ABI. Strings are borrowed, NUL-terminated `const char*` valid only for the duration of the call they cross. */ @@ -54,6 +58,8 @@ typedef struct { EchoCreateFn fn; void* user_data; } EchoCreateBox; static void echo_create_trampoline(int ret, const char* ctx_addr, const char* err_msg, void* ud) { EchoCreateBox* box = (EchoCreateBox*)ud; if (!box) return; + /* Non-terminal progress ping: keep the box for the terminal reply. */ + if (ret == NIMFFI_RET_STALE_WARN) return; if (!box->fn) { free(box); return; } if (ret != 0) { box->fn(ret, NULL, err_msg ? err_msg : "FFI create failed", box->user_data); diff --git a/examples/echo/c_bindings/echo.h b/examples/echo/c_bindings/echo.h index d68f69d..5e3daa3 100644 --- a/examples/echo/c_bindings/echo.h +++ b/examples/echo/c_bindings/echo.h @@ -222,6 +222,8 @@ typedef void (*EchoCreateFn)(int err_code, EchoCtx* ctx, const char* err_msg, vo typedef struct { EchoCreateFn fn; void* user_data; } EchoCreateBox; static void echo_create_trampoline(int ret, const char* msg, size_t len, void* ud) { EchoCreateBox* box = (EchoCreateBox*)ud; + /* Non-terminal progress ping: keep the box for the terminal reply. */ + if (ret == NIMFFI_RET_STALE_WARN) return; if (!box->fn) { free(box); return; @@ -297,6 +299,8 @@ typedef void (*EchoShoutReplyFn)(int err_code, const ShoutResponse* reply, const typedef struct { EchoShoutReplyFn fn; void* user_data; } EchoShoutCallBox; static void echo_shout_reply_trampoline(int ret, const char* msg, size_t len, void* ud) { EchoShoutCallBox* box = (EchoShoutCallBox*)ud; + /* Non-terminal progress ping: keep the box for the terminal reply. */ + if (ret == NIMFFI_RET_STALE_WARN) return; if (!box->fn) { free(box); return; @@ -357,6 +361,8 @@ typedef void (*EchoVersionReplyFn)(int err_code, const NimFfiStr* reply, const c typedef struct { EchoVersionReplyFn fn; void* user_data; } EchoVersionCallBox; static void echo_version_reply_trampoline(int ret, const char* msg, size_t len, void* ud) { EchoVersionCallBox* box = (EchoVersionCallBox*)ud; + /* Non-terminal progress ping: keep the box for the terminal reply. */ + if (ret == NIMFFI_RET_STALE_WARN) return; if (!box->fn) { free(box); return; diff --git a/examples/echo/c_bindings/nim_ffi_cbor.h b/examples/echo/c_bindings/nim_ffi_cbor.h index fd38ffd..98d375c 100644 --- a/examples/echo/c_bindings/nim_ffi_cbor.h +++ b/examples/echo/c_bindings/nim_ffi_cbor.h @@ -20,10 +20,17 @@ typedef void (*FFICallback)(int ret, const char* msg, size_t len, void* user_dat * value handed to a result callback's `err_code` (or returned by a submit call) * is a failure. NIMFFI_RET_MISSING_CALLBACK is a special case from the Nim * dispatcher: the callback will never fire, so the request path must report the - * failure itself. */ + * failure itself. + * + * NIMFFI_RET_STALE_WARN is the one NON-terminal code: nim-ffi delivers it every + * ~5s while a handler is still running (with `msg`/`len` carrying the elapsed + * milliseconds as decimal text), then still ends with a terminal RET_OK/RET_ERR. + * A caller that only wants the final answer must ignore it, not treat it as an + * error. */ #define NIMFFI_RET_OK 0 #define NIMFFI_RET_ERROR 1 #define NIMFFI_RET_MISSING_CALLBACK 2 +#define NIMFFI_RET_STALE_WARN 3 /* ── leaf encoders ─────────────────────────────────────────────────────── */ static inline CborError nimffi_enc_bool(CborEncoder* e, const bool* v) { diff --git a/examples/echo/cpp_bindings/echo.hpp b/examples/echo/cpp_bindings/echo.hpp index fa89091..4c47cf3 100644 --- a/examples/echo/cpp_bindings/echo.hpp +++ b/examples/echo/cpp_bindings/echo.hpp @@ -29,6 +29,16 @@ extern "C" { #include } +// nim-ffi result-callback status codes (mirror ffi/ffi_types.nim and the C +// header). Guarded so a translation unit that also pulls in the C header keeps +// a single definition. +#ifndef NIMFFI_RET_OK +#define NIMFFI_RET_OK 0 +#define NIMFFI_RET_ERR 1 +#define NIMFFI_RET_MISSING_CALLBACK 2 +#define NIMFFI_RET_STALE_WARN 3 +#endif + // ============================================================ // Result — exception-free error channel // ============================================================ @@ -448,6 +458,12 @@ struct FFICallState_ { }; inline void ffi_cb_(int ret, const char* msg, size_t len, void* ud) { + // NIMFFI_RET_STALE_WARN (3) is a non-terminal progress ping: the request is + // still running. This blocking wrapper only reports the final result, so + // ignore it WITHOUT touching `ud` — a terminal callback still owns the + // shared handle and will free it. + if (ret == NIMFFI_RET_STALE_WARN) return; + // ffi_call_ heap-allocated a shared_ptr and passed its address as ud; // take ownership here so it's freed on every exit path. std::unique_ptr> handle( @@ -455,7 +471,7 @@ inline void ffi_cb_(int ret, const char* msg, size_t len, void* ud) { FFICallState_& s = **handle; std::lock_guard lock(s.mtx); - s.ok = (ret == 0); + s.ok = (ret == NIMFFI_RET_OK); if (msg && len > 0) { const auto* p = reinterpret_cast(msg); if (s.ok) s.bytes.assign(p, p + len); @@ -472,7 +488,7 @@ inline Result> ffi_call_( auto state = std::make_shared(); auto* cb_ref = new std::shared_ptr(state); const int ret = f(ffi_cb_, cb_ref); - if (ret == 2) { + if (ret == NIMFFI_RET_MISSING_CALLBACK) { delete cb_ref; return Result::err("RET_MISSING_CALLBACK (internal error)"); } diff --git a/examples/timer/c_bindings/my_timer.h b/examples/timer/c_bindings/my_timer.h index 119f614..8079020 100644 --- a/examples/timer/c_bindings/my_timer.h +++ b/examples/timer/c_bindings/my_timer.h @@ -829,6 +829,8 @@ typedef void (*MyTimerCreateFn)(int err_code, MyTimerCtx* ctx, const char* err_m typedef struct { MyTimerCreateFn fn; void* user_data; } MyTimerCreateBox; static void my_timer_create_trampoline(int ret, const char* msg, size_t len, void* ud) { MyTimerCreateBox* box = (MyTimerCreateBox*)ud; + /* Non-terminal progress ping: keep the box for the terminal reply. */ + if (ret == NIMFFI_RET_STALE_WARN) return; if (!box->fn) { free(box); return; @@ -940,6 +942,8 @@ typedef void (*MyTimerEchoReplyFn)(int err_code, const EchoResponse* reply, cons typedef struct { MyTimerEchoReplyFn fn; void* user_data; } MyTimerEchoCallBox; static void my_timer_echo_reply_trampoline(int ret, const char* msg, size_t len, void* ud) { MyTimerEchoCallBox* box = (MyTimerEchoCallBox*)ud; + /* Non-terminal progress ping: keep the box for the terminal reply. */ + if (ret == NIMFFI_RET_STALE_WARN) return; if (!box->fn) { free(box); return; @@ -1000,6 +1004,8 @@ typedef void (*MyTimerVersionReplyFn)(int err_code, const NimFfiStr* reply, cons typedef struct { MyTimerVersionReplyFn fn; void* user_data; } MyTimerVersionCallBox; static void my_timer_version_reply_trampoline(int ret, const char* msg, size_t len, void* ud) { MyTimerVersionCallBox* box = (MyTimerVersionCallBox*)ud; + /* Non-terminal progress ping: keep the box for the terminal reply. */ + if (ret == NIMFFI_RET_STALE_WARN) return; if (!box->fn) { free(box); return; @@ -1059,6 +1065,8 @@ typedef void (*MyTimerComplexReplyFn)(int err_code, const ComplexResponse* reply typedef struct { MyTimerComplexReplyFn fn; void* user_data; } MyTimerComplexCallBox; static void my_timer_complex_reply_trampoline(int ret, const char* msg, size_t len, void* ud) { MyTimerComplexCallBox* box = (MyTimerComplexCallBox*)ud; + /* Non-terminal progress ping: keep the box for the terminal reply. */ + if (ret == NIMFFI_RET_STALE_WARN) return; if (!box->fn) { free(box); return; @@ -1119,6 +1127,8 @@ typedef void (*MyTimerScheduleReplyFn)(int err_code, const ScheduleResult* reply typedef struct { MyTimerScheduleReplyFn fn; void* user_data; } MyTimerScheduleCallBox; static void my_timer_schedule_reply_trampoline(int ret, const char* msg, size_t len, void* ud) { MyTimerScheduleCallBox* box = (MyTimerScheduleCallBox*)ud; + /* Non-terminal progress ping: keep the box for the terminal reply. */ + if (ret == NIMFFI_RET_STALE_WARN) return; if (!box->fn) { free(box); return; diff --git a/examples/timer/c_bindings/nim_ffi_cbor.h b/examples/timer/c_bindings/nim_ffi_cbor.h index fd38ffd..98d375c 100644 --- a/examples/timer/c_bindings/nim_ffi_cbor.h +++ b/examples/timer/c_bindings/nim_ffi_cbor.h @@ -20,10 +20,17 @@ typedef void (*FFICallback)(int ret, const char* msg, size_t len, void* user_dat * value handed to a result callback's `err_code` (or returned by a submit call) * is a failure. NIMFFI_RET_MISSING_CALLBACK is a special case from the Nim * dispatcher: the callback will never fire, so the request path must report the - * failure itself. */ + * failure itself. + * + * NIMFFI_RET_STALE_WARN is the one NON-terminal code: nim-ffi delivers it every + * ~5s while a handler is still running (with `msg`/`len` carrying the elapsed + * milliseconds as decimal text), then still ends with a terminal RET_OK/RET_ERR. + * A caller that only wants the final answer must ignore it, not treat it as an + * error. */ #define NIMFFI_RET_OK 0 #define NIMFFI_RET_ERROR 1 #define NIMFFI_RET_MISSING_CALLBACK 2 +#define NIMFFI_RET_STALE_WARN 3 /* ── leaf encoders ─────────────────────────────────────────────────────── */ static inline CborError nimffi_enc_bool(CborEncoder* e, const bool* v) { diff --git a/examples/timer/cpp_bindings/my_timer.hpp b/examples/timer/cpp_bindings/my_timer.hpp index a2e4963..9190cfb 100644 --- a/examples/timer/cpp_bindings/my_timer.hpp +++ b/examples/timer/cpp_bindings/my_timer.hpp @@ -29,6 +29,16 @@ extern "C" { #include } +// nim-ffi result-callback status codes (mirror ffi/ffi_types.nim and the C +// header). Guarded so a translation unit that also pulls in the C header keeps +// a single definition. +#ifndef NIMFFI_RET_OK +#define NIMFFI_RET_OK 0 +#define NIMFFI_RET_ERR 1 +#define NIMFFI_RET_MISSING_CALLBACK 2 +#define NIMFFI_RET_STALE_WARN 3 +#endif + #include // ============================================================ // Result — exception-free error channel @@ -748,6 +758,12 @@ struct FFICallState_ { }; inline void ffi_cb_(int ret, const char* msg, size_t len, void* ud) { + // NIMFFI_RET_STALE_WARN (3) is a non-terminal progress ping: the request is + // still running. This blocking wrapper only reports the final result, so + // ignore it WITHOUT touching `ud` — a terminal callback still owns the + // shared handle and will free it. + if (ret == NIMFFI_RET_STALE_WARN) return; + // ffi_call_ heap-allocated a shared_ptr and passed its address as ud; // take ownership here so it's freed on every exit path. std::unique_ptr> handle( @@ -755,7 +771,7 @@ inline void ffi_cb_(int ret, const char* msg, size_t len, void* ud) { FFICallState_& s = **handle; std::lock_guard lock(s.mtx); - s.ok = (ret == 0); + s.ok = (ret == NIMFFI_RET_OK); if (msg && len > 0) { const auto* p = reinterpret_cast(msg); if (s.ok) s.bytes.assign(p, p + len); @@ -772,7 +788,7 @@ inline Result> ffi_call_( auto state = std::make_shared(); auto* cb_ref = new std::shared_ptr(state); const int ret = f(ffi_cb_, cb_ref); - if (ret == 2) { + if (ret == NIMFFI_RET_MISSING_CALLBACK) { delete cb_ref; return Result::err("RET_MISSING_CALLBACK (internal error)"); } diff --git a/examples/timer/rust_bindings/src/api.rs b/examples/timer/rust_bindings/src/api.rs index 14a47a1..829113b 100644 --- a/examples/timer/rust_bindings/src/api.rs +++ b/examples/timer/rust_bindings/src/api.rs @@ -31,16 +31,26 @@ unsafe fn ffi_payload(ret: c_int, msg: *const c_char, len: usize) -> FFIResult { } else { slice::from_raw_parts(msg as *const u8, len).to_vec() }; - if ret == 0 { Ok(bytes) } + if ret == NIMFFI_RET_OK { Ok(bytes) } else { Err(String::from_utf8_lossy(&bytes).into_owned()) } } +// nim-ffi result-callback status codes (mirror ffi/ffi_types.nim). +const NIMFFI_RET_OK: c_int = 0; +const NIMFFI_RET_MISSING_CALLBACK: c_int = 2; +const NIMFFI_RET_STALE_WARN: c_int = 3; + unsafe extern "C" fn on_result( ret: c_int, msg: *const c_char, len: usize, user_data: *mut c_void, ) { + // NIMFFI_RET_STALE_WARN (3) is a non-terminal progress ping: the request + // is still running. This wrapper only delivers the final result, so ignore + // it WITHOUT reclaiming the box — a terminal callback still owns the Sender. + if ret == NIMFFI_RET_STALE_WARN { return; } + // Take ownership of the boxed Sender — dropping it at end of scope // releases the only outstanding handle. let tx = Box::from_raw(user_data as *mut FFISender); @@ -66,7 +76,7 @@ where let (tx, rx) = flume::bounded::(1); let raw = Box::into_raw(Box::new(tx)) as *mut c_void; let ret = f(on_result, raw); - if ret == 2 { + if ret == NIMFFI_RET_MISSING_CALLBACK { // Callback will never fire; reclaim the box to avoid a leak. drop(unsafe { Box::from_raw(raw as *mut FFISender) }); return Err("RET_MISSING_CALLBACK (internal error)".into()); @@ -87,7 +97,7 @@ where let (tx, rx) = flume::bounded::(1); let raw = Box::into_raw(Box::new(tx)) as *mut c_void; let ret = f(on_result, raw); - if ret == 2 { + if ret == NIMFFI_RET_MISSING_CALLBACK { drop(unsafe { Box::from_raw(raw as *mut FFISender) }); return Err("RET_MISSING_CALLBACK (internal error)".into()); } diff --git a/ffi/codegen/c.nim b/ffi/codegen/c.nim index f841995..4e8fcc0 100644 --- a/ffi/codegen/c.nim +++ b/ffi/codegen/c.nim @@ -433,6 +433,10 @@ proc emitReplyTrampolineHead(lines: var seq[string], tramp, boxType, fallback: s "static void " & tramp & "(int ret, const char* msg, size_t len, void* ud) {" ) lines.add(" " & boxType & "* box = (" & boxType & "*)ud;") + lines.add( + " /* Non-terminal progress ping: keep the box for the terminal reply. */" + ) + lines.add(" if (ret == NIMFFI_RET_STALE_WARN) return;") lines.add(" if (!box->fn) {") lines.add(" free(box);") lines.add(" return;") diff --git a/ffi/codegen/c_abi.nim b/ffi/codegen/c_abi.nim index b58bf42..1582891 100644 --- a/ffi/codegen/c_abi.nim +++ b/ffi/codegen/c_abi.nim @@ -271,6 +271,10 @@ proc emitCtxAndCtor( ) lines.add(" " & createBox & "* box = (" & createBox & "*)ud;") lines.add(" if (!box) return;") + lines.add( + " /* Non-terminal progress ping: keep the box for the terminal reply. */" + ) + lines.add(" if (ret == NIMFFI_RET_STALE_WARN) return;") lines.add(" if (!box->fn) { free(box); return; }") lines.add(" if (ret != 0) {") lines.add( @@ -402,6 +406,12 @@ proc generateCAbiLibHeader*( lines.add("#define NIMFFI_RET_OK 0") lines.add("#define NIMFFI_RET_ERR 1") lines.add("#define NIMFFI_RET_MISSING_CALLBACK 2") + lines.add("/* Non-terminal: the request is still running. Fires every ~5s with `msg`") + lines.add( + " carrying the elapsed milliseconds as decimal text; always followed by a" + ) + lines.add(" terminal RET_OK/RET_ERR. Ignore it unless you want progress. */") + lines.add("#define NIMFFI_RET_STALE_WARN 3") lines.add("") lines.add("/* Flat wire structs — the C ABI. Strings are borrowed, NUL-terminated") lines.add(" `const char*` valid only for the duration of the call they cross. */") diff --git a/ffi/codegen/meta.nim b/ffi/codegen/meta.nim index 15eeb14..7c5088d 100644 --- a/ffi/codegen/meta.nim +++ b/ffi/codegen/meta.nim @@ -80,35 +80,9 @@ proc abiCodegenImplemented*(fmt: ABIFormat): bool = 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"`. + ## used to route it to its parser. `"abi = c"` → `"abi"`. override.split('=')[0].strip().toLowerAscii() -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 = 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: - parseInt(raw) - except ValueError: - return ( - false, - 0, - "invalid timeout value: '" & raw & - "'; expected a positive integer of milliseconds", - ) - if ms <= 0: - return (false, 0, "timeout must be a positive number of milliseconds, got: " & raw) - (true, ms, "") - proc parseABIFormatName*(name: string): tuple[ok: bool, fmt: ABIFormat] = ## Bare format name (`"c"`/`"cbor"`, case-insensitive) → `ABIFormat`; ## `ok` is false otherwise. diff --git a/ffi/codegen/rust.nim b/ffi/codegen/rust.nim index 56544f3..671ecc0 100644 --- a/ffi/codegen/rust.nim +++ b/ffi/codegen/rust.nim @@ -368,16 +368,32 @@ proc generateApiRs*( lines.add(" } else {") lines.add(" slice::from_raw_parts(msg as *const u8, len).to_vec()") lines.add(" };") - lines.add(" if ret == 0 { Ok(bytes) }") + lines.add(" if ret == NIMFFI_RET_OK { Ok(bytes) }") lines.add(" else { Err(String::from_utf8_lossy(&bytes).into_owned()) }") lines.add("}") lines.add("") + lines.add("// nim-ffi result-callback status codes (mirror ffi/ffi_types.nim).") + lines.add("const NIMFFI_RET_OK: c_int = 0;") + lines.add("const NIMFFI_RET_MISSING_CALLBACK: c_int = 2;") + lines.add("const NIMFFI_RET_STALE_WARN: c_int = 3;") + lines.add("") lines.add("unsafe extern \"C\" fn on_result(") lines.add(" ret: c_int,") lines.add(" msg: *const c_char,") lines.add(" len: usize,") lines.add(" user_data: *mut c_void,") lines.add(") {") + lines.add( + " // NIMFFI_RET_STALE_WARN (3) is a non-terminal progress ping: the request" + ) + lines.add( + " // is still running. This wrapper only delivers the final result, so ignore" + ) + lines.add( + " // it WITHOUT reclaiming the box — a terminal callback still owns the Sender." + ) + lines.add(" if ret == NIMFFI_RET_STALE_WARN { return; }") + lines.add("") lines.add(" // Take ownership of the boxed Sender — dropping it at end of scope") lines.add(" // releases the only outstanding handle.") lines.add(" let tx = Box::from_raw(user_data as *mut FFISender);") @@ -419,7 +435,7 @@ proc generateApiRs*( lines.add(" let (tx, rx) = flume::bounded::(1);") lines.add(" let raw = Box::into_raw(Box::new(tx)) as *mut c_void;") lines.add(" let ret = f(on_result, raw);") - lines.add(" if ret == 2 {") + lines.add(" if ret == NIMFFI_RET_MISSING_CALLBACK {") lines.add(" // Callback will never fire; reclaim the box to avoid a leak.") lines.add(" drop(unsafe { Box::from_raw(raw as *mut FFISender) });") lines.add(" return Err(\"RET_MISSING_CALLBACK (internal error)\".into());") @@ -442,7 +458,7 @@ proc generateApiRs*( lines.add(" let (tx, rx) = flume::bounded::(1);") lines.add(" let raw = Box::into_raw(Box::new(tx)) as *mut c_void;") lines.add(" let ret = f(on_result, raw);") - lines.add(" if ret == 2 {") + lines.add(" if ret == NIMFFI_RET_MISSING_CALLBACK {") lines.add(" drop(unsafe { Box::from_raw(raw as *mut FFISender) });") lines.add(" return Err(\"RET_MISSING_CALLBACK (internal error)\".into());") lines.add(" }") diff --git a/ffi/codegen/templates/c/cbor_helpers.h.tpl b/ffi/codegen/templates/c/cbor_helpers.h.tpl index a0c463a..3a5e29e 100644 --- a/ffi/codegen/templates/c/cbor_helpers.h.tpl +++ b/ffi/codegen/templates/c/cbor_helpers.h.tpl @@ -20,10 +20,17 @@ typedef void (*FFICallback)(int ret, const char* msg, size_t len, void* user_dat * value handed to a result callback's `err_code` (or returned by a submit call) * is a failure. NIMFFI_RET_MISSING_CALLBACK is a special case from the Nim * dispatcher: the callback will never fire, so the request path must report the - * failure itself. */ + * failure itself. + * + * NIMFFI_RET_STALE_WARN is the one NON-terminal code: nim-ffi delivers it every + * ~5s while a handler is still running (with `msg`/`len` carrying the elapsed + * milliseconds as decimal text), then still ends with a terminal RET_OK/RET_ERR. + * A caller that only wants the final answer must ignore it, not treat it as an + * error. */ #define NIMFFI_RET_OK 0 #define NIMFFI_RET_ERROR 1 #define NIMFFI_RET_MISSING_CALLBACK 2 +#define NIMFFI_RET_STALE_WARN 3 /* ── leaf encoders ─────────────────────────────────────────────────────── */ static inline CborError nimffi_enc_bool(CborEncoder* e, const bool* v) { diff --git a/ffi/codegen/templates/cpp/header_prelude.hpp.tpl b/ffi/codegen/templates/cpp/header_prelude.hpp.tpl index 66029b6..91713ef 100644 --- a/ffi/codegen/templates/cpp/header_prelude.hpp.tpl +++ b/ffi/codegen/templates/cpp/header_prelude.hpp.tpl @@ -28,3 +28,13 @@ extern "C" { #include } + +// nim-ffi result-callback status codes (mirror ffi/ffi_types.nim and the C +// header). Guarded so a translation unit that also pulls in the C header keeps +// a single definition. +#ifndef NIMFFI_RET_OK +#define NIMFFI_RET_OK 0 +#define NIMFFI_RET_ERR 1 +#define NIMFFI_RET_MISSING_CALLBACK 2 +#define NIMFFI_RET_STALE_WARN 3 +#endif diff --git a/ffi/codegen/templates/cpp/sync_call_helper.hpp.tpl b/ffi/codegen/templates/cpp/sync_call_helper.hpp.tpl index bfe833a..8229383 100644 --- a/ffi/codegen/templates/cpp/sync_call_helper.hpp.tpl +++ b/ffi/codegen/templates/cpp/sync_call_helper.hpp.tpl @@ -17,6 +17,12 @@ struct FFICallState_ { }; inline void ffi_cb_(int ret, const char* msg, size_t len, void* ud) { + // NIMFFI_RET_STALE_WARN (3) is a non-terminal progress ping: the request is + // still running. This blocking wrapper only reports the final result, so + // ignore it WITHOUT touching `ud` — a terminal callback still owns the + // shared handle and will free it. + if (ret == NIMFFI_RET_STALE_WARN) return; + // ffi_call_ heap-allocated a shared_ptr and passed its address as ud; // take ownership here so it's freed on every exit path. std::unique_ptr> handle( @@ -24,7 +30,7 @@ inline void ffi_cb_(int ret, const char* msg, size_t len, void* ud) { FFICallState_& s = **handle; std::lock_guard lock(s.mtx); - s.ok = (ret == 0); + s.ok = (ret == NIMFFI_RET_OK); if (msg && len > 0) { const auto* p = reinterpret_cast(msg); if (s.ok) s.bytes.assign(p, p + len); @@ -41,7 +47,7 @@ inline Result> ffi_call_( auto state = std::make_shared(); auto* cb_ref = new std::shared_ptr(state); const int ret = f(ffi_cb_, cb_ref); - if (ret == 2) { + if (ret == NIMFFI_RET_MISSING_CALLBACK) { delete cb_ref; return Result::err("RET_MISSING_CALLBACK (internal error)"); } diff --git a/ffi/ffi_context.nim b/ffi/ffi_context.nim index 751e63c..7833188 100644 --- a/ffi/ffi_context.nim +++ b/ffi/ffi_context.nim @@ -42,12 +42,9 @@ type FFIContext*[T] = object # keeps the event thread draining until then so teardown-emitted events land running: Atomic[bool] # To control when the threads are running registeredRequests: ptr Table[cstring, FFIRequestProc] - requestTimeouts: ptr Table[cstring, int] - # 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 - # Per-handler deadline unless a `{.ffi: "timeout = ".}` override raises - # it; `InfiniteDuration` opts out. See processRequest for the trip behavior. + staleWarnInterval*: Duration + # 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,8 +55,12 @@ const EventThreadTickInterval* = 1.seconds FFIHeartbeatStartDelay* = 10.seconds # grace window for library startup FFIHeartbeatStaleThreshold* = 1.seconds - DefaultRequestTimeout* = 5.seconds - # Finite fallback (issue #93) so a wedged handler can't hang a caller forever. + +const StaleWarnIntervalMs* {.intdefine: "ffiStaleWarnIntervalMs".} = 5000 + ## `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.} @@ -130,7 +131,7 @@ proc initContextResources*[T](ctx: ptr FFIContext[T]): Result[void, string] = ctx.ffiHeartbeat.store(0) ctx.eventQueueStuck.store(false) ctx.ffiThreadExited.store(false) - ctx.defaultRequestTimeout = DefaultRequestTimeout + ctx.staleWarnInterval = StaleWarnInterval var success = false defer: @@ -146,7 +147,6 @@ proc initContextResources*[T](ctx: ptr FFIContext[T]): Result[void, string] = newSignalOrErr(ctx.eventThreadExitSignal, "eventThreadExitSignal") ctx.registeredRequests = addr ffi_types.registeredRequests - ctx.requestTimeouts = addr ffi_types.requestTimeoutsMs ctx.running.store(true) diff --git a/ffi/ffi_thread.nim b/ffi/ffi_thread.nim index f98834b..61076ef 100644 --- a/ffi/ffi_thread.nim +++ b/ffi/ffi_thread.nim @@ -44,46 +44,36 @@ proc sendRequestToFFIThread*( ok() -func resolveRequestTimeout[T](reqIdCs: cstring, ctx: ptr FFIContext[T]): Duration = - ## Per-proc `{.ffi: "timeout = ".}` override if one was registered for this - ## request type, otherwise the context-wide default. - let ms = ctx[].requestTimeouts[].getOrDefault(reqIdCs, 0) - if ms > 0: ms.milliseconds else: ctx.defaultRequestTimeout - -proc reportTimeoutIfTripped( +proc awaitWithStaleWarnings( retFut: Future[Result[seq[byte], string]], request: ptr FFIThreadRequest, - deadline: Duration, + interval: Duration, reqId: string, -) {.async.} = - ## Waits for the handler or its deadline, whichever comes first. On a trip we - ## 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. `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, - # keeping the per-request cost off the fast path. - if retFut.finished(): - return - let timer = sleepAsync(deadline) - # `race` returns the first to finish WITHOUT cancelling the loser, so the - # handler keeps running when the timer wins. - discard await race(retFut, timer) - if not timer.finished(): - await timer.cancelAndWait() - if retFut.finished(): - return - warn "ffi request timed out; caller unblocked, handler left running", - reqId = reqId, timeoutMs = deadline.milliseconds - fireCallback( - Result[seq[byte], string].err( - "ffi request timed out after " & $deadline.milliseconds & "ms" - ), - request, - ) +): Future[Result[seq[byte], string]] {.async.} = + ## 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. + return await retFut + var elapsed = 0'i64 + while not retFut.finished(): + let timer = sleepAsync(interval) + # `race` returns the first to finish WITHOUT cancelling the loser, so the + # handler keeps running when the timer wins. + discard await race(retFut, timer) + if retFut.finished(): + if not timer.finished(): + await timer.cancelAndWait() + break + elapsed += intervalMs + warn "ffi request still in flight; caller notified via RET_STALE_WARN", + reqId = reqId, elapsedMs = elapsed + fireStaleWarn(request, elapsed) + return await retFut proc processRequest[T]( request: ptr FFIThreadRequest, ctx: ptr FFIContext[T] @@ -100,16 +90,12 @@ proc processRequest[T]( else: ctx[].registeredRequests[][reqIdCs](cast[pointer](request), ctx) - # CatchableError covers CancelledError from the shutdown drain; handleRes must - # still run, so the timeout race and the handler await share one try — a cancel - # mid-race 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: - # May answer the caller early with a timeout err; the handler keeps running. - await reportTimeoutIfTripped( - retFut, request, resolveRequestTimeout(reqIdCs, ctx), reqId - ) - await retFut + await awaitWithStaleWarnings(retFut, request, ctx.staleWarnInterval, reqId) except CatchableError as e: Result[seq[byte], string].err( "Error in processRequest for " & reqId & ": " & e.msg diff --git a/ffi/ffi_thread_request.nim b/ffi/ffi_thread_request.nim index bd7d3c6..da723bb 100644 --- a/ffi/ffi_thread_request.nim +++ b/ffi/ffi_thread_request.nim @@ -251,9 +251,26 @@ proc fireCallback*(res: Result[seq[byte], string], request: ptr FFIThreadRequest RET_OK, cast[ptr cchar](addr sentinel), 1.csize_t, request[].userData ) +proc fireStaleWarn*(request: ptr FFIThreadRequest, elapsedMs: int64) = + ## 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: + let msg = $elapsedMs + request[].callback( + RET_STALE_WARN, + cast[ptr cchar](unsafeAddr msg[0]), + cast[csize_t](msg.len), + request[].userData, + ) + 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. + ## Terminal step of every request: delivers the response and frees the request + ## 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 a3add13..b615fa7 100644 --- a/ffi/ffi_types.nim +++ b/ffi/ffi_types.nim @@ -7,10 +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` 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: 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 ################################################################################ @@ -37,11 +47,5 @@ template foreignThreadGc*(body: untyped) = ## The value is a proc that handles the request asynchronously. var registeredRequests*: Table[cstring, FFIRequestProc] -## Per-request handler-timeout overrides in milliseconds, keyed by the same Req -## type name as `registeredRequests`. Populated at compile time from a -## `{.ffi: "timeout = ".}` spec; an absent key means "use the context's -## `defaultRequestTimeout`". Like `registeredRequests`, never mutated at run time. -var requestTimeoutsMs*: Table[cstring, int] - ### End of FFI utils ################################################################################ diff --git a/ffi/internal/c_macro_helpers.nim b/ffi/internal/c_macro_helpers.nim index 7d4d312..a5ef23c 100644 --- a/ffi/internal/c_macro_helpers.nim +++ b/ffi/internal/c_macro_helpers.nim @@ -686,6 +686,11 @@ proc objectTrampBody(boxName, respType, respWire: NimNode): NimNode = let box = cast[ptr `boxName`](ud) if box.isNil(): return + if ret == RET_STALE_WARN: + # Non-terminal progress signal: keep the box for the eventual terminal + # reply and don't decode (there's no reply payload yet). Typed wrappers + # don't surface it; the raw FFICallBack boundary does. + return defer: freeBox(box) if box.fn.isNil(): @@ -719,6 +724,11 @@ proc stringTrampBody(boxName: NimNode): NimNode = let box = cast[ptr `boxName`](ud) if box.isNil(): return + if ret == RET_STALE_WARN: + # Non-terminal progress signal: keep the box for the eventual terminal + # reply and don't decode. Typed wrappers don't surface it; the raw + # FFICallBack boundary does. + return defer: freeBox(box) if box.fn.isNil(): diff --git a/ffi/internal/ffi_macro.nim b/ffi/internal/ffi_macro.nim index b0b9cf5..420cd59 100644 --- a/ffi/internal/ffi_macro.nim +++ b/ffi/internal/ffi_macro.nim @@ -61,19 +61,14 @@ proc resolveABIFormat(abiSpecs: seq[NimNode]): ABIFormat {.compileTime.} = fmt = parsed.fmt fmt -proc resolveFFISpecs( - specs: seq[NimNode] -): tuple[abi: ABIFormat, timeoutMs: int] {.compileTime.} = - ## Resolve an annotation's `"abi = ..."` and `"timeout = ..."` string specs - ## (last of each wins), inheriting the library-default ABI when absent. - ## `timeoutMs == 0` means "no per-proc override" (use the context default). +proc resolveFFISpecs(specs: seq[NimNode]): ABIFormat {.compileTime.} = + ## Resolve an annotation's `"abi = ..."` string specs (last wins), inheriting + ## the library-default ABI when absent. var abi = currentDefaultABIFormat - var timeoutMs = 0 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: " & override.repr + "FFI override must be a string literal like \"abi = c\", got: " & override.repr ) case overrideKey($override) of "abi": @@ -81,30 +76,9 @@ proc resolveFFISpecs( if not parsed.ok: error(parsed.err) abi = parsed.fmt - of "timeout": - let parsed = parseTimeoutSpec($override) - if not parsed.ok: - error(parsed.err) - timeoutMs = parsed.ms else: - error( - "unknown FFI override '" & $override & - "'; expected `abi = ...` or `timeout = ...`" - ) - (abi, timeoutMs) - -proc registerRequestTimeout( - reqTypeName: NimNode, timeoutMs: int -): NimNode {.compileTime.} = - ## Top-level assignment that records a per-proc handler timeout at module init, - ## keyed by the same Req type name the dispatcher registry uses. Empty when no - ## override was given. - if timeoutMs <= 0: - return newStmtList() - newAssignment( - newTree(nnkBracketExpr, ident("requestTimeoutsMs"), newLit($reqTypeName)), - newLit(timeoutMs), - ) + error("unknown FFI override '" & $override & "'; expected `abi = ...`") + abi proc gateABIFormat(fmt: ABIFormat, where: string) {.compileTime.} = ## Abort if the selected ABI's codegen isn't wired yet (only `Cbor` is), so a @@ -678,7 +652,7 @@ macro ffiRaw*(args: varargs[untyped]): untyped = requireBeforeGenBindings("`.ffiRaw.`") requireLibraryDeclared("`.ffiRaw.`") let prc = args[^1] - let (rawAbiFormat, rawTimeoutMs) = resolveFFISpecs(args[0 ..^ 2]) + let rawAbiFormat = resolveFFISpecs(args[0 ..^ 2]) gateABIFormat(rawAbiFormat, "`.ffiRaw.` proc") let procName = prc[0] @@ -752,8 +726,7 @@ macro ffiRaw*(args: varargs[untyped]): untyped = registerReqFFI(`reqName`, `paramIdent`: `paramType`): `anonymousProcNode` - let stmts = - newStmtList(registerReq, ffiProc, registerRequestTimeout(reqName, rawTimeoutMs)) + let stmts = newStmtList(registerReq, ffiProc) when defined(ffiDumpMacros): echo stmts.repr @@ -827,14 +800,12 @@ macro ffi*(args: varargs[untyped]): untyped = requireBeforeGenBindings("`.ffi.`") # Annotated node is the last vararg; leading args are `"abi = ..."` specs. let prc = args[^1] - let (abiFormat, timeoutMs) = resolveFFISpecs(args[0 ..^ 2]) + let abiFormat = resolveFFISpecs(args[0 ..^ 2]) # A value type stands alone (no library required). Its `c` companion is # emitted later by `genBindings()`, since a type-pragma macro can only return # 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 only applies to procs)") gateFFITypeABIFormat(abiFormat, "`.ffi.` type") var cleanTypeDef = prc.copyNimTree() if cleanTypeDef[0].kind == nnkPragmaExpr: @@ -1096,13 +1067,9 @@ macro ffi*(args: varargs[untyped]): untyped = cExportName, libTypeName, reqTypeName, extraParamNames, extraParamTypes, resultRetType, ) - return newStmtList( - helperProc, registerReq, registerRequestTimeout(reqTypeName, timeoutMs) - ) + return newStmtList(helperProc, registerReq) - return newStmtList( - helperProc, registerReq, ffiProc, registerRequestTimeout(reqTypeName, timeoutMs) - ) + return newStmtList(helperProc, registerReq, ffiProc) proc scalarPath(): NimNode = ## The scalar fast path lives in `ffi_scalar`; here we only build the shared @@ -1377,7 +1344,7 @@ macro ffiCtor*(args: varargs[untyped]): untyped = requireBeforeGenBindings("`.ffiCtor.`") requireLibraryDeclared("`.ffiCtor.`") let prc = args[^1] - let (abiFormat, timeoutMs) = resolveFFISpecs(args[0 ..^ 2]) + let abiFormat = resolveFFISpecs(args[0 ..^ 2]) gateABIFormat(abiFormat, "`.ffiCtor.` proc") let procName = prc[0] @@ -1551,25 +1518,10 @@ macro ffiCtor*(args: varargs[untyped]): untyped = # The flat-struct exported wrapper is emitted at genBindings() time (see # flushCAbiDispatch); the CBOR `ffiProc` is not. registerCAbiCtor(cExportName, libTypeName, reqTypeName, paramNames, paramTypes) - newStmtList( - typeDef, - ffiNewReqProc, - helperProc, - processProc, - addToReg, - poolDecl, - registerRequestTimeout(reqTypeName, timeoutMs), - ) + newStmtList(typeDef, ffiNewReqProc, helperProc, processProc, addToReg, poolDecl) else: newStmtList( - typeDef, - ffiNewReqProc, - helperProc, - processProc, - addToReg, - poolDecl, - ffiProc, - registerRequestTimeout(reqTypeName, timeoutMs), + typeDef, ffiNewReqProc, helperProc, processProc, addToReg, poolDecl, ffiProc ) when defined(ffiDumpMacros): diff --git a/tests/unit/test_abi_format.nim b/tests/unit/test_abi_format.nim index 4e486f4..825aa57 100644 --- a/tests/unit/test_abi_format.nim +++ b/tests/unit/test_abi_format.nim @@ -38,13 +38,6 @@ proc abitest_echo*( ): Future[Result[int, string]] {.ffi: "abi = cbor".} = return ok(n) -# Per-proc handler-timeout override (issue #93): parsed like the abi spec and -# recorded in `requestTimeoutsMs`, keyed by the generated Req type name. -proc abitest_slow*( - lib: AbiLib, n: int -): Future[Result[int, string]] {.ffi: "timeout = 30000".} = - return ok(n) - # Event with an explicit ABI override passed after the wire name. proc abitest_pinged*(p: Pinged) {.ffiEvent("on_pinged", "abi = cbor").} @@ -98,30 +91,12 @@ suite "ABI format parsing": check parseAbiSpec("abi = bson").ok == false # unknown format check "bson" in parseAbiSpec("abi = bson").err -suite "handler-timeout spec parsing (issue #93)": +suite "pragma override key parsing": test "overrideKey extracts the lowercased, trimmed key": - check overrideKey("timeout = 30000") == "timeout" + check overrideKey("abi = c") == "abi" check overrideKey(" ABI = c ") == "abi" check overrideKey("bare") == "bare" - test "parseTimeoutSpec accepts `timeout = `, flexible spacing": - check parseTimeoutSpec("timeout = 30000") == (true, 30000, "") - check parseTimeoutSpec("TIMEOUT=100").ms == 100 - check parseTimeoutSpec(" timeout = 5 ").ms == 5 - - test "parseTimeoutSpec rejects malformed specs and non-positive values": - check parseTimeoutSpec("30000").ok == false # missing `timeout =` - check parseTimeoutSpec("abi = c").ok == false # wrong key - check parseTimeoutSpec("timeout = 1 = 2").ok == false # too many `=` - check parseTimeoutSpec("timeout = abc").ok == false # not an integer - check parseTimeoutSpec("timeout = 0").ok == false # must be positive - check parseTimeoutSpec("timeout = -5").ok == false # must be positive - - test "a `timeout` override is recorded; a plain proc has no entry": - # Populated at module init from the annotations above. - check requestTimeoutsMs["AbitestSlowReq".cstring] == 30000 - check not requestTimeoutsMs.hasKey("AbitestPingReq".cstring) - suite "ABI proc-dispatch readiness": test "both cbor and c proc-dispatch are wired": # This predicate is what the proc-form macros consult. Both ABIs now have a diff --git a/tests/unit/test_ffi_context.nim b/tests/unit/test_ffi_context.nim index 2af7bee..b1080a1 100644 --- a/tests/unit/test_ffi_context.nim +++ b/tests/unit/test_ffi_context.nim @@ -27,6 +27,10 @@ proc deinitCallbackData(d: var CallbackData) = proc testCallback( retCode: cint, msg: ptr cchar, len: csize_t, userData: pointer ) {.cdecl, gcsafe, raises: [].} = + # 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) acquire(d[].lock) d[].retCode = retCode @@ -228,12 +232,6 @@ suite "destroyFFIContext refc workaround": check false return - # This case stresses the refc destroy workaround, not the request timeout: - # the 50k-allocation handler can outrun the finite default deadline on a slow - # sanitizer/ARM runner, tripping a spurious timeout err. Opt out so the check - # below observes the handler's real result. - ctx.defaultRequestTimeout = InfiniteDuration - var d: CallbackData initCallbackData(d) defer: @@ -619,25 +617,18 @@ suite "reentrancy guard (PR #23 review, item 6)": check nestedMsg.startsWith("err:") check "reentrant ffi call" in nestedMsg -# Per-proc handler timeout (issue #93): a `{.ffi: "timeout = ".}` override -# bounds how long a handler may run before the caller is unblocked with an err. -# The handler is NOT cancelled — it keeps running — so the callback must still -# fire exactly once. +# Non-terminal RET_STALE_WARN progress signal: while a handler runs, the caller +# is pinged every `ctx.staleWarnInterval` with the elapsed time, then still gets +# exactly one terminal RET_OK/RET_ERR. nim-ffi never times the handler out. -type TimeoutConfig {.ffi.} = object +type StaleConfig {.ffi.} = object dummy: int -proc testlib_slow_timeout*( - lib: SimpleLib, cfg: TimeoutConfig -): Future[Result[string, string]] {.ffi: "timeout = 100".} = - await sleepAsync(500.milliseconds) - return ok("slow-timeout-done") - -proc testlib_under_deadline*( - lib: SimpleLib, cfg: TimeoutConfig -): Future[Result[string, string]] {.ffi: "timeout = 1000".} = - await sleepAsync(50.milliseconds) - return ok("under-deadline-done") +proc testlib_slow_stale*( + lib: SimpleLib, cfg: StaleConfig +): Future[Result[string, string]] {.ffi.} = + await sleepAsync(350.milliseconds) + return ok("slow-stale-done") proc createSimpleCtx(): ptr FFIContext[SimpleLib] = ## Spins up a SimpleLib context via the ctor and returns it (nil on failure). @@ -658,51 +649,86 @@ proc createSimpleCtx(): ptr FFIContext[SimpleLib] = return nil cast[ptr FFIContext[SimpleLib]](ctxAddr) -suite "per-proc request timeout (issue #93)": - test "handler past its deadline yields a timeout err, fired exactly once": +## Callback that keeps the non-terminal stale pings apart from the one terminal +## answer, so a test can assert on both. +type StaleData = object + lock: Lock + cond: Cond + staleCount: int + lastElapsed: string + terminalDone: bool + terminalRet: cint + terminalBytes: seq[byte] + +proc initStaleData(d: var StaleData) = + d.lock.initLock() + d.cond.initCond() + +proc deinitStaleData(d: var StaleData) = + d.cond.deinitCond() + d.lock.deinitLock() + +proc staleCallback( + retCode: cint, msg: ptr cchar, len: csize_t, userData: pointer +) {.cdecl, gcsafe, raises: [].} = + let d = cast[ptr StaleData](userData) + let n = int(len) + acquire(d[].lock) + if retCode == RET_STALE_WARN: + var s = newString(n) + if n > 0 and not msg.isNil: + copyMem(addr s[0], msg, n) + inc d[].staleCount + d[].lastElapsed = s + else: + var b = newSeq[byte](n) + if n > 0 and not msg.isNil: + copyMem(addr b[0], msg, n) + d[].terminalRet = retCode + d[].terminalBytes = b + d[].terminalDone = true + signal(d[].cond) + release(d[].lock) + +proc waitTerminal(d: var StaleData) = + acquire(d.lock) + while not d.terminalDone: + wait(d.cond, d.lock) + release(d.lock) + +suite "non-terminal RET_STALE_WARN progress signal": + test "a slow handler pings the caller, then delivers one terminal RET_OK": let ctx = createSimpleCtx() check not ctx.isNil() defer: check SimpleLibFFIPool.destroyFFIContext(ctx).isOk() - var d: CallbackData - initCallbackData(d) - defer: - deinitCallbackData(d) + # Tune the cadence down so the 350 ms handler trips several pings quickly. + ctx.staleWarnInterval = 80.milliseconds - var reqBytes = cborEncode(TestlibSlowTimeoutReq(cfg: TimeoutConfig(dummy: 0))) - let ret = testlib_slow_timeout( - ctx, testCallback, addr d, encodedPtr(reqBytes), reqBytes.len.csize_t + var d: StaleData + initStaleData(d) + defer: + deinitStaleData(d) + + var reqBytes = cborEncode(TestlibSlowStaleReq(cfg: StaleConfig(dummy: 0))) + let ret = testlib_slow_stale( + ctx, staleCallback, addr d, encodedPtr(reqBytes), reqBytes.len.csize_t ) check ret == RET_OK - waitCallback(d) - check d.retCode == RET_ERR - check "timed out" in callbackErr(d) + waitTerminal(d) - # The handler (500 ms) is still running past the 100 ms deadline; once it - # finishes it must NOT deliver a second callback. - os.sleep(700) - check d.callCount == 1 + # A 350 ms handler at an 80 ms cadence trips at least a couple of pings, and + # the k-th ping reports exactly k*80 ms of elapsed time. + check d.staleCount >= 2 + check parseInt(d.lastElapsed) == d.staleCount * 80 - test "handler finishing under its deadline returns normally": - let ctx = createSimpleCtx() - check not ctx.isNil() - defer: - check SimpleLibFFIPool.destroyFFIContext(ctx).isOk() + # Exactly one terminal answer carrying the handler's real result. + check d.terminalRet == RET_OK + check cborDecode(d.terminalBytes, string).value == "slow-stale-done" - var d: CallbackData - initCallbackData(d) - defer: - deinitCallbackData(d) - - var reqBytes = cborEncode(TestlibUnderDeadlineReq(cfg: TimeoutConfig(dummy: 0))) - let ret = testlib_under_deadline( - ctx, testCallback, addr d, encodedPtr(reqBytes), reqBytes.len.csize_t - ) - check ret == RET_OK - - waitCallback(d) - check d.retCode == RET_OK - check cborDecode(callbackBytes(d), string).value == "under-deadline-done" - check d.callCount == 1 + # Nothing trails the terminal callback. + let staleAtTerminal = d.staleCount + os.sleep(200) + check d.staleCount == staleAtTerminal