Skip to content

quic: stream not reading last chunk on close when using async iterator#35798

Closed
mmomtchev wants to merge 1 commit intonodejs:masterfrom
mmomtchev:async-iter-last
Closed

quic: stream not reading last chunk on close when using async iterator#35798
mmomtchev wants to merge 1 commit intonodejs:masterfrom
mmomtchev:async-iter-last

Conversation

@mmomtchev
Copy link
Contributor

@mmomtchev mmomtchev commented Oct 25, 2020

The async iterator can sometimes miss the last chunk
when there has been an asynchronous event
and a null chunk as used by
test-quic-simple-server-uni.js

Fixes: #35789

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

No new unit tests have been added, this fixes the QUIC unit test : test/parallel/test-quic-simple-server-uni.js

@mmomtchev
stream: fix async iter not reading last chunk on close
01f0d72 
The async iterator can sometimes miss the last chunk
when there has been an asynchronous event
and a null chunk as used by
test-quic-simple-server-uni.js

Fixes: nodejs#35789
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Oct 25, 2020

Review requested:

  • @nodejs/streams

@mmomtchev
Copy link
Contributor Author

mmomtchev commented Oct 25, 2020

@ronag , check this please

ronag
ronag requested changes Oct 25, 2020
View reviewed changes
Copy link
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

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

This is not correct. Will explain later.

@mmomtchev
Copy link
Contributor Author

mmomtchev commented Oct 25, 2020 •
edited
Loading

This is not correct. Will explain later.

Is the null read the problem? Is a null read allowed?
I remade that same unit test around on('data') and it doesn't miss the last chunk in this case

@mmomtchev
Copy link
Contributor Author

mmomtchev commented Oct 26, 2020

@ronag, In fact, when using stream.end(), when processing the last chunk, the close event is already waiting in a nextTick callback and is not giving a chance to the await Promise(next) to resolve. Thus, you always get the close event before reading the last chunk.

@ronag
Copy link
Member

ronag commented Oct 26, 2020 •
edited
Loading

The Readable should not .destroy() until the last chunk has been read. If it is, then it's a bug somewhere else. Who is invoking .destroy()?

@mmomtchev
Copy link
Contributor Author

mmomtchev commented Oct 26, 2020 •
edited
Loading

@ronag, yes, this is a quic issue, I confirm, thanks
It was simply undetected before the async iterator change

@mmomtchev mmomtchev changed the title stream: fix async iterator not reading last chunk on close quic: stream not reading last chunk on close when using async iterator Oct 26, 2020
@mmomtchev
Copy link
Contributor Author

mmomtchev commented Oct 26, 2020

This is a problem with the QUIC code synchronously destroying the Readable when a write is final

@mmomtchev mmomtchev closed this Oct 26, 2020
@jasnell
Copy link
Member

jasnell commented Oct 26, 2020

This entire bit of code is going to be refactored very soon (within a few weeks)

@mmomtchev
Copy link
Contributor Author

mmomtchev commented Oct 26, 2020

@jasnell I fixed it in a separate PR, it is a trivial change
#35820

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ronag ronag ronag requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

quic: async_iterator doesn't return the last chunk

4 participants

@mmomtchev @nodejs-github-bot @ronag @jasnell