quic: delay destroying the readable until 'end' by mmomtchev · Pull Request #35820 · nodejs/node · GitHub

Delay destroying the readable until the end has been
received. This fixes the existing unit test that uses
an async iterator which won't read the last chunk
if the Readable has been destroyed.

Refs: 1d8ecd8
Fixes: #35789

Checklist

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

Delay destroying the readable until the end has been received. This fixes the existing unit test that uses an async iterator which won't read the last chunk if the Readable has been destroyed. Refs: nodejs@1d8ecd8 Fixes: nodejs#35789

Review requested:

@nodejs/quic

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.90%. Comparing base (ba907ff) to head (52e4d79).

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #35820   +/-   ##
=======================================
  Coverage   87.90%   87.90%           
=======================================
  Files         477      477           
  Lines      113172   113172           
  Branches    25425    25428    +3     
=======================================
+ Hits        99480    99485    +5     
+ Misses       7992     7990    -2     
+ Partials     5700     5697    -3

see 18 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Doesn't it have autoDestroy enabled?

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

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

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

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

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

You need a QUIC-enabled build

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.

@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

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.

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

@mmomtchev can you please also add a test so we can get a better understanding of what we are trying to fix here?

This looks like a more fundamental bug to me. IMO kDestroy should not be called until 'end' or at least destroy() should not be called in the graceful case. @jasnell

@ronag @jasnell How about pushing forward this PR as a temporary solution? It is a simple change that will fix a flaky OSX unit test that is failing very often (always maybe?)
@ronag destroy() is called in the graceful case almost everywhere - http, http2, tls...
I agree that it should not - that fix for the ECONNRESET issue showed why it is important to have a single generic and well-defined code-path for closing - but given that @jasnell is refactoring anyway, maybe this should be left out of this PR

Closing as no longer relevant since quic was removed.

quic: delay destroying the readable until 'end'

52e4d79

Delay destroying the readable until the end has been received. This fixes the existing unit test that uses an async iterator which won't read the last chunk if the Readable has been destroyed. Refs: nodejs@1d8ecd8 Fixes: nodejs#35789

nodejs-github-bot added dont-land-on-v12.x labels Oct 26, 2020

nodejs-github-bot added the quic Issues and PRs related to the QUIC implementation / HTTP/3. label Oct 26, 2020

mmomtchev mentioned this pull request Oct 26, 2020

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

Closed

3 tasks

Trott requested a review from ronag October 28, 2020 13:51

Trott approved these changes Oct 28, 2020

View reviewed changes

Trott requested a review from jasnell October 28, 2020 13:52

ronag reviewed Oct 28, 2020

View reviewed changes

ronag requested changes Oct 28, 2020

View reviewed changes

mmomtchev mentioned this pull request Nov 3, 2020

http2: move events to the JSStreamSocket #35772

Merged

3 tasks

mmomtchev mentioned this pull request Nov 10, 2020

quic: uni and bidi simple server tests fail on master #36071

Closed

jasnell closed this Feb 1, 2021

+                    // Delay destroying the Readable until the end has been received
+                    this.once('end', () => {
+                      this.destroy();
+                    });

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

quic: delay destroying the readable until 'end' by mmomtchev · Pull Request #35820 · nodejs/node · GitHub

Uh oh!

Uh oh!

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

quic: delay destroying the readable until 'end'#35820mmomtchev wants to merge 1 commit intonodejs:masternodejs/node:masterfrom mmomtchev:quic-delay-readable-destroymmomtchev/node:quic-delay-readable-destroyCopy head branch name to clipboard

Conversation

mmomtchev commented Oct 26, 2020

Checklist

Uh oh!

nodejs-github-bot commented Oct 26, 2020

Uh oh!

codecov-io commented Oct 26, 2020 • edited by codecov Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mmomtchev Oct 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ronag commented Oct 28, 2020

Uh oh!

ronag commented Oct 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mmomtchev commented Dec 1, 2020

Uh oh!

jasnell commented Feb 1, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

quic: delay destroying the readable until 'end'#35820
mmomtchev wants to merge 1 commit into
nodejs:masterfrom
mmomtchev:quic-delay-readable-destroy

codecov-io commented Oct 26, 2020 •
edited by codecov Bot
Loading

mmomtchev Oct 28, 2020 •
edited
Loading

ronag commented Oct 28, 2020 •
edited
Loading