Merge pull request #2 from 2-towns/chore/review-and-fix-bugs

chore: review and fix bugs

api.md

@@ -98,13 +98,20 @@ Returns a `MappingResult` containing the mapping `id` (needed for `destroyMappin
 
 Returns an error if no NAT device is found, the mapping fails, or the timeout expires.
 
+> **Warning:** `onStateChange` runs on libplum's internal C thread, not the
+> chronos event loop. Do not call chronos APIs or touch non-thread-safe state
+> from it; restrict it to thread-safe operations (e.g. `Atomic`, a channel).
+
 ### destroyMapping
 
 ```nim
 proc destroyMapping*(id: cint)
 ```
 
-Removes a mapping. Must be called exactly once after a successful `createMapping`.
+Removes a mapping. Must be called exactly once after a successful `createMapping`,
+otherwise the mapping's internal handle is leaked for the lifetime of the process.
+Calling it again, or with an unknown `id`, is a safe no-op. `cleanup` also releases
+any mappings still active.
 
 ### hasMapping
 

libplum/libplum.nim

@@ -15,17 +15,6 @@ const
   libraryPath = libplumPath & "/libplum.a"
 {.passc: "-I" & includePath & " -DPLUM_STATIC".}
 {.passl: libraryPath.}
-# libplum declares some parameters as `const T*` in C (read-only pointer).
-# Nim has no equivalent, so the generated C code drops the `const`, causing
-# a type mismatch warning in GCC 15+. This pragma suppresses that warning
-# only in this translation unit and is valid for both C and C++.
-{.
-  emit: """
-#ifdef __GNUC__
-#pragma GCC diagnostic ignored "-Wno-incompatible-pointer-types"
-#endif
-"""
-.}
 
 when defined(windows):
   {.passl: "-lws2_32 -liphlpapi -lbcrypt".}
@@ -108,6 +97,8 @@ type
 
 # Import plum functions
 
+{.push raises: [].}
+
 proc plum_init*(
   config: ptr plum_config_t
 ): cint {.importc: "plum_init", header: "plum.h".}
@@ -129,3 +120,5 @@ proc plum_destroy_mapping*(
 proc plum_get_local_address*(
   buffer: cstring, size: csize_t
 ): cint {.importc: "plum_get_local_address", header: "plum.h".}
+
+{.pop.}

libplum/plum.nim

@@ -12,6 +12,10 @@ import chronos/threadsync
 import results
 import ./libplum
 
+# libplum declares some parameters as `const T*` in C (read-only pointer).
+# Nim has no equivalent, so the generated C code drops the `const`, causing
+# a type mismatch warning in GCC 15+. This pragma suppresses that warning
+# only in this translation unit and is valid for both C and C++.
 {.
   emit: """
 #ifdef __GNUC__
@@ -24,11 +28,22 @@ export results
 
 {.pragma: callback, cdecl, raises: [], gcsafe.}
 
+{.push raises: [].}
+
 type
   PlumProtocol* = enum
     TCP = PLUM_IP_PROTOCOL_TCP.int
     UDP = PLUM_IP_PROTOCOL_UDP.int
 
+  PlumLogLevel* {.pure.} = enum
+    Verbose = PLUM_LOG_LEVEL_VERBOSE.int
+    Debug = PLUM_LOG_LEVEL_DEBUG.int
+    Info = PLUM_LOG_LEVEL_INFO.int
+    Warn = PLUM_LOG_LEVEL_WARN.int
+    Error = PLUM_LOG_LEVEL_ERROR.int
+    Fatal = PLUM_LOG_LEVEL_FATAL.int
+    None = PLUM_LOG_LEVEL_NONE.int
+
   PlumState* = enum
     Destroyed = PLUM_STATE_DESTROYED.int
     Pending = PLUM_STATE_PENDING.int
@@ -55,6 +70,9 @@ type
     mapping*: PlumMapping
 
   PlumStateCallback* = proc(state: PlumState, mapping: PlumMapping) {.callback.}
+    ## Invoked on mapping state changes after the initial result. Runs on
+    ## libplum's internal C thread, not the chronos loop: only touch
+    ## thread-safe state from it (e.g. Atomic), never chronos APIs.
 
   MappingHandle = ref object
     signal: ThreadSignalPtr
@@ -70,8 +88,8 @@ type
     resolvedInternalPort: uint16
     resolvedExternalPort: uint16
     resolvedExternalHost: array[PLUM_MAX_HOST_LEN, char]
-    # Use abandoned pattern for memory freeing
-    abandoned: Atomic[bool]
+    # Refcount-like
+    signalReleases: Atomic[int]
     onStateChange: PlumStateCallback
 
 # libplum calls mappingCallback from its own C thread. Under refc, any thread
@@ -79,15 +97,21 @@ type
 template foreignThreadGc(body: untyped) =
   when declared(setupForeignThreadGc):
     setupForeignThreadGc()
-  body
-  when declared(tearDownForeignThreadGc):
-    tearDownForeignThreadGc()
+  try:
+    body
+  finally:
+    when declared(tearDownForeignThreadGc):
+      tearDownForeignThreadGc()
 
 var activeMappingsLock: Lock
 var activeMappings {.guard: activeMappingsLock.}: Table[cint, MappingHandle]
 
 initLock(activeMappingsLock)
 
+proc releaseSignal(handle: MappingHandle) =
+  if handle.signalReleases.fetchAdd(1) == 1:
+    discard handle.signal.close()
+
 # We can be confident that the pattern is GC Safe using
 # a lock.
 template withSafeLock(body: untyped) =
@@ -95,9 +119,7 @@ template withSafeLock(body: untyped) =
     withLock activeMappingsLock:
       body
 
-proc mappingCallback(
-    id: cint, state: plum_state_t, raw: ptr plum_mapping_t
-) {.cdecl, raises: [].} =
+proc mappingCallback(id: cint, state: plum_state_t, raw: ptr plum_mapping_t) {.cdecl.} =
   ## Called from libplum's internal C thread on SUCCESS, FAILURE, and DESTROYED.
 
   foreignThreadGc:
@@ -109,12 +131,7 @@ proc mappingCallback(
     if plumState == Destroyed:
       withSafeLock:
         activeMappings.del(id)
-      # The handle can be abandoned after a timeout during a
-      # mapping creation.
-      # In that case, destroy is called internally and the
-      # signal pointer can be closed.
-      if handle.abandoned.load():
-        discard handle.signal.close()
+      handle.releaseSignal()
       # Release the pin set in createMapping: the C library is done with the
       # raw pointer and will never call this callback again for this mapping.
       GC_unref(handle)
@@ -148,15 +165,15 @@ proc mappingCallback(
         handle.onStateChange(plumState, mapping)
 
 proc init*(
-    logLevel: plum_log_level_t = PLUM_LOG_LEVEL_NONE,
-    discoverTimeout: int = 0,
-    mappingTimeout: int = 0,
-    recheckPeriod: int = 0,
-): Result[void, string] {.raises: [].} =
+    logLevel: PlumLogLevel = PlumLogLevel.None,
+    discoverTimeout: int32 = 0,
+    mappingTimeout: int32 = 0,
+    recheckPeriod: int32 = 0,
+): Result[void, string] =
   ## init MUST be called to setup internal plum thread (plum_init).
 
   var config = plum_config_t(
-    log_level: logLevel,
+    log_level: plum_log_level_t(logLevel.int),
     log_callback: nil,
     dummytls_domain: nil,
     discover_timeout: discoverTimeout.cint,
@@ -170,7 +187,7 @@ proc init*(
   else:
     err("plum_init failed: " & $res)
 
-proc cleanup*(): Result[void, string] {.raises: [].} =
+proc cleanup*(): Result[void, string] =
   ## cleanup MUST be called to stop the thread and clean the setup.
 
   let res = plum_cleanup()
@@ -198,7 +215,6 @@ proc createMapping*(
     user_ptr: cast[pointer](handle),
   )
 
-  # Avoid issue with refc.
   # Pin the handle to prevent GC: the C library holds a raw pointer to
   # it (user_ptr) and might use it until DESTROYED fires.
   GC_ref(handle)
@@ -221,40 +237,23 @@ proc createMapping*(
     raise
   finally:
     if not completed:
-      # Timeout or cancellation: we cannot close the signal here because
-      # the C callback may fire later and call fireSync on it.
-      # Mark the handle as abandoned so the DESTROYED callback closes it.
-      # Access via activeMappings rather than the local `handle` ref,
-      # in order to make sure we have the valid reference.
-      withSafeLock:
-        let h = activeMappings.getOrDefault(id)
-        if not h.isNil:
-          h.abandoned.store(true)
+      # Timeout or cancellation: a late callback may still fireSync, so the
+      # mapping is torn down and the close is decided by releaseSignal.
       discard plum_destroy_mapping(id)
-    else:
-      # Signal fired normally, safe to close.
-      discard signal.close()
+    handle.releaseSignal()
 
-  # Reached only when completed = true (CancelledError skips this).
+  # Timeout path: the signal never fired within the deadline.
   if not completed:
     return err("plum: mapping " & $id & " timed out")
 
-  # Read result via activeMappings rather than the local `handle` ref in
-  # order to make sure we have the valid reference.
-  var resolvedState: PlumState
-  var resolvedMapping: PlumMapping
-
-  withSafeLock:
-    let h = activeMappings.getOrDefault(id)
-    if not h.isNil:
-      resolvedState = h.resolvedState
-      resolvedMapping = PlumMapping(
-        protocol: h.resolvedProtocol,
-        mappingProtocol: h.resolvedMappingProtocol,
-        internalPort: h.resolvedInternalPort,
-        externalPort: h.resolvedExternalPort,
-        externalHost: $cast[cstring](unsafeAddr h.resolvedExternalHost),
-      )
+  let resolvedState = handle.resolvedState
+  let resolvedMapping = PlumMapping(
+    protocol: handle.resolvedProtocol,
+    mappingProtocol: handle.resolvedMappingProtocol,
+    internalPort: handle.resolvedInternalPort,
+    externalPort: handle.resolvedExternalPort,
+    externalHost: $cast[cstring](unsafeAddr handle.resolvedExternalHost),
+  )
 
   if resolvedState == Success:
     return ok(MappingResult(id: id, mapping: resolvedMapping))
@@ -262,16 +261,29 @@ proc createMapping*(
     discard plum_destroy_mapping(id)
     return err("plum: mapping " & $id & " failed")
 
-proc destroyMapping*(id: cint) {.raises: [].} =
+proc destroyMapping*(id: cint) =
   ## Must be called exactly once after a successful createMapping.
+  ## Safe to call again or on an unknown id
+  withSafeLock:
+    if id notin activeMappings:
+      return
   discard plum_destroy_mapping(id)
 
-proc hasMapping*(id: cint): bool {.raises: [].} =
-  ## Returns true if the mapping exists and has not been destroyed yet.
-  withSafeLock:
-    result = id in activeMappings
+proc hasMapping*(id: cint): bool =
+  ## Returns true if the mapping exists and is not being destroyed.
+  var st: plum_state_t
+  if plum_query_mapping(id, addr st, nil) == PLUM_ERR_SUCCESS:
+    PlumState(st.int) notin {Destroying, Destroyed}
+  else:
+    false
 
-proc getLocalAddress*(): Result[string, string] {.raises: [].} =
+proc activeMappingCount*(): int =
+  ## Number of mappings the wrapper still tracks. Drops to 0 once every
+  ## mapping has fired DESTROYED. Mainly useful to detect handle leaks.
+  withSafeLock:
+    result = activeMappings.len
+
+proc getLocalAddress*(): Result[string, string] =
   var buf = newString(PLUM_MAX_ADDRESS_LEN)
   let res = plum_get_local_address(buf.cstring, buf.len.csize_t)
   if res >= 0:
@@ -279,3 +291,5 @@ proc getLocalAddress*(): Result[string, string] {.raises: [].} =
     ok(buf)
   else:
     err("plum_get_local_address failed: " & $res)
+
+{.pop.}

tests/test_plum.nim

@@ -16,6 +16,8 @@ import chronos
 import libplum/plum
 import libplum/libplum
 
+const miniupnp_protocol {.strdefine.} = ""
+
 suite "plum":
   test "init and cleanup":
     let r = init()
@@ -39,7 +41,37 @@ suite "plum":
   test "hasMapping returns false for unknown id":
     check not hasMapping(999)
 
-const miniupnp_protocol {.strdefine.} = ""
+  # Only valid where no NAT device answers; the integration container runs
+  # miniupnpd, which would make the mapping succeed before the timeout.
+  when miniupnp_protocol == "":
+    test "createMapping times out without a NAT device":
+      require init().isOk()
+      defer:
+        discard cleanup()
+
+      let r = waitFor createMapping(TCP, 8101, timeout = milliseconds(50))
+      check r.isErr()
+
+    test "cleanup while a createMapping is pending completes cleanly":
+      require init().isOk()
+      defer:
+        discard cleanup()
+
+      # No NAT device answers, so the mapping stays PENDING until we cleanup.
+      let fut = createMapping(TCP, 8501, timeout = milliseconds(200))
+      waitFor sleepAsync(50.milliseconds)
+      discard cleanup()
+      let r = waitFor fut
+      check r.isErr()
+      check activeMappingCount() == 0
+
+  test "destroyMapping is a no-op on an unknown id":
+    require init().isOk()
+    defer:
+      discard cleanup()
+
+    destroyMapping(999.cint)
+    check not hasMapping(999)
 
 # The flag is passed by the Docker / Podman container.
 when miniupnp_protocol != "":
@@ -54,7 +86,7 @@ when miniupnp_protocol != "":
   let mappingTimeout = seconds(40)
 
   let logLevel =
-    if getEnv("LIBPLUM_VERBOSE") == "1": PLUM_LOG_LEVEL_VERBOSE else: PLUM_LOG_LEVEL_NONE
+    if getEnv("LIBPLUM_VERBOSE") == "1": PlumLogLevel.Verbose else: PlumLogLevel.None
 
   var gRenewed: Atomic[bool]
 
@@ -89,15 +121,13 @@ when miniupnp_protocol != "":
 
   suite "plum - " & miniupnp_protocol & " using miniupnp":
     test "createMapping TCP and destroyMapping":
-      require init(discoverTimeout = discoverMs, logLevel = logLevel).isOk()
+      require init(discoverTimeout = discoverMs.int32, logLevel = logLevel).isOk()
       defer:
         discard cleanup()
 
       let r = waitFor createMapping(TCP, 8101, timeout = mappingTimeout)
       require r.isOk()
       let res = r.value
-      defer:
-        destroyMapping(res.id)
 
       checkpoint miniupnp_protocol & " TCP: " & res.mapping.externalHost & ":" &
         $res.mapping.externalPort
@@ -112,8 +142,13 @@ when miniupnp_protocol != "":
       else:
         check res.mapping.mappingProtocol == PCP
 
+      destroyMapping(res.id)
+      check not hasMapping(res.id)
+      # second destroy on a real id must be a safe no-op
+      destroyMapping(res.id)
+
     test "createMapping UDP and destroying":
-      require init(discoverTimeout = discoverMs, logLevel = logLevel).isOk()
+      require init(discoverTimeout = discoverMs.int32, logLevel = logLevel).isOk()
       defer:
         discard cleanup()
 
@@ -137,7 +172,9 @@ when miniupnp_protocol != "":
 
     test "mapping is renewed after miniupnpd restart":
       require init(
-        discoverTimeout = discoverMs, logLevel = logLevel, recheckPeriod = recheckMs
+        discoverTimeout = discoverMs.int32,
+        logLevel = logLevel,
+        recheckPeriod = recheckMs.int32,
       )
         .isOk()
       defer:
@@ -165,3 +202,13 @@ when miniupnp_protocol != "":
       startMiniupnpd(miniupnp_protocol)
 
       check waitRenewal()
+
+    test "cleanup releases active mappings":
+      require init(discoverTimeout = discoverMs.int32, logLevel = logLevel).isOk()
+
+      let r = waitFor createMapping(TCP, 8401, timeout = mappingTimeout)
+      require r.isOk()
+
+      # no destroyMapping on purpose: cleanup must release everything
+      check cleanup().isOk()
+      check activeMappingCount() == 0

vendor/libplum

@@ -1 +1 @@
-Subproject commit b74757f88c2a98ea905f5b81ad040232bc019c35
+Subproject commit b5d5f7d31b319b21136ec861d59d95f02eaf207a