From e6afcf2f963139b1cf08eba8d19b80917ab1b29a Mon Sep 17 00:00:00 2001 From: Arnaud Date: Fri, 5 Jun 2026 10:46:48 +0400 Subject: [PATCH] Close the mapping signal after its single guaranteed fire --- CLAUDE.md | 12 +++++++++--- README.md | 8 ++++++++ libplum/plum.nim | 44 ++++++++++++++++++++++++++++---------------- 3 files changed, 45 insertions(+), 19 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 3912a64..9e77536 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -26,9 +26,15 @@ Debug env vars: `LIBPLUM_VERBOSE=1`, `TEST_VERBOSE=1`, `MINIUPNPD_VERBOSE=1`. - `mappingCallback` runs on libplum's internal C thread, wrapped in `foreignThreadGc`; it must stay `raises: []` and only touch thread-safe state -- Never call `ThreadSignalPtr.close()` from the C thread: close() unregisters - the fd from the *calling* thread's dispatcher; only the chronos loop thread - may close a signal +- The mapping signal fires exactly once (first of SUCCESS/FAILURE/DESTROYED + via `resolved.exchange`); `createMapping` owns it and closes it on the + chronos loop thread after consuming that fire. Never call + `ThreadSignalPtr.close()` from the C thread: close() unregisters the fd + 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`) +- 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 + `vendor/libplum/src/client.c`. Re-verify this when bumping the submodule diff --git a/README.md b/README.md index 511cfeb..ae2ebf5 100644 --- a/README.md +++ b/README.md @@ -121,6 +121,14 @@ Format the code with [nph](https://github.com/arnetheduck/nph): nimble format ``` +### Updating the libplum submodule + +The async wrapper relies on a libplum behavior verified in `vendor/libplum/src/client.c`: +the DESTROYED callback fires exactly once for every created mapping, either on explicit +destroy or during `plum_cleanup` (synchronously, before it returns). `createMapping` +waits for this callback to safely reclaim its internal signal. When bumping the +submodule, re-check that this still holds. + ## License Licensed and distributed under either of diff --git a/libplum/plum.nim b/libplum/plum.nim index d1d8228..2549561 100644 --- a/libplum/plum.nim +++ b/libplum/plum.nim @@ -125,9 +125,12 @@ proc mappingCallback(id: cint, state: plum_state_t, raw: ptr plum_mapping_t) {.c if plumState == Destroyed: withSafeLock: activeMappings.del(id) - # Do not close the signal here: close() unregisters the fd from the - # calling thread's dispatcher, and this is libplum's C thread, not the - # chronos loop. createMapping decides what happens to the signal. + + if not handle.resolved.exchange(true): + # Set Destroyed state and fire the signal to notify any waiting threads. + handle.resolvedState = Destroyed + discard handle.signal.fireSync() + # 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) @@ -192,6 +195,17 @@ proc cleanup*(): Result[void, string] = else: err("plum_cleanup failed: " & $res) +const destroyConfirmTimeout = seconds(5) + +proc destroyAndReclaim(id: cint, signal: ThreadSignalPtr) {.async: (raises: []).} = + ## Tear the mapping down and close its signal once DESTROYED confirms + ## (the signal fires exactly once, see mappingCallback). If libplum never + ## confirms in time, leak the signal rather than closing it while the C + ## thread may still fire it. + discard plum_destroy_mapping(id) + if await noCancel withTimeout(signal.wait(), destroyConfirmTimeout): + discard signal.close() + proc createMapping*( protocol: PlumProtocol, internalPort: uint16, @@ -226,26 +240,22 @@ proc createMapping*( var completed = false try: - # Wait for the callback to fireSync + # Wait for the signal's single fire (in mappingCallback) completed = await withTimeout(signal.wait(), timeout) except CancelledError: - # CancelledError skips the lines below and propagates after finally. + # The signal was cancelled so it is safe to destroy and reclaim. + await destroyAndReclaim(id, signal) raise - finally: - if completed: - # The signal fired (once, see mappingCallback) and was consumed: - # nobody will touch it again, safe to close here on the loop thread. - discard signal.close() - else: - # Timeout or cancellation: a late callback may still fireSync, so - # closing the signal is unsafe (here or from the C thread). We - # deliberately leak one eventfd instead. - discard plum_destroy_mapping(id) - # Timeout path: the signal never fired within the deadline. if not completed: + # The signal was not fired so it is safe to destroy and reclaim. + await destroyAndReclaim(id, signal) return err("plum: mapping " & $id & " timed out") + # The signal fired and was consumed: nobody will touch it again, + # safe to close here. + discard signal.close() + let resolvedState = handle.resolvedState let resolvedMapping = PlumMapping( protocol: handle.resolvedProtocol, @@ -257,6 +267,8 @@ proc createMapping*( if resolvedState == Success: return ok(MappingResult(id: id, mapping: resolvedMapping)) + elif resolvedState == Destroyed: + return err("plum: mapping " & $id & " destroyed before completion") else: discard plum_destroy_mapping(id) return err("plum: mapping " & $id & " failed")