-
Notifications
You must be signed in to change notification settings - Fork 30k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
stream: don't destroy final readable stream in pipeline
If the last stream in a pipeline is still usable/readable don't destroy it to allow further composition. Fixes: #32105 PR-URL: #32110 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
- Loading branch information
Showing
2 changed files
with
57 additions
and
4 deletions.
There are no files selected for viewing
This file contains 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
This file contains 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
4d93e10
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this is still an issue in the edgecase where a pipeline is piping a stream back into itself (e.g. like an echo server) ...
This will end up failing inconsistently.
4d93e10
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand this example? Could you maybe show a full repro?
4d93e10
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Take a look at https://github.com/jasnell/quic/blob/rebase-nodejs-node/test/parallel/test-quic-quicsocket-packetloss-stream-tx.js
Essentially, when you have a
Duplex
and you're piping it back in to itself, it will fail inconsistently. In the example above, I send a single character at a time from the client that is echoed back by the server. When the stream ends on the client, the result is checked against what is expected. However, the client side of the stream is closing prematurely, which indicates that the server side is closing prematurely. I've traced it back to the pipeline closing things down. When I pipe manually using the'data'
and'end'
events, everything works perfectly.4d93e10
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll checkout and build your fork and see if I can reproduce it there and make sense of it.
4d93e10
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For more context, it is a recent change that broke this. I just rebased on master today. My prior rebase was on Feb 13th and pipeline was working fine then.
4d93e10
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect #31940
4d93e10
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might be fixed once #32158 lands. Do you think we could wait until then before digging into this or would you prefer a solution asap?
4d93e10
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you known how can I fix it temporary ? My server is down due to this bug.
4d93e10
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jasnell @BenoitClaveau could you try with #32198?