diff --git a/CLAUDE.md b/CLAUDE.md index 9e77536..3eb0c59 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -33,7 +33,8 @@ Debug env vars: `LIBPLUM_VERBOSE=1`, `TEST_VERBOSE=1`, `MINIUPNPD_VERBOSE=1`. from the *calling* thread's dispatcher - `MappingHandle` is pinned with `GC_ref` while libplum holds `user_ptr`; unpinned only in the DESTROYED callback -- `activeMappings` is guarded by `activeMappingsLock` (`withSafeLock`) +- `activeMappings` is an `Atomic[int]` counting pinned handles; libplum is the source of truth for mapping state + (`hasMapping`) - The wrapper relies on a libplum guarantee: DESTROYED fires exactly once for every created mapping (explicit destroy, or `destroy_all_mappings` during `plum_cleanup`, synchronously before it returns), verified in diff --git a/libplum/plum.nim b/libplum/plum.nim index 6711a93..ca14cf7 100644 --- a/libplum/plum.nim +++ b/libplum/plum.nim @@ -6,7 +6,7 @@ # This file may not be copied, modified, or distributed except according to # those terms. -import std/[atomics, locks, tables] +import std/atomics import chronos import chronos/threadsync import results @@ -101,17 +101,9 @@ template foreignThreadGc(body: untyped) = when declared(tearDownForeignThreadGc): tearDownForeignThreadGc() -var activeMappingsLock: Lock -var activeMappings {.guard: activeMappingsLock.}: Table[cint, MappingHandle] - -initLock(activeMappingsLock) - -# We can be confident that the pattern is GC Safe using -# a lock. -template withSafeLock(body: untyped) = - {.cast(gcsafe).}: - withLock activeMappingsLock: - body +# Keep a counter for convenience for testing +# and exposed as an API over activeMappingCount() +var activeMappings: Atomic[int] 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. @@ -123,8 +115,7 @@ proc mappingCallback(id: cint, state: plum_state_t, raw: ptr plum_mapping_t) {.c let plumState = PlumState(state.int) if plumState == Destroyed: - withSafeLock: - activeMappings.del(id) + discard activeMappings.fetchSub(1) if not handle.resolved.exchange(true): # Set Destroyed state and fire the signal to notify any waiting threads. @@ -235,8 +226,7 @@ proc createMapping*( discard signal.close() return err("plum_create_mapping failed: " & $id) - withSafeLock: - activeMappings[id] = handle + discard activeMappings.fetchAdd(1) var completed = false try: @@ -290,9 +280,8 @@ proc hasMapping*(id: cint): bool = 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 + ## mapping has fired DESTROYED. + activeMappings.load() proc getLocalAddress*(): Result[string, string] = var buf = newString(PLUM_MAX_ADDRESS_LEN) diff --git a/tests/test_plum.nim b/tests/test_plum.nim index fc8efd3..7ab7f13 100644 --- a/tests/test_plum.nim +++ b/tests/test_plum.nim @@ -70,7 +70,6 @@ suite "plum": discard cleanup() destroyMapping(999.cint) - check not hasMapping(999) # The flag is passed by the Docker / Podman container. when miniupnp_protocol != "":