Update master from release v0.1#79
Closed
Ivansete-status wants to merge 10 commits into
Closed
Conversation
A nim-ffi library is loaded into a foreign host (Go/Rust/...) that must own OS signal handling; if the Nim runtime installs its own handlers it clobbers the host's (e.g. Go's SIGSEGV -> sigpanic recovery, stack growth, goroutine preemption), turning recoverable faults into hard process crashes. The flag can only be set on the consumer's final build command, so a dependency cannot inject it -- but it CAN refuse to compile without it. Add a compile-time guard so any consumer that omits -d:noSignalHandler fails the build with an actionable message instead of crashing at runtime (the cause of a real status-go regression). Standalone Nim binaries (nim-ffi's own tests) build with -d:ffiAllowSignalHandler. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Ivansete-status
force-pushed
the
update-master-from-release-v0.1
branch
from
June 11, 2026 14:57
d4c87c1 to
e0d1ba3
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
- Call initializeLibrary() (setupForeignThreadGc) in the `.ffi.` request wrapper and in add/remove_event_listener so a foreign (Go) caller thread has an initialised Nim heap before any allocation ($reqTypeName / $eventName / registry ops). Without it such a thread segfaults in the allocator under GC pressure — the production unwrap SIGSEGV. - recycleContext resets the event registry/queue + stuck flag on park so a reused pool slot starts clean. - ffiDtor doc/cleanup for the async recycle ABI. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The noSignalHandler compile-time guard rejects any build of the ffi modules that doesn't set -d:noSignalHandler (embedded) or -d:ffiAllowSignalHandler (standalone). test.yml compiled the unit tests with a raw `nim c` missing the flag, so every per-test job failed at the guard. The nimble test tasks already pass it; mirror that here. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The recycle/async-destroy work changed the Nim `ffiDtor` export from `int destroy(ctx)` to `int destroy(ctx, callback, userData)`, but the C++ and Rust generators still emitted the 1-arg signature. Foreign callers therefore passed only `ctx`; inside Nim, `callback`/`userData` held uninitialised register garbage. `requestRecycle` stored the garbage callback and the recycle handler later invoked it — a jump through a wild pointer that segfaulted in every C++ E2E / ASan / TSan job (the crash surfaced at teardown, after each test's assertions had already passed). Generate the 3-arg ABI and have the destructor/Drop block on the recycle callback via the existing sync-call helper, so the pool slot is fully drained and parked before the handle goes away — otherwise rapid create/destroy churn (StressShortLivedPerThreadContext, ThreadedHammer) could outrun the recycle and exhaust the pool. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
destroyFFIContext stopped/joined the worker threads and marked the slot for rebuild, but no longer deinited the context — so the six ThreadSignalPtr fds were orphaned every full teardown (the exact leak this path exists to prevent), and the still-initialised Lock + event registry/queue locks were left live. createFFIContext's rebuild path (initialized == false) reruns initContextResources, which calls initLock / initEventRegistry / initEventQueue and installs fresh signals over the stale handles — re-initialising a live lock is UB. Restore the deinitContextResources() call (as the pre-recycle code did) before marking the slot uninitialised so the rebuild starts from clean state. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The branch carried a dated `[0.2.0] - 2026-06-04` release section and `version = "0.2.0"` while the active release line is still 0.1.x, which misrepresents unshipped work as a released version. Treat 0.1.4 as the latest release and move everything above it back under `[Unreleased]`, merging the 0.2.0 bullets into the existing Added/Changed/Fixed groups. Set the package version to 0.1.4 and realign the example `requires` (>= 0.2.0 -> >= 0.1.4) so they stay satisfiable. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Collaborator
Author
|
Sorry for the spam. I will ping again once this is really ready for review 🙏 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is to introduce the enhancements implemented in
release/v0.1branch so that we don't miss them for the future.