fix(asyncengine): wake both callbacks on Event.Error (#709) - #710
Open
cryo2010 wants to merge 2 commits into
Open
fix(asyncengine): wake both callbacks on Event.Error (#709)#710cryo2010 wants to merge 2 commits into
cryo2010 wants to merge 2 commits into
Conversation
poll() only resumed an I/O callback for its own direction: Event.Error
woke a side only when the event was exactly {Error}. When the kernel
coalesced the error with the opposite direction (e.g. {Read, Error} with
a full send buffer, or {Write, Error} with no data to read), the other
side's callback was never scheduled. Its readLoop/writeStreamLoop never
resumed, leaking the parked readOnce/write future and hanging the
consumer.
This restores the fix from status-im#672 (reverted by status-im#700 for causing a kqueue
double-callback crash) without the crash:
- poll(): schedule the reader on {Read, Error} and the writer on
{Write, Error}, so neither side is left un-resumed.
- readerCb: tolerate the resulting spurious/duplicate wakeups (kqueue
reports {Read, Error} and {Write, Error} as separate events for the
same fd). Skip reading when {Closed, ReadPaused} is already set and
replace `doAssert progress` with `if readIntoBuffer():`;
completeReader() still runs unconditionally so close-injected events
and genuine EOF/error still complete the parked read. writeStreamLoop
is already idempotent.
Adds the dispatcher-level regression test back (both callbacks must fire
on {Read, Error}); status-im#700's kqueue "Reader notification after buffer got
full" test continues to pass.
cryo2010
marked this pull request as draft
August 22, 2026 05:33
cryo2010
marked this pull request as ready for review
August 22, 2026 05:40
Member
|
Thanks for the PR - this however ended up a duplicate of #705 - the new test looks interesting though, if you would care to resolve the conflicts. |
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
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.
Fixes #709.
Problem
poll()only resumes an I/O callback for its own direction:Event.Errorwakes a side only when the event is exactly{Error}. When the kernel coalesces the error with the opposite direction — e.g.{Read, Error}(send buffer full, so noEPOLLOUT) or{Write, Error}(nothing to read) — the other side's callback is never scheduled. Its readLoop/writeStreamLoopnever resumes, so a parkedreadOnce/write` future is never completed: the consumer hangs forever and the future leaks.This is the leak fixed by #672 and re-introduced by #700, which reverted #672 because its blunt form caused a kqueue double-callback crash and deferred "a proper fix." This is an attempt at a final fix.
Change
chronos/internal/asyncengine.nim—poll()schedules the reader on{Read, Error}and the writer on{Write, Error}, so neither side is left un-resumed:chronos/transports/stream.nim—readerCbnow tolerates the resulting spurious/duplicate wakeups (kqueue reports{Read, Error}and{Write, Error}as separate events for the same fd): it skips reading when{Closed, ReadPaused}is already set, and usesif readIntoBuffer():instead ofdoAssert progress.completeReader()still runs unconditionally so the close-injected event and genuine EOF/error still complete the parked read.writeStreamLoopis already idempotent (an empty queue simply re-pauses), so it needs no change.tests/testbugs.nim— restores the dispatcher-level regression test (both reader and writer callbacks must fire on{Read, Error}). It fails onmaster(writerFlag == false) and passes with this change, on both the epoll and poll engines. Revert "Always report Event.Error to both read/write callbacks" #700's kqueue "Reader notification after buffer got full" test continues to pass.Verification
Deterministic dispatcher test, before/after:
Reproduced the leak end-to-end under Valgrind (Docker
nimlang/nim:2.2.10, Linux aarch64, Valgrind 3.24.0, ORC +-d:useMalloc, epoll engine). Aconsumercoroutine awaits a future resolved by a writer callback (standing in forwriteStreamLoop), with the fd held in{Read, Error}; the repro is in #709.Before (on
master):parked-future.finished = falseAfter (this PR):
consumer resumed,parked-future.finished = trueThe
definitely/indirectly lostblocks (the orphaned future and its suspendedconsumerframe) go from136/240to0/0. Thepossibly lost12,864 bytes in 11 blocksis unchanged between the two builds — ORC/dispatcherinterior-pointer bookkeeping, unrelated to this bug.
Suites pass on both engines:
testbugs,teststream,testasyncstream,testhttpclient,testshttpserver(TLS),testserver,testfut,testsoon,testsignal,testdatagram.