diff --git a/library/README.md b/library/README.md index ec08581eb7..754d78d9a1 100644 --- a/library/README.md +++ b/library/README.md @@ -69,6 +69,11 @@ void *logosdelivery_create_node( **Returns:** the context handle, or `NULL` on failure. Creation is asynchronous: wait for `onCreated` before you make any other call. +A context whose `onCreated` reported `RET_ERR` stays live but holds no node. +Every later call on it answers `RET_ERR` with `library is not initialized: the +constructor failed or has not run yet`, and `logosdelivery_destroy` still +releases it. + **Example configuration JSON:** ```json { @@ -108,7 +113,8 @@ int logosdelivery_start_node(void *ctx, LogosDeliveryScalarRawFn callback, void ``` #### `logosdelivery_stop_node` -Stops the node. +Stops the node and removes the event listeners. A second call is a no-op that +reports `RET_OK`. ```c int logosdelivery_stop_node(void *ctx, LogosDeliveryScalarRawFn callback, void *userData); @@ -122,6 +128,30 @@ use `ctx` afterwards. int logosdelivery_destroy(void *ctx); ``` +Stops the node first if it still runs, so skipping `logosdelivery_stop_node` no +longer leaves a live node behind. It blocks for up to 15 s at the nim-ffi +defaults (`2 * ffiRecycleTimeoutMs + ffiTeardownTimeoutMs + 2 s`). + +Prefer an explicit `logosdelivery_stop_node`: a failed stop here is only logged +(`RET_ERR` covers an invalid `ctx` and a failed context teardown, nothing else), +and nim-ffi cancels the stop at `ffiTeardownTimeoutMs` (10 s), leaving the node +half stopped. + +### Context-free calls + +No `ctx` and no callback: `dlsym` the symbol and read the return value. + +#### `logosdelivery_version` +Version and git commit hash. Callable before `logosdelivery_create_node`, though +the first call into the library starts the Nim runtime. + +```c +const char *logosdelivery_version(void); +``` + +The buffer belongs to the calling thread and stays valid until that thread calls +`logosdelivery_version` again, so copy the bytes. + ### Messaging #### `logosdelivery_subscribe` diff --git a/library/liblogosdelivery.h b/library/liblogosdelivery.h index d701725290..11973602ae 100644 --- a/library/liblogosdelivery.h +++ b/library/liblogosdelivery.h @@ -3,8 +3,8 @@ // The call surface is generated from the {.ffi.} annotations in library/*.nim // and written to generated/logosdelivery.h by `make liblogosdelivery`. That file // is a build artifact, not checked in, so build the library before you compile -// against this header. This file adds the event-listener ABI, which nim-ffi -// exports from declareLibrary but does not emit into the `abi = c` header. +// against this header. This file adds what nim-ffi exports but leaves out of the +// `abi = c` header: the event-listener ABI, and the synchronous exports. #pragma once #ifndef __liblogosdelivery__ #define __liblogosdelivery__ @@ -32,6 +32,10 @@ extern "C" { #endif + // Version and git commit hash. Needs no ctx. The buffer belongs to the calling + // thread and lasts until that thread calls this again, so copy it. + const char *logosdelivery_version(void); + // Raw result-delivery callback used by the event API. `msg` is a byte run of // `len` bytes, not NUL-terminated, and is valid only for the duration of the // call. diff --git a/library/liblogosdelivery.nim b/library/liblogosdelivery.nim index 853311aef1..811135d598 100644 --- a/library/liblogosdelivery.nim +++ b/library/liblogosdelivery.nim @@ -18,6 +18,7 @@ include ./logos_delivery_api/node_api, ./logos_delivery_api/messaging_api, ./logos_delivery_api/debug_api, + ./logos_delivery_api/sync_exports, ./kernel_api/peer_manager_api, ./kernel_api/discovery_api, ./kernel_api/debug_node_api, diff --git a/library/logos_delivery_api/node_api.nim b/library/logos_delivery_api/node_api.nim index 553fc1f788..8689f03533 100644 --- a/library/logos_delivery_api/node_api.nim +++ b/library/logos_delivery_api/node_api.nim @@ -179,13 +179,6 @@ proc logosdelivery_create_node( return ok(lib) -proc logosdelivery_destroy(self: LogosDelivery) {.ffiDtor.} = - ## nim-ffi runs this when the host destroys the node, before the FFI - ## thread can serve a later create. The forwarders registered at create - ## feed the C listener registry, which lives from create to destroy; the - ## node's broker scope carries them and dies with the node, here. - await self.teardownFFIEventScope() - proc logosdelivery_start_node( self: LogosDelivery ): Future[Result[string, string]] {.ffi.} = @@ -195,11 +188,26 @@ proc logosdelivery_start_node( return err("failed to start: " & errMsg) return ok("") +proc stopNode(self: LogosDelivery): Future[Result[void, string]] {.async.} = + if not self.isRunning(): + return ok() + + await self.stop() + proc logosdelivery_stop_node( self: LogosDelivery ): Future[Result[string, string]] {.ffi.} = - (await self.stop()).isOkOr: + (await self.stopNode()).isOkOr: let errMsg = $error chronicles.error "STOP_NODE failed", err = errMsg return err("failed to stop: " & errMsg) return ok("") + +proc logosdelivery_destroy(self: LogosDelivery) {.ffiDtor.} = + ## Safety net for a host that skips `stop_node` (#4108): nim-ffi recycles the + ## worker rather than joining it, so an unstopped node keeps running. + ## The forwarders registered at create live until here, with the node's + ## broker scope; `teardownFFIEventScope` is the other end of create. + (await self.stopNode()).isOkOr: + chronicles.error "DESTROY failed", err = error + await self.teardownFFIEventScope() diff --git a/library/logos_delivery_api/sync_exports.nim b/library/logos_delivery_api/sync_exports.nim new file mode 100644 index 0000000000..f6c496ea54 --- /dev/null +++ b/library/logos_delivery_api/sync_exports.nim @@ -0,0 +1,7 @@ +## Synchronous entry points: no context, no callback, the value crosses the +## C ABI directly. `{.ffi.}` routes a no-argument proc with a plain return type +## here on its own. + +proc logosdelivery_version(): string {.ffi.} = + ## Same string `waku_version` answers over the context surface. + WakuNodeVersionString diff --git a/logos_delivery.nimble b/logos_delivery.nimble index cd216f6887..62cc75d360 100644 --- a/logos_delivery.nimble +++ b/logos_delivery.nimble @@ -57,12 +57,12 @@ requires "nim >= 2.2.4", "zlib", # Debug & Testing "testutils", - "unittest2" + "unittest2", + # FFI + "ffi == 0.3.0" # Packages not on nimble (use git URLs) -requires "https://github.com/logos-messaging/nim-ffi#53515de17af0ef3e88b2aec9675b8163dddc14ae" # v0.3.0-rc.2 - requires "https://github.com/logos-messaging/nim-sds.git#b12f5ee07c5b764303b51fb948b32a4ade1de3b5" requires "https://github.com/NagyZoltanPeter/nim-brokers.git#v3.3.0" diff --git a/logos_delivery/logos_delivery.nim b/logos_delivery/logos_delivery.nim index 55867e5827..0e5767ed38 100644 --- a/logos_delivery/logos_delivery.nim +++ b/logos_delivery/logos_delivery.nim @@ -216,6 +216,14 @@ proc stop*(self: LogosDelivery): Future[Result[void, string]] {.async.} = return ok() +func isRunning*(self: LogosDelivery): bool = + ## True while a layer still holds what `stop` releases; the channel manager + ## needs no test, its `stop` is already a no-op when empty. + let transportUp = + not self.waku.isNil() and not self.waku.node.isNil() and self.waku.node.started + let messagingUp = not self.messagingClient.isNil() and self.messagingClient.started + transportUp or messagingUp + proc isOnline*(self: LogosDelivery): Future[Result[bool, string]] {.async.} = if self.waku.isNil(): return err("Waku node is not initialized") diff --git a/nimble.lock b/nimble.lock index 8d924d507f..e9696130aa 100644 --- a/nimble.lock +++ b/nimble.lock @@ -644,7 +644,7 @@ }, "ffi": { "version": "0.3.0", - "vcsRevision": "53515de17af0ef3e88b2aec9675b8163dddc14ae", + "vcsRevision": "b6c17dc822960b626d76d814de90208c0a40a44e", "url": "https://github.com/logos-messaging/nim-ffi", "downloadMethod": "git", "dependencies": [ @@ -655,7 +655,7 @@ "cbor_serialization" ], "checksums": { - "sha1": "1d84ceaf8594f4970c5a37f916003ffc0531dc4e" + "sha1": "74e796df3ef39d828e014df701127edfbee459e0" } }, "boringssl": { diff --git a/nix/deps.nix b/nix/deps.nix index 858d4b58b3..5cac5f24c5 100644 --- a/nix/deps.nix +++ b/nix/deps.nix @@ -285,8 +285,8 @@ ffi = pkgs.fetchgit { url = "https://github.com/logos-messaging/nim-ffi"; - rev = "53515de17af0ef3e88b2aec9675b8163dddc14ae"; - sha256 = "0ncf9j7fhgd3nswr4rh19jx77dl974sajphdl04cb602hshgj5ij"; + rev = "b6c17dc822960b626d76d814de90208c0a40a44e"; + sha256 = "1sjnax54j39igsxkig095grj4x3j9div4fimrmz609byhp4qdavh"; fetchSubmodules = true; }; diff --git a/nix/submodules.json b/nix/submodules.json index c9ac2d8148..34a537a78c 100644 --- a/nix/submodules.json +++ b/nix/submodules.json @@ -56,7 +56,7 @@ { "path": "vendor/nim-ffi", "url": "https://github.com/logos-messaging/nim-ffi", - "rev": "53515de17af0ef3e88b2aec9675b8163dddc14ae" + "rev": "b6c17dc822960b626d76d814de90208c0a40a44e" } , { diff --git a/tests/ffi/test_ffi_persistency_lifecycle.nim b/tests/ffi/test_ffi_persistency_lifecycle.nim index 99528c2f16..eaaa72fa60 100644 --- a/tests/ffi/test_ffi_persistency_lifecycle.nim +++ b/tests/ffi/test_ffi_persistency_lifecycle.nim @@ -20,13 +20,19 @@ ## destroying ctx1 without stop must not corrupt ctx2's persistency. ## Historically a UB probe (stale global into a released heap -> ## SIGSEGV on Linux, zombie singleton on macOS/arm64); kept as a guard. -## The destroy-without-stop teardown gap itself is tracked separately -## (issue #4108). ## ## case different-storage-paths ## two contexts with different local-storage-paths must both start; ## the former singleton refused the second rootDir. ## +## case destroy-stops-node +## destroy without stop must tear the node down (issue #4108), and the +## context-free `logosdelivery_version` export must answer with no node. +## +## case call-after-failed-ctor +## a context whose constructor failed must reject a later call with an +## error instead of faulting on its nil library. +## ## Each case runs in a child process (the second one can fault) with its ## output captured to a file, so a crash is an exit code rather than a dead ## test binary. @@ -53,6 +59,8 @@ const CaseStopSteals = "--case-stop-steals-persistence" CaseDestroyOnly = "--case-destroy-without-stop" CaseTwoPaths = "--case-different-storage-paths" + CaseDestroyStops = "--case-destroy-stops-node" + CaseFailedCtor = "--case-call-after-failed-ctor" DisabledMarker = "SDS persistence disabled" ## Logged by `sdsPersistence()` when the singleton is unusable. @@ -79,6 +87,9 @@ type cdecl, gcsafe .} CtxFn = proc(ctx: pointer, cb: FfiCallback, userData: pointer): cint {.cdecl, gcsafe.} + DestroyFn = proc(ctx: pointer): cint {.cdecl, gcsafe.} + ## The `{.ffiDtor.}` export takes no callback: it blocks until teardown ends. + VersionFn = proc(): cstring {.cdecl, gcsafe.} ChannelCreateFn = proc( ctx: pointer, cb: FfiCallback, @@ -93,7 +104,8 @@ type createNode: CreateNodeFn startNode: CtxFn stopNode: CtxFn - destroy: CtxFn + destroy: DestroyFn + version: VersionFn channelCreate: ChannelCreateFn channelExists: ChannelExistsFn @@ -165,7 +177,8 @@ proc loadApi(): Api = createNode: cast[CreateNodeFn](lib.need("logosdelivery_create_node")), startNode: cast[CtxFn](lib.need("logosdelivery_start_node")), stopNode: cast[CtxFn](lib.need("logosdelivery_stop_node")), - destroy: cast[CtxFn](lib.need("logosdelivery_destroy")), + destroy: cast[DestroyFn](lib.need("logosdelivery_destroy")), + version: cast[VersionFn](lib.need("logosdelivery_version")), channelCreate: cast[ChannelCreateFn](lib.need("logosdelivery_channel_create")), channelExists: cast[ChannelExistsFn](lib.need("logosdelivery_channel_exists")), ) @@ -183,6 +196,14 @@ proc expectOk(step: string, r: tuple[ok: bool, ret: int, msg: string]) = echo " FAIL: ", step, " expected RET_OK" failed = true +proc expectErr( + step: string, r: tuple[ok: bool, ret: int, msg: string], needle: string +) = + note(step, r) + if not r.ok or r.ret == RetOk or not r.msg.contains(needle): + echo " FAIL: ", step, " expected an error carrying '", needle, "'" + failed = true + proc nodeConfig(storagePath: string, tcpPort, discv5Port: int): string = $( %*{ @@ -215,6 +236,19 @@ proc call(api: Api, s: ptr Slot, label: string, fn: CtxFn, ctx: pointer) = discard fn(ctx, onDone, s) expectOk(label, awaitSlot(s)) +proc destroyCtx(api: Api, label: string, ctx: pointer) = + ## `destroy` answers with its return code, not through the slot. + expectOk(label, (ok: true, ret: int(api.destroy(ctx)), msg: "")) + +proc expectVersion(api: Api) = + ## Synchronous exports never reach the generated header, so nothing but + ## `loadApi` and this call checks that one is still exported. + let version = $api.version() + echo " [ctx-free logosdelivery_version] ", version + if not version.startsWith("version / git commit hash:"): + echo " FAIL: logosdelivery_version returned an unexpected string" + failed = true + proc createChannel(api: Api, s: ptr Slot, label: string, ctx: pointer, id: string) = armSlot(s) discard api.channelCreate( @@ -253,16 +287,16 @@ proc runStopSteals(api: Api, s: ptr Slot) = api.createChannel(s, "ctx2 channel_create (before ctx1 stop)", ctx2, "before") api.call(s, "ctx1 stop_node", api.stopNode, ctx1) - api.call(s, "ctx1 destroy", api.destroy, ctx1) + api.destroyCtx("ctx1 destroy", ctx1) api.createChannel(s, "ctx2 channel_create (after ctx1 stop)", ctx2, "after") api.call(s, "ctx2 stop_node", api.stopNode, ctx2) - api.call(s, "ctx2 destroy", api.destroy, ctx2) + api.destroyCtx("ctx2 destroy", ctx2) proc runDestroyOnly(api: Api, s: ptr Slot) = - ## Same, but ctx1 is destroyed without stop_node -- reset() never runs, so - ## the global keeps pointing into ctx1's released FFI-thread heap. + ## Same, but ctx1 is destroyed without stop_node. The dtor reaches reset() on + ## that path too now; the case guards the era when it did not. let root = caseRoot("shared") let ctx1 = createCtx(api, s, "ctx1", root, 60030, 60031) let ctx2 = createCtx(api, s, "ctx2", root, 60040, 60041) @@ -277,13 +311,13 @@ proc runDestroyOnly(api: Api, s: ptr Slot) = ## against released memory. api.createChannel(s, "ctx1 channel_create (opens the sds job)", ctx1, "owned-by-ctx1") - api.call(s, "ctx1 destroy (no stop_node)", api.destroy, ctx1) + api.destroyCtx("ctx1 destroy (no stop_node)", ctx1) api.churn(s, ctx2) api.createChannel(s, "ctx2 channel_create (after ctx1 destroy)", ctx2, "after") api.call(s, "ctx2 stop_node", api.stopNode, ctx2) - api.call(s, "ctx2 destroy", api.destroy, ctx2) + api.destroyCtx("ctx2 destroy", ctx2) proc runTwoPaths(api: Api, s: ptr Slot) = ## Two contexts, two storage paths. The singleton refuses to be re-targeted, @@ -299,8 +333,46 @@ proc runTwoPaths(api: Api, s: ptr Slot) = api.call(s, "ctx2 start_node (different local-storage-path)", api.startNode, ctx2) api.call(s, "ctx1 stop_node", api.stopNode, ctx1) - api.call(s, "ctx1 destroy", api.destroy, ctx1) - api.call(s, "ctx2 destroy", api.destroy, ctx2) + api.destroyCtx("ctx1 destroy", ctx1) + api.destroyCtx("ctx2 destroy", ctx2) + +proc runDestroyStopsNode(api: Api, s: ptr Slot) = + ## The ports are the observable: ctx1 bound them before the destroy, so a + ## failed second bind means ctx1 never let go, not that a stranger holds them. + api.expectVersion() + + let ctx1 = createCtx(api, s, "ctx1", caseRoot("dtor_a"), 60070, 60071) + if failed: + return + + api.call(s, "ctx1 start_node", api.startNode, ctx1) + api.destroyCtx("ctx1 destroy (no stop_node)", ctx1) + + let ctx2 = createCtx(api, s, "ctx2 (same ports)", caseRoot("dtor_b"), 60070, 60071) + if failed: + return + + api.call(s, "ctx2 start_node (ports must be free)", api.startNode, ctx2) + api.call(s, "ctx2 stop_node", api.stopNode, ctx2) + api.destroyCtx("ctx2 destroy", ctx2) + +proc runCallAfterFailedCtor(api: Api, s: ptr Slot) = + ## `create_node` hands back a live context before the constructor runs, so a + ## host holds one even when the constructor fails. `LogosDelivery` is a `ref`, + ## and a call against it used to read the fields of a nil library. + armSlot(s) + let ctx = api.createNode("{ not a config }".cstring, onDone, s) + if ctx.isNil(): + echo " FAIL: create_node returned nil, the case needs a live context" + failed = true + return + expectErr("ctx create_node (invalid config)", awaitSlot(s), "parseLogosDeliveryConf") + + armSlot(s) + discard api.startNode(ctx, onDone, s) + expectErr("ctx start_node (constructor failed)", awaitSlot(s), "not initialized") + + api.destroyCtx("ctx destroy (constructor failed)", ctx) proc runChild(which: string) = let api = loadApi() @@ -313,6 +385,10 @@ proc runChild(which: string) = runDestroyOnly(api, s) of CaseTwoPaths: runTwoPaths(api, s) + of CaseDestroyStops: + runDestroyStopsNode(api, s) + of CaseFailedCtor: + runCallAfterFailedCtor(api, s) else: quit("unknown case " & which, 2) @@ -387,3 +463,27 @@ suite "FFI - persistency lifecycle across library contexts": removeDir(caseRoot("path_b")) check r.code == 0 + + test "destroy without stop_node must stop the node and free its ports": + if not fileExists(libPath()): + echo "skipped: no ", libPath() + skip() + else: + removeDir(caseRoot("dtor_a")) + removeDir(caseRoot("dtor_b")) + let r = runCase(CaseDestroyStops) + report(CaseDestroyStops, r) + removeDir(caseRoot("dtor_a")) + removeDir(caseRoot("dtor_b")) + + check r.code == 0 + + test "a call against a context whose constructor failed must not fault": + if not fileExists(libPath()): + echo "skipped: no ", libPath() + skip() + else: + let r = runCase(CaseFailedCtor) + report(CaseFailedCtor, r) + + check r.code == 0 diff --git a/tests/waku_relay/test_wakunode_relay.nim b/tests/waku_relay/test_wakunode_relay.nim index dda3c65ae7..3e624f8626 100644 --- a/tests/waku_relay/test_wakunode_relay.nim +++ b/tests/waku_relay/test_wakunode_relay.nim @@ -605,6 +605,18 @@ suite "WakuNode - Relay": check: nodes[i].wakuRelay.peerStats[nodes[0].switch.peerInfo.peerId].score == -249999.9 + proc badPeerIsolated(): bool = + nodes[0].peerManager.switch.connManager.getConnections().len == 0 and + toSeq(1 ..< 5).allIt( + nodes[it].peerManager.switch.connManager.getConnections().len == 3 + ) + + ## The disconnect trails the score drop, and the delay grows on a loaded + ## runner, so poll instead of reading the counts once. + let deadline = Moment.now() + 30.seconds + while Moment.now() < deadline and not badPeerIsolated(): + await sleepAsync(500.millis) + # nodes[0] was blacklisted from all other peers, no connections check: nodes[0].peerManager.switch.connManager.getConnections().len == 0