From 98fb7da9e2f8e8e6bca0cd5dc5d185428337e16d Mon Sep 17 00:00:00 2001 From: Gabriel Cruz <8129788+gmelodie@users.noreply.github.com> Date: Wed, 22 Jul 2026 16:21:18 -0300 Subject: [PATCH] chore(c): ctx_destroy returns int (#135) --- CHANGELOG.md | 11 ++++++ examples/echo/c_abi_bindings/echo.h | 8 ++-- examples/echo/c_bindings/echo.h | 8 ++-- examples/timer/c_bindings/my_timer.h | 12 ++++-- ffi/codegen/c.nim | 31 ++++++++-------- tests/e2e/c/test_timer_e2e.c | 2 +- tests/e2e/c_abi/test_echo_c_abi.c | 2 +- tests/unit/test_c_abi_codegen.nim | 13 +++++++ tests/unit/test_c_codegen.nim | 55 ++++++++++++++++++++++++++++ 9 files changed, 115 insertions(+), 27 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ec6cb5c..24d0abc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,17 @@ All notable changes to this project are documented in this file. ## [Unreleased] ### Changed +- The generated C `_ctx_destroy()` now returns `int` instead of `void`, + propagating the exported `_destroy()` status code (`NIMFFI_RET_OK` on + success, `RET_ERR` on a null/invalid context or a failed context teardown) + instead of discarding it, so a host can observe a failed teardown. Existing + callers that invoke it as a statement are unaffected + ([#133](https://github.com/logos-messaging/nim-ffi/issues/133)). +- A failed `_ctx_destroy()` no longer frees the event-listener boxes. A + non-`NIMFFI_RET_OK` teardown leaves the worker threads live, and they still + hold each box as callback `user_data`; the boxes are now leaked rather than + freed out from under a running event thread. The context struct and the + listener array are still freed unconditionally. - User event callbacks now run on a dedicated event thread fed by a bounded SPSC queue (default capacity 1024), so a slow listener can no longer block the FFI thread or concurrent `add_event_listener` / diff --git a/examples/echo/c_abi_bindings/echo.h b/examples/echo/c_abi_bindings/echo.h index def7d56..c49b507 100644 --- a/examples/echo/c_abi_bindings/echo.h +++ b/examples/echo/c_abi_bindings/echo.h @@ -118,10 +118,12 @@ static inline int echo_ctx_create(const EchoConfig* config, EchoCreateFn on_crea return 0; } -static inline void echo_ctx_destroy(EchoCtx* ctx) { - if (!ctx) return; - if (ctx->ptr) { echo_destroy(ctx->ptr); ctx->ptr = NULL; } +static inline int echo_ctx_destroy(EchoCtx* ctx) { + if (!ctx) return NIMFFI_RET_OK; + int rc = NIMFFI_RET_OK; + if (ctx->ptr) { rc = echo_destroy(ctx->ptr); ctx->ptr = NULL; } free(ctx); + return rc; } static inline int echo_ctx_shout(const EchoCtx* ctx, const ShoutRequest* req, EchoShoutReplyFn on_reply, void* user_data) { diff --git a/examples/echo/c_bindings/echo.h b/examples/echo/c_bindings/echo.h index 5e3daa3..13e8021 100644 --- a/examples/echo/c_bindings/echo.h +++ b/examples/echo/c_bindings/echo.h @@ -289,10 +289,12 @@ static inline int echo_ctx_create(const EchoConfig* config, EchoCreateFn on_crea return 0; } -static inline void echo_ctx_destroy(EchoCtx* ctx) { - if (!ctx) return; - if (ctx->ptr) { echo_destroy(ctx->ptr); ctx->ptr = NULL; } +static inline int echo_ctx_destroy(EchoCtx* ctx) { + if (!ctx) return NIMFFI_RET_OK; + int rc = NIMFFI_RET_OK; + if (ctx->ptr) { rc = echo_destroy(ctx->ptr); ctx->ptr = NULL; } free(ctx); + return rc; } typedef void (*EchoShoutReplyFn)(int err_code, const ShoutResponse* reply, const char* err_msg, void* user_data); diff --git a/examples/timer/c_bindings/my_timer.h b/examples/timer/c_bindings/my_timer.h index 8079020..38d45df 100644 --- a/examples/timer/c_bindings/my_timer.h +++ b/examples/timer/c_bindings/my_timer.h @@ -896,12 +896,16 @@ static inline int my_timer_ctx_create(const TimerConfig* config, MyTimerCreateFn return 0; } -static inline void my_timer_ctx_destroy(MyTimerCtx* ctx) { - if (!ctx) return; - if (ctx->ptr) { my_timer_destroy(ctx->ptr); ctx->ptr = NULL; } - for (size_t i = 0; i < ctx->listeners_len; i++) free(ctx->listeners[i].box); +static inline int my_timer_ctx_destroy(MyTimerCtx* ctx) { + if (!ctx) return NIMFFI_RET_OK; + int rc = NIMFFI_RET_OK; + if (ctx->ptr) { rc = my_timer_destroy(ctx->ptr); ctx->ptr = NULL; } + if (rc == NIMFFI_RET_OK) { + for (size_t i = 0; i < ctx->listeners_len; i++) free(ctx->listeners[i].box); + } free(ctx->listeners); free(ctx); + return rc; } static inline uint64_t my_timer_ctx_add_on_echo_fired_listener(MyTimerCtx* ctx, MyTimerOnEchoFiredFn fn, void* user_data) { diff --git a/ffi/codegen/c.nim b/ffi/codegen/c.nim index b248ad5..64ae619 100644 --- a/ffi/codegen/c.nim +++ b/ffi/codegen/c.nim @@ -528,16 +528,25 @@ proc emitDestructor( ctxType, libName, dtorProcName: string, events: seq[FFIEventMeta], ) = - lines.add("static inline void " & libName & "_ctx_destroy(" & ctxType & "* ctx) {") - lines.add(" if (!ctx) return;") + lines.add("static inline int " & libName & "_ctx_destroy(" & ctxType & "* ctx) {") + lines.add(" if (!ctx) return NIMFFI_RET_OK;") + lines.add(" int rc = NIMFFI_RET_OK;") if dtorProcName.len > 0: - lines.add(" if (ctx->ptr) { " & dtorProcName & "(ctx->ptr); ctx->ptr = NULL; }") - if events.len > 0: lines.add( - " for (size_t i = 0; i < ctx->listeners_len; i++) free(ctx->listeners[i].box);" + " if (ctx->ptr) { rc = " & dtorProcName & "(ctx->ptr); ctx->ptr = NULL; }" ) + if events.len > 0: + # A failed teardown leaves the worker threads live (ffi_context.nim: + # stopAndJoinThreads), and they still hold each box as callback user_data. + # Leaking a box beats handing a running event thread a dangling pointer. + lines.add(" if (rc == NIMFFI_RET_OK) {") + lines.add( + " for (size_t i = 0; i < ctx->listeners_len; i++) free(ctx->listeners[i].box);" + ) + lines.add(" }") lines.add(" free(ctx->listeners);") lines.add(" free(ctx);") + lines.add(" return rc;") lines.add("}") lines.add("") @@ -1194,15 +1203,6 @@ proc emitAbiCtxAndCtor( lines.add("}") lines.add("") -proc emitAbiDestructor(lines: var seq[string], ctxType, libName, dtorProcName: string) = - lines.add("static inline void " & libName & "_ctx_destroy(" & ctxType & "* ctx) {") - lines.add(" if (!ctx) return;") - if dtorProcName.len > 0: - lines.add(" if (ctx->ptr) { " & dtorProcName & "(ctx->ptr); ctx->ptr = NULL; }") - lines.add(" free(ctx);") - lines.add("}") - lines.add("") - proc emitAbiMethod( lines: var seq[string], reg: var AbiReg, @@ -1400,7 +1400,8 @@ proc generateCAbiLibHeader*( lines.add("/* High-level context wrapper */") emitAbiCtxAndCtor(lines, reg, libName, libType, ctxType, classified.ctors) - emitAbiDestructor(lines, ctxType, libName, classified.dtorProcName) + # abi = c has no events, so the destructor is the CBOR one minus the listener sweep. + emitDestructor(lines, ctxType, libName, classified.dtorProcName, @[]) for m in classified.methods: if m.scalarFastPath: emitAbiScalarMethod(lines, reg, ctxType, libName, libType, m) diff --git a/tests/e2e/c/test_timer_e2e.c b/tests/e2e/c/test_timer_e2e.c index e6939e8..a4d864f 100644 --- a/tests/e2e/c/test_timer_e2e.c +++ b/tests/e2e/c/test_timer_e2e.c @@ -257,7 +257,7 @@ int main(void) { test_schedule_ok(ctx); test_schedule_error(ctx); test_event(ctx); - my_timer_ctx_destroy(ctx); + assert(my_timer_ctx_destroy(ctx) == NIMFFI_RET_OK); printf("all C e2e checks passed\n"); return 0; } diff --git a/tests/e2e/c_abi/test_echo_c_abi.c b/tests/e2e/c_abi/test_echo_c_abi.c index 893ff43..bf9967c 100644 --- a/tests/e2e/c_abi/test_echo_c_abi.c +++ b/tests/e2e/c_abi/test_echo_c_abi.c @@ -107,7 +107,7 @@ int main(void) { EchoCtx* ctx = make_ctx(); test_shout(ctx); test_version(ctx); - echo_ctx_destroy(ctx); + assert(echo_ctx_destroy(ctx) == NIMFFI_RET_OK); printf("all abi=c echo e2e checks passed\n"); return 0; } diff --git a/tests/unit/test_c_abi_codegen.nim b/tests/unit/test_c_abi_codegen.nim index 3383ed7..8df85d0 100644 --- a/tests/unit/test_c_abi_codegen.nim +++ b/tests/unit/test_c_abi_codegen.nim @@ -133,6 +133,19 @@ suite "generateCAbiLibHeader": check "timer_ctx_version(" in header check "timer_ctx_destroy(" in header + test "the context destructor propagates the destructor's status code": + check( + """ +static inline int timer_ctx_destroy(TimerCtx* ctx) { + if (!ctx) return NIMFFI_RET_OK; + int rc = NIMFFI_RET_OK; + if (ctx->ptr) { rc = timer_destroy(ctx->ptr); ctx->ptr = NULL; } + free(ctx); + return rc; +}""" in + header + ) + test "scalar-fast-path methods take inline args, not a Req struct": check "TimerAddReq" notin header check "TimerNameReq" notin header diff --git a/tests/unit/test_c_codegen.nim b/tests/unit/test_c_codegen.nim index a7ff5ab..972a8f5 100644 --- a/tests/unit/test_c_codegen.nim +++ b/tests/unit/test_c_codegen.nim @@ -124,6 +124,19 @@ suite "generateCLibHeader: ABI declarations and context API": check "timer_ctx_version(" in header check "timer_ctx_destroy(" in header + test "the context destructor propagates the destructor's status code": + check( + """ +static inline int timer_ctx_destroy(TimerCtx* ctx) { + if (!ctx) return NIMFFI_RET_OK; + int rc = NIMFFI_RET_OK; + if (ctx->ptr) { rc = timer_destroy(ctx->ptr); ctx->ptr = NULL; } + free(ctx); + return rc; +}""" in + header + ) + test "the async API is callback-driven, not blocking": # methods take a typed reply callback + user_data; no out-param, no char** err check "typedef void (*TimerVersionReplyFn)(int err_code, const NimFfiStr* reply, const char* err_msg, void* user_data);" in @@ -192,6 +205,25 @@ suite "generateCLibHeader: events": test "the context tracks listeners only when events exist": check "TimerCtxListener* listeners;" in header + test "a failed teardown leaks the listener boxes instead of dangling them": + # A non-OK rc leaves the worker threads live, still holding each box as + # callback user_data, so the sweep must sit behind the rc guard. + check( + """ +static inline int timer_ctx_destroy(TimerCtx* ctx) { + if (!ctx) return NIMFFI_RET_OK; + int rc = NIMFFI_RET_OK; + if (ctx->ptr) { rc = timer_destroy(ctx->ptr); ctx->ptr = NULL; } + if (rc == NIMFFI_RET_OK) { + for (size_t i = 0; i < ctx->listeners_len; i++) free(ctx->listeners[i].box); + } + free(ctx->listeners); + free(ctx); + return rc; +}""" in + header + ) + suite "generateCLibHeader: no-event libraries stay lean": test "a library without events has no listener bookkeeping": let procs = @[ @@ -208,6 +240,29 @@ suite "generateCLibHeader: no-event libraries stay lean": check "listeners_len" notin header check "_add_event_listener" in header # raw ABI symbol is always declared + test "a library without a dtor still reports success from ctx_destroy": + let procs = @[ + FFIProcMeta( + procName: "timer_create", + libName: "timer", + kind: FFIKind.CTOR, + libTypeName: "Timer", + extraParams: @[], + returnTypeName: "Timer", + ) + ] + let header = generateCLibHeader(procs, @[], "timer") + check( + """ +static inline int timer_ctx_destroy(TimerCtx* ctx) { + if (!ctx) return NIMFFI_RET_OK; + int rc = NIMFFI_RET_OK; + free(ctx); + return rc; +}""" in + header + ) + suite "generateCLibHeader: scalar-fast-path procs are excluded": setup: let procs = @[