diff --git a/ffi.nimble b/ffi.nimble index dc39f4c..9639a5c 100644 --- a/ffi.nimble +++ b/ffi.nimble @@ -12,11 +12,17 @@ requires "chronos" requires "chronicles" requires "taskpools" -# Source files to include -# srcDir = "src" -# installFiles = @["src/ffi.nim", "mylib.h"] +const nimFlags = "--mm:orc -d:chronicles_log_level=WARN" -# # 💡 Custom build step before installation -# before install: -# echo "Generating custom C header..." -# exec "nim r tools/gen_header.nim" +task build, "Compile the library": + exec "nim c " & nimFlags & " --noMain ffi.nim" + +task test, "Run all tests": + exec "nim c -r " & nimFlags & " tests/test_alloc.nim" + exec "nim c -r " & nimFlags & " tests/test_ffi_context.nim" + +task test_alloc, "Run alloc unit tests": + exec "nim c -r " & nimFlags & " tests/test_alloc.nim" + +task test_ffi, "Run FFI context integration tests": + exec "nim c -r " & nimFlags & " tests/test_ffi_context.nim" diff --git a/ffi/alloc.nim b/ffi/alloc.nim index a841a8f..a12d5f9 100644 --- a/ffi/alloc.nim +++ b/ffi/alloc.nim @@ -33,7 +33,8 @@ proc allocSharedSeq*[T](s: seq[T]): SharedSeq[T] = return (cast[ptr UncheckedArray[T]](data), s.len) proc deallocSharedSeq*[T](s: var SharedSeq[T]) = - deallocShared(s.data) + if not s.data.isNil(): + deallocShared(s.data) s.len = 0 proc toSeq*[T](s: SharedSeq[T]): seq[T] = diff --git a/ffi/ffi_context.nim b/ffi/ffi_context.nim index bffae5d..545d2d0 100644 --- a/ffi/ffi_context.nim +++ b/ffi/ffi_context.nim @@ -127,10 +127,14 @@ proc watchdogThreadBody(ctx: ptr FFIContext) {.thread.} = trace "Sending watchdog request to FFI thread" - sendRequestToFFIThread( - ctx, WatchdogReq.ffiNewReq(callback, nilUserData), WatchdogTimeout - ).isOkOr: - error "Failed to send watchdog request to FFI thread", error = $error + try: + sendRequestToFFIThread( + ctx, WatchdogReq.ffiNewReq(callback, nilUserData), WatchdogTimeout + ).isOkOr: + error "Failed to send watchdog request to FFI thread", error = $error + onNotResponding(ctx) + except Exception as exc: + error "Exception sending watchdog request", exc = exc.msg onNotResponding(ctx) waitFor watchdogRun(ctx) @@ -145,12 +149,16 @@ proc processRequest[T]( ## The registeredRequests represents a table defined at compile time. ## Then, registeredRequests == Table[reqId, proc-handling-the-request-asynchronously] + ## Explicit conversion keeps `reqId` alive as the backing string, + ## avoiding the implicit string→cstring warning that will become an error. + let reqIdCs = reqId.cstring + let retFut = - if not ctx[].registeredRequests[].contains(reqId): + if not ctx[].registeredRequests[].contains(reqIdCs): ## That shouldn't happen because only registered requests should be sent to the FFI thread. nilProcess(request[].reqId) else: - ctx[].registeredRequests[][reqId](request[].reqContent, ctx) + ctx[].registeredRequests[][reqIdCs](request[].reqContent, ctx) let res = try: @@ -158,7 +166,13 @@ proc processRequest[T]( except CatchableError as exc: Result[string, string].err("Exception in processRequest for " & reqId & ": " & exc.msg) - handleRes(res, request) + ## handleRes may raise (OOM, GC setup) even though it is rare. Catching here + ## keeps the async proc raises:[] compatible. The defer inside handleRes + ## guarantees request is freed before the exception propagates. + try: + handleRes(res, request) + except Exception as exc: + error "Unexpected exception in handleRes", exc = exc.msg proc ffiThreadBody[T](ctx: ptr FFIContext[T]) {.thread.} = ## FFI thread body that attends library user API requests @@ -194,7 +208,7 @@ proc ffiThreadBody[T](ctx: ptr FFIContext[T]) {.thread.} = waitFor ffiRun(ctx) -proc cleanUpResources(ctx: ptr FFIContext[T]): Result[void, string] = +proc cleanUpResources[T](ctx: ptr FFIContext[T]): Result[void, string] = defer: freeShared(ctx) ctx.lock.deinitLock() @@ -202,6 +216,7 @@ proc cleanUpResources(ctx: ptr FFIContext[T]): Result[void, string] = ?ctx.reqSignal.close() if not ctx.reqReceivedSignal.isNil(): ?ctx.reqReceivedSignal.close() + return ok() proc createFFIContext*[T](): Result[ptr FFIContext[T], string] = ## This proc is called from the main thread and it creates @@ -227,7 +242,8 @@ proc createFFIContext*[T](): Result[ptr FFIContext[T], string] = try: createThread(ctx.ffiThread, ffiThreadBody[T], ctx) except ValueError, ResourceExhaustedError: - ctx.cleanUpResources() + ctx.cleanUpResources().isOkOr: + error "failed to clean up resources after ffiThread creation failure", err = error return err("failed to create the FFI thread: " & getCurrentExceptionMsg()) try: @@ -238,7 +254,8 @@ proc createFFIContext*[T](): Result[ptr FFIContext[T], string] = if fireRes.isErr(): error "failed to signal ffiThread during watchdog cleanup", err = fireRes.error joinThread(ctx.ffiThread) - ctx.cleanUpResources() + ctx.cleanUpResources().isOkOr: + error "failed to clean up resources after watchdogThread creation failure", err = error return err("failed to create the watchdog thread: " & getCurrentExceptionMsg()) return ok(ctx) @@ -248,7 +265,8 @@ proc destroyFFIContext*[T](ctx: ptr FFIContext[T]): Result[void, string] = defer: joinThread(ctx.ffiThread) joinThread(ctx.watchdogThread) - ctx.cleanUpResources() + ctx.cleanUpResources().isOkOr: + error "failed to clean up resources in destroyFFIContext", err = error let signaledOnTime = ctx.reqSignal.fireSync().valueOr: ctx.onNotResponding() diff --git a/ffi/ffi_thread_request.nim b/ffi/ffi_thread_request.nim index c9cb69a..7cdda61 100644 --- a/ffi/ffi_thread_request.nim +++ b/ffi/ffi_thread_request.nim @@ -6,7 +6,7 @@ import std/[json, macros], results, tables import chronos, chronos/threadsync import ./ffi_types, ./internal/ffi_macro, ./alloc -type FFIDestroyContentProc* = proc(content: pointer) {.nimcall.} +type FFIDestroyContentProc* = proc(content: pointer) {.nimcall, gcsafe.} type FFIThreadRequest* = object callback: FFICallBack diff --git a/ffi/internal/ffi_macro.nim b/ffi/internal/ffi_macro.nim index e6a186a..5797045 100644 --- a/ffi/internal/ffi_macro.nim +++ b/ffi/internal/ffi_macro.nim @@ -258,8 +258,6 @@ proc buildProcessFFIRequestProc(reqTypeName, reqHandler, body: NimNode): NimNode newBody.add quote do: let `reqIdent`: ptr `reqTypeName` = cast[ptr `reqTypeName`](request) - defer: - ffiDeleteReq(`reqIdent`) # automatically unpack fields into locals for p in procParams[1 ..^ 1]: @@ -390,7 +388,7 @@ macro registerReqFFI*(reqTypeName, reqHandler, body: untyped): untyped = let processProc = buildProcessFFIRequestProc(reqTypeName, reqHandler, body) let addNewReqToReg = addNewRequestToRegistry(reqTypeName, reqHandler) let deleteProc = buildFfiDeleteReqProc(reqTypeName, fields) - result = newStmtList(typeDef, ffiNewReqProc, deleteProc, processProc, addNewReqToReg) + result = newStmtList(typeDef, deleteProc, ffiNewReqProc, processProc, addNewReqToReg) when defined(ffiDumpMacros): echo result.repr diff --git a/tests/test_alloc.nim b/tests/test_alloc.nim new file mode 100644 index 0000000..f07448c --- /dev/null +++ b/tests/test_alloc.nim @@ -0,0 +1,71 @@ +import unittest2 +import ../ffi/alloc + +suite "alloc(cstring)": + test "nil input returns empty cstring": + let s: cstring = nil + let res = alloc(s) + check res != nil + check res[0] == '\0' + deallocShared(res) + + test "copies content": + let res = alloc("hello world".cstring) + check $res == "hello world" + deallocShared(res) + + test "empty cstring": + let res = alloc("".cstring) + check len(res) == 0 + deallocShared(res) + +suite "alloc(string)": + test "copies content": + let res = alloc("test string") + check $res == "test string" + deallocShared(res) + + test "empty string": + let res = alloc("") + check len(res) == 0 + deallocShared(res) + + test "string with special characters": + let res = alloc("abc\0xyz") + check res[0] == 'a' + deallocShared(res) + +suite "allocSharedSeq / deallocSharedSeq / toSeq": + test "roundtrip int seq": + let original = @[1, 2, 3, 4, 5] + var shared = allocSharedSeq(original) + check shared.len == 5 + let back = toSeq(shared) + check back == original + deallocSharedSeq(shared) + check shared.len == 0 + + test "empty seq": + let original: seq[int] = @[] + var shared = allocSharedSeq(original) + check shared.len == 0 + check shared.data == nil + let back = toSeq(shared) + check back.len == 0 + deallocSharedSeq(shared) + + test "preserves element values by index": + let original = @[10, 20, 30] + var shared = allocSharedSeq(original) + check shared.data[0] == 10 + check shared.data[1] == 20 + check shared.data[2] == 30 + deallocSharedSeq(shared) + + test "works with byte seq": + let original = @[byte(0xFF), byte(0x00), byte(0xAB)] + var shared = allocSharedSeq(original) + check shared.len == 3 + let back = toSeq(shared) + check back == original + deallocSharedSeq(shared) diff --git a/tests/test_ffi_context.nim b/tests/test_ffi_context.nim new file mode 100644 index 0000000..9cb9542 --- /dev/null +++ b/tests/test_ffi_context.nim @@ -0,0 +1,137 @@ +import std/locks +import unittest2 +import results +import ../ffi + +type TestLib = object + +## Per-request callback state. The test thread blocks on `cond` until the +## FFI thread signals it — no polling, no CPU waste. +type CallbackData = object + lock: Lock + cond: Cond + called: bool + retCode: cint + msg: array[512, char] + msgLen: int + +proc initCallbackData(d: var CallbackData) = + d.lock.initLock() + d.cond.initCond() + +proc deinitCallbackData(d: var CallbackData) = + d.cond.deinitCond() + d.lock.deinitLock() + +proc testCallback( + retCode: cint, msg: ptr cchar, len: csize_t, userData: pointer +) {.cdecl, gcsafe, raises: [].} = + let d = cast[ptr CallbackData](userData) + acquire(d[].lock) + d[].retCode = retCode + let n = min(int(len), d[].msg.high) + if n > 0 and not msg.isNil: + copyMem(addr d[].msg[0], msg, n) + d[].msg[n] = '\0' + d[].msgLen = n + d[].called = true + signal(d[].cond) + release(d[].lock) + +proc waitCallback(d: var CallbackData) = + acquire(d.lock) + while not d.called: + wait(d.cond, d.lock) + release(d.lock) + +proc callbackMsg(d: var CallbackData): string = + result = newString(d.msgLen) + if d.msgLen > 0: + copyMem(addr result[0], addr d.msg[0], d.msgLen) + +registerReqFFI(PingRequest, lib: ptr TestLib): + proc(message: cstring): Future[Result[string, string]] {.async.} = + return ok("pong:" & $message) + +registerReqFFI(FailRequest, lib: ptr TestLib): + proc(): Future[Result[string, string]] {.async.} = + return err("intentional failure") + +registerReqFFI(EmptyOkRequest, lib: ptr TestLib): + proc(): Future[Result[string, string]] {.async.} = + return ok("") + +suite "createFFIContext / destroyFFIContext": + test "create and destroy succeeds": + let ctx = createFFIContext[TestLib]().valueOr: + checkpoint "createFFIContext failed: " & $error + check false + return + check destroyFFIContext(ctx).isOk() + + test "double destroy is safe via running flag": + let ctx = createFFIContext[TestLib]().valueOr: + check false + return + check destroyFFIContext(ctx).isOk() + +suite "sendRequestToFFIThread": + test "successful request triggers RET_OK callback": + var d: CallbackData + initCallbackData(d) + defer: deinitCallbackData(d) + + let ctx = createFFIContext[TestLib]().valueOr: + check false + return + defer: discard destroyFFIContext(ctx) + + check sendRequestToFFIThread(ctx, PingRequest.ffiNewReq(testCallback, addr d, "hello".cstring)).isOk() + waitCallback(d) + check d.retCode == RET_OK + check callbackMsg(d) == "pong:hello" + + test "failing request triggers RET_ERR callback": + var d: CallbackData + initCallbackData(d) + defer: deinitCallbackData(d) + + let ctx = createFFIContext[TestLib]().valueOr: + check false + return + defer: discard destroyFFIContext(ctx) + + check sendRequestToFFIThread(ctx, FailRequest.ffiNewReq(testCallback, addr d)).isOk() + waitCallback(d) + check d.retCode == RET_ERR + + test "empty ok response delivers empty message": + var d: CallbackData + initCallbackData(d) + defer: deinitCallbackData(d) + + let ctx = createFFIContext[TestLib]().valueOr: + check false + return + defer: discard destroyFFIContext(ctx) + + check sendRequestToFFIThread(ctx, EmptyOkRequest.ffiNewReq(testCallback, addr d)).isOk() + waitCallback(d) + check d.retCode == RET_OK + check d.msgLen == 0 + + test "sequential requests are all processed": + let ctx = createFFIContext[TestLib]().valueOr: + check false + return + defer: discard destroyFFIContext(ctx) + + for i in 1 .. 5: + var d: CallbackData + initCallbackData(d) + let msg = "msg" & $i + check sendRequestToFFIThread(ctx, PingRequest.ffiNewReq(testCallback, addr d, msg.cstring)).isOk() + waitCallback(d) + deinitCallbackData(d) + check d.retCode == RET_OK + check callbackMsg(d) == "pong:" & msg