Replace the activeMappings table with an atomic counter

This commit is contained in:
Arnaud 2026-06-05 11:29:27 +04:00
parent ab79787bcc
commit bdea6ef3b5
No known key found for this signature in database
GPG Key ID: A6C7C781817146FA
3 changed files with 10 additions and 21 deletions

View File

@ -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

View File

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

View File

@ -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 != "":