add new tests and some corrections

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"

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] =

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()

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

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

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)

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