quic: delay destroying the readable until 'end' #35820
Conversation
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:
|
Codecov Report
@@ 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
|
| // Delay destroying the Readable until the end has been received | ||
| this.once('end', () => { | ||
| this.destroy(); | ||
| }); |
There was a problem hiding this comment.
Doesn't it have autoDestroy enabled?
|
|
||
| // Put the QuicStream into detached mode before calling destroy | ||
| this[kSetHandle](); | ||
| this.destroy(); |
There was a problem hiding this comment.
I think just removing this line would be the samething if autoDestroy is enabled.
There was a problem hiding this comment.
Also I think the current code is probably correct, kDestroy is not supposed to be a graceful shutdown? @jasnell?
There was a problem hiding this comment.
autoDestroy is indeed enabled
kDestroy() is called from onSessionClose() which is called from C++ QuicSession::Close(), it handles both errors and a normal close
There was a problem hiding this comment.
I'm pretty sure that the code below is unnecessary and removing destroy() here might or might not be correct.
There was a problem hiding this comment.
node test/parallel/test-quic-simple-server-uni.js
You need a QUIC-enabled build
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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
There was a problem hiding this comment.
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.
|
|
||
| // Put the QuicStream into detached mode before calling destroy | ||
| this[kSetHandle](); | ||
| this.destroy(); |
There was a problem hiding this comment.
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 |
|
@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?) |
|
Closing as no longer relevant since quic was removed. |
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), orvcbuild test(Windows) passes