Always resolve while draining the stream#542
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
peter-leonov-ch
force-pushed
the
always_drain_stream
branch
from
b0e1c30 to
e70fba2
Compare
peter-leonov-ch
force-pushed
the
always_drain_stream
branch
from
2af114c to
a525f09
Compare
peter-leonov-ch
changed the title
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
| for await (const _ of stream) { | ||
| // consume the stream | ||
| } |
There was a problem hiding this comment.
@slvrtrn this is too good to be true 😅 Is there anything I'm missing here?
There was a problem hiding this comment.
can it throw now? there used to be error handler as well
There was a problem hiding this comment.
Yes it can. I've only dared to touch this piece after adding a bunch of unit tests for drainStream(). On the way I've fixed a hanging state when drainStream() receives a closed stream which does not emit the end event. This case is covered by the iterator out of the box.
The very reason I've tried the iterator here is that the pattern "check the state => add listeners => listen => remove listeners => resolve / reject" reminded about the even stream => async iterator wrappers in general.
Stateful iterators handle both the streaming bit that we need here and the burdensome init / teardown stages.
There was a problem hiding this comment.
Now if we wanted to silence the errors we'd just wrap the loop in a try/catch. It's also easier to measure time inside the function by wrapping the loop in a "span" or console.time()-like construct.
## Summary This is a more conservative version of: * #542 Here we keep the iterator approach for future. Also, splitting the public API `drainStream()` from the new `drainStreamInternal()` internal one. --------- Co-authored-by: Copilot <[email protected]>
peter-leonov-ch
force-pushed
the
always_drain_stream
branch
from
2acd6bc to
1dadfa3
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates the Node client’s stream draining helper to avoid hanging when draining a stream that has already ended, by switching to async-iteration-based consumption.
Changes:
- Replace manual event-listener-based draining in
drainStream()withfor await ... ofconsumption. - Rely on async iterator semantics to handle already-ended streams without waiting for an
endevent.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
| for await (const _ of stream) { | ||
| // consume the stream |
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
| for await (const _ of stream) { |
Summary
Make sure that an already ended stream does not hang waiting for the
endevent.While writing tests for
drainStream()it turned out thatfor awaitcovers all the tests cases.Checklist
Delete items not relevant to your PR: