Fix lifecycle / teardown races in RawChannel - #170
Open
sebi2k1 wants to merge 1 commit into
Open
Conversation
Four small correctness fixes in native/can.cc: - ~RawChannel closed m_SocketFd BEFORE joining the reader thread, while the reader is mid-poll() on that fd. Closing an fd from one thread while another is in poll() on it is POSIX-undefined — the fd can be reused by an unrelated open() before poll() returns. Reorder: stopThread() first, then close(m_SocketFd). - async_channel_stopped() could run twice — once from JS Stop() and once from the reader-thread uv_async_send issued on POLLHUP/POLLERR. The second invocation would re-run the listener loop, schedule a double uv_close (libuv assertion), and call Unref() one too many times. Add an m_StoppedAlready single-shot guard. - pthread_create's return value was ignored. The subsequent CHECK_CONDITION(m_Thread, ...) checked the output pthread_t handle, which is an opaque type that may legitimately be zero on some implementations. Capture rc and check rc == 0 instead. - pthread_mutex_init / pthread_cond_init were paired with nothing in the destructor. Add pthread_*_destroy calls in ~RawChannel, gated by a new m_SyncInitialized flag (the sync primitives are only initialised on the successful constructor path, after a successful bind()). These are review-validated fixes; the races (and the Unref ordering) are hard or impossible to surface from a mocha test without thread- sanitizer instrumentation or kernel-level cooperation. No behavioural test is shipped — the Docker build-test passes 47/57 (same as master; the 10 failures are all pre-existing 'Error while creating channel' caused by missing vcan0/vcan1 in the build image). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
sebi2k1
force-pushed
the
fix/lifecycle-teardown
branch
from
June 2, 2026 09:05
0171339 to
80406ee
Compare
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.
Summary
Four small correctness fixes in
native/can.cc. Each is a real bug from the review backlog; the over-engineered close-callback dance in the original draft of this PR has been dropped — N-API'sUnref()is asynchronous w.r.t. the JS GC and finalize, so the use-after-free I was guarding against can't actually happen on the same loop tick.~RawChannelorderclose(m_SocketFd)ran beforestopThread()while the reader is mid-poll()on that fd — POSIX-undefined (fd can be reused by another thread'sopen()beforepoll()returns). Stop thread first, then close.async_channel_stoppedre-entryStop()and the reader-threaduv_async_sendon POLLHUP/POLLERR. Second invocation would double-uv_close(libuv assertion) and over-Unref(). Addedm_StoppedAlreadysingle-shot guard.pthread_createreturn ignoredCHECK_CONDITION(m_Thread, ...)checked the output opaque handle, which may legitimately be zero. Now capturesrcand checksrc == 0.pthread_mutex_init/pthread_cond_inithad no matching_destroy. Added in~RawChannel, guarded bym_SyncInitializedso it's only attempted when the constructor's successful path actually ran the inits.Item 9 from the backlog (uv_close → Unref ordering) was reconsidered and closed without code change:
ObjectWrap::Unref()only flips the JS reference from strong to weak; the finalizer doesn't run synchronously. By the time GC fires and~RawChannelruns, libuv's close phase has long since completed.Two new members (
m_StoppedAlready,m_SyncInitialized) are zero-init'd in the constructor's initializer list.This PR is based on
fix/correctness-group-1(PR #166), which already contains the Group 1b CAN-FD bit-extraction fix (PR #169) merged into the same chain.Out of scope
std::jthread/std::mutexwould structurally eliminate the destroy-on-shutdown bookkeeping and tighten join semantics. Tracked separately.pthread_createfailure (the rare path where uv handles get init'd then orphaned because the thread couldn't start). Pre-existing leak; not made worse here.Test plan
No new behavioural tests. The races here are not reasonably exercisable from mocha — the JS path can't call
stop()twice (second call throws "Channel not started"), the real double-call comes from a POLLHUP that needs root + kernel cooperation to manufacture, and the destructor reorder is a race-window fix that needs TSan or ASan to catch deterministically. Earlier drafts of this PR included atest-lifecycle.jsthat I claimed validated the fixes — on review those tests didn't actually exercise any of the bugs, so they've been removed.docker build -f Dockerfile.build-test -t node-can-build-test .— native + TS build pass.npx mochainside the container — 47 pass, 1 pending, 10 failing. Same numbers asmaster; the 10 failures are all pre-existingError while creating channelcases caused by missingvcan0/vcan1in the build image.🤖 Generated with Claude Code