feat(ffi): pin nim-ffi 0.3.0 and stop the node on destroy - #4111
feat(ffi): pin nim-ffi 0.3.0 and stop the node on destroy#4111gmelodie wants to merge 6 commits into
Conversation
|
You can find the images built from this PR at Built from 5a3da84 |
afad5bd to
019a5cc
Compare
|
|
||
| (await self.start()).isOkOr: | ||
| ## A retry would stack a second set. | ||
| await self.dropFFIEventListeners() |
There was a problem hiding this comment.
dropFFIEventListeners nukes every event listener on the shared ctx using dropAllListeners, which includes the node's internal ones which are not re-registered when we retry failed starts or try a stop-then-start.
This PR is actually LGTM. It's just stepping on a pre-existing landmine/bug. I will fix the listener registration / lifetime bugs as my next task, if you want to wait for that before merging this PR (merging it as-is is also kind of fine because stop and restart is already broken and I'll have to fix it anyway, so this breaks it just a bit more in the unhappy path).
There was a problem hiding this comment.
I think any listener must be valid after successful start....
But I have concerns with the stop too, please not merge it yet.
|
@fcecin @NagyZoltanPeter ready for reviewing! |
Description
nim-ffi 0.3.0 runs the
{.ffiDtor.}body on the context recycle path (logos-messaging/nim-ffi#147). That is the first of the two pieces #4108 asks for. This PR pins that release and adds the second piece:logosdelivery_destroynow stops the node.Until now a host that called
logosdelivery_destroywithoutlogosdelivery_stop_nodefirst got a node it could no longer reach but that kept running. nim-ffi recycles the worker thread instead of joining it, so the libp2p switch, the discv5 loop, the REST server and the persistency storage threads survived the destroy for the rest of the process lifetime, while the recycle cleared the event registry under them. The dtor wasdiscard, and the pinned 0.3.0-rc.2 never ran it anyway.The bump also makes
{.ffiExport.}reachable.logosdelivery_version()uses it: the call takes no context, so a host can read the version of a.sobefore it builds a node.Changes
logos_delivery.nimblenow requiresffi == 0.3.0by package name. nim-ffi is on the official nimble package list, so the entry moves into the mainrequiresblock and leaves the git-URL block. Thev0.3.0tag points at commitb6c17dc8, sonimble.lockandnix/deps.nixkeep that revision and its hashes.logosdelivery_destroyandlogosdelivery_stop_nodenow share one teardown,stopNode, which drops the FFI event listeners and then stops the node whileLogosDelivery.isRunningsays one runs.logosdelivery_stop_nodeis therefore idempotent: a second call reportsRET_OKand does nothing.LogosDelivery.isRunningis new inlogos_delivery/logos_delivery.nim, next tostartandstop. It answers for the whole stack, so the FFI layer no longer readswaku.node.startedthrough three types.logosdelivery_start_nodedrops the listeners it registered whenstartfails. A host that retried a failed start used to stack a second full set of broker listeners, and every event then fanned out twice.logosdelivery_version()is a new C entry point inlibrary/logos_delivery_api/sync_exports.nim. nim-ffi does not emit{.ffiExport.}procs into the generatedabi = cheader, so it is declared by hand inlibrary/liblogosdelivery.hnext to the event-listener ABI.tests/ffi/test_ffi_persistency_lifecycle.nimgains adestroy-stops-nodecase: ctx1 binds a TCP and a discv5 port, is destroyed without a stop, and ctx2 must bind the same two ports. ctx1 had to bind them first, so a failure of the second bind means ctx1 never released them. The case also callslogosdelivery_versionbefore any context exists. The same file hadlogosdelivery_destroytyped with a callback it lost in #4082, so every destroy step waited 60 s for a callback that never fires; it now uses the realint(void*)signature.0.3.0 also rejects a call against a
reflibrary that no constructor ever stored, instead of reading the fields of a nil library.LogosDeliveryis aref object, andlogosdelivery_create_nodehands the host a live context before the constructor runs, so the host holds one even when the constructor fails. The casecall-after-failed-ctorcreates a context from an invalid config, callslogosdelivery_start_nodeon it, and expects an error that carriesnot initialized.logosdelivery_destroystill releases that context, because nim-ffi gates the{.ffiDtor.}body on a stored library.library/README.mdstates the behaviour underlogosdelivery_create_node.library/README.mdrecords the two limits of the destroy path. The recycle handler drops what the hook returns, so a failed stop is logged and the host still readsRET_OK. nim-ffi cancels the stop atffiTeardownTimeoutMs(10 s) and frees the library anyway, which leaves the node half stopped.logosdelivery_stop_nodestays the call that runs to completion and reports its result.Impact on hosts: no C signature changes.
logosdelivery_destroynow blocks its caller for as long as the stop takes, bounded by2 * ffiRecycleTimeoutMs + ffiTeardownTimeoutMs + 2 s, which is 15 s at the nim-ffi defaults. A host that already callslogosdelivery_stop_nodefirst sees no change, because the guard makes the destroy a no-op.Dependency provenance: the
v0.3.0tag is an annotated tag on commitb6c17dc8. GitHub reports it as verified, and it carries the same signing key as thev0.3.0-rc.2tag that master pins. The rc.2 to 0.3.0 delta adds norequiresentry, so there is no new transitive dependency. The nimble and nix hashes were recomputed with a script that reproduces the checked-in rc.2 values byte for byte, and nimble itself resolves the package intonimbledeps/pkgs2/ffi-0.3.0-74e796df3ef39d828e014df701127edfbee459e0, which matches thenimble.lockchecksum.Validation:
nim checkpasses onlibrary/liblogosdelivery.nimand on the test,nimble dumpaccepts the new requirement, andnphis clean. The new e2e cases are not run yet, because they need the built shared library.Issue
closes #4108