Skip to content

quic: delay destroying the readable until 'end' #35820

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
mmomtchev wants to merge 1 commit into from
Closed

quic: delay destroying the readable until 'end' #35820

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion lib/internal/quic/core.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2673,7 +2673,14 @@ class QuicStream extends Duplex {

// Put the QuicStream into detached mode before calling destroy
this[kSetHandle]();
this.destroy();
Copy link
Member

@ronag ronag Oct 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think just removing this line would be the samething if autoDestroy is enabled.

Copy link
Member

@ronag ronag Oct 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also I think the current code is probably correct, kDestroy is not supposed to be a graceful shutdown? @jasnell?

Copy link
Contributor Author

@mmomtchev mmomtchev Oct 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

autoDestroy is indeed enabled
kDestroy() is called from onSessionClose() which is called from C++ QuicSession::Close(), it handles both errors and a normal close

Copy link
Member

@ronag ronag Oct 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure that the code below is unnecessary and removing destroy() here might or might not be correct.

Copy link
Contributor Author

@mmomtchev mmomtchev Oct 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

node test/parallel/test-quic-simple-server-uni.js

You need a QUIC-enabled build

Copy link
Contributor Author

@mmomtchev mmomtchev Oct 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test was working before you stopped sending the data from destroyed streams.
In QUIC you can mark a data chunk as being the final data chunk. In this case the client is supposed to close the connection upon receiving it. The QUIC code will immediately destroy the Readable in a nextTick handler. When using on('data') the user code will get a chance to read the last chunk, but when using the async iterator, and after your change, the Promise won't be resolved until the 'close' is processed.

Copy link
Contributor Author

@mmomtchev mmomtchev Oct 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jasnell said that a complete shake-up on that code was in the works, so he probably doesn't care that much 😄
But I had already debugged it before he said it

Copy link
Member

@jasnell jasnell Oct 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. Starting likely week after next this entire part of the code is going to be refactored entirely away from the Duplex interface. I don't have time to detail the specifics at the moment but will do so soon.

if (this._readableState.endEmitted)
this.destroy();
else {
// Delay destroying the Readable until the end has been received
this.once('end', () => {
this.destroy();
});
Copy link
Member

@ronag ronag Oct 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't it have autoDestroy enabled?

}
}

[kHeaders](headers, kind, push_id) {
Expand Down