Stop identity stream aborts from waiting for a read - #7399
Conversation
|
LGTM! |
e49179e to
d006054
Compare
d006054 to
8cb3518
Compare
8cb3518 to
3f20666
Compare
guybedford
left a comment
guybedford
left a comment
There was a problem hiding this comment.
Verified locally: with the fix reverted on the base, the three new tests fail in identity-ts@ with abort() must not wait for a read; with it, all identity and piping variants pass. Also checked that abort with nothing parked followed by a read rejects rather than hangs, that a second abort() after the one-shot hook is consumed still settles, and that abort racing an already-pending read is unchanged from before.
The ordering holds as described: the hook fires before writableStreamStartErroring, and the parked write's continuation runs in a microtask after that, so it always observes erroring with the abort reason as the stored error. The one-shot is safe since the identity controller isn't user-reachable, so nothing can re-enter between the hook and the erroring transition.
Nit: after the rebase #readyHook is declared as #readyHook: (() => void) | undefined while the new #abortHook?: (() => void) | undefined keeps the optional form; worth matching.
A write to an IdentityTransformStream or FixedLengthStream waits in the sink until a read consumes it. abort() runs the abort steps only once the in-flight write has settled, and only a read could wake that write, so with no reader the abort and the write never settled. Error-handling code that aborts a streaming response's writer hung. The writable machinery now has a module-private abort hook, called once when an abort is requested, right after the controller's signal fires. The identity streams use it to wake the waiting write, which finds the stream erroring and rejects with the abort reason; the abort then completes, as in the C++ implementation. An 'abort' listener on the controller's signal would do the same, but adding one costs about 8 microseconds, more than doubling the cost of constructing these streams. Reads after such an abort reject with the abort reason. The C++ implementation rejects them with "Network connection lost.", because cancelling the waiting sink write puts the transform into its disconnection error first; the identity suite pins both. The TypeScript streams implementation is experimental (typescript_implemented_streams), so no compatibility flag is needed.
3f20666 to
735a2d7
Compare
2 participants
A write to an IdentityTransformStream or FixedLengthStream waits in the
sink until a read consumes it. abort() runs the abort steps only once
the in-flight write has settled, and only a read could wake that write,
so with no reader the abort and the write never settled. Error-handling
code that aborts a streaming response's writer hung.
The writable machinery now has a module-private abort hook, called once
when an abort is requested, right after the controller's signal fires.
The identity streams use it to wake the waiting write, which finds the
stream erroring and rejects with the abort reason; the abort then
completes, as in the C++ implementation. An 'abort' listener on the
controller's signal would do the same, but adding one costs about 8
microseconds, more than doubling the cost of constructing these streams.
Reads after such an abort reject with the abort reason. The C++
implementation rejects them with "Network connection lost.", because
cancelling the waiting sink write puts the transform into its
disconnection error first; the identity suite pins both