stream: fix stream.finished on Duplex#33133
Uh oh!
There was an error while loading. Please reload this page.
Conversation
finished would incorrectly believe that a Duplex is already closed if either the readable or writable side has completed. Fixes: nodejs#33130
ronag commented Apr 28, 2020
@nodejs/streams @mafintosh@szmarczak |
ronag commented Apr 28, 2020
fast-track? |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
szmarczak commented Apr 28, 2020
@ronag You need to delay the write to |
Uh oh!
There was an error while loading. Please reload this page.
nodejs-github-bot commented Apr 28, 2020
| (wState&&wState.errorEmitted)||(rState&&rState.errorEmitted)|| | ||
| (wState&&wState.finished)||(rState&&rState.endEmitted)|| | ||
| (rState&&stream.req&&stream.aborted); | ||
| constclosed=( |
There was a problem hiding this comment.
Can we simplify this a bit? Perhaps at the very least group together similar dependencies, like:
constclosed=((wState&&(wState.closed||wState.errorEmitted))||(rState&&(rState.closed||rState.errorEmitted||(stream.req&&stream.aborted)))||((!writable||(wState&&wState.finished))&&(!readable||(rState&&rState.endEmitted))));
There was a problem hiding this comment.
Unless there is a performance benefit to the above suggestion, I prefer @ronag's grouping because it's easier to read. Though it has more lines, it has less parentheses and the lines are ordered by the properties (e.g. closed) of the states. It reads like "is either state closed, or either state errorEmitted, or ..".
There was a problem hiding this comment.
I have not personally benchmarked it, so I cannot say if it is measurable or not. However, it is reducing the number of duplicate checks so V8 should be performing less work.
However, my code suggestion was merely one possibility. I'm not opposed to rearranging the checks in other ways, such as introducing separate if statements, etc.
There was a problem hiding this comment.
I prefer the current formatting and believe any performance implication here would be negligible in practice. This is not a hot path as far as I'm aware. A future simplification could be to use the ?. operator.
| // TODO(ronag): Throw some kind of error? Does it make sense | ||
| // to call finished() on a "finished" stream? | ||
| // TODO(ronag): willEmitClose? | ||
| process.nextTick(()=>{ |
There was a problem hiding this comment.
For bonus points this could also be simplified to process.nextTick(callback); if you want to change it while we're in here. Either way is fine though.
There was a problem hiding this comment.
@mscdex Strangely CI fails with your suggestion. Not sure why. Leaving it as is for the purposes of this PR.
There was a problem hiding this comment.
Ah, it's because we re-assign callback in the disposer.
nodejs-github-bot commented Apr 29, 2020
This reverts commit 882f704.
nodejs-github-bot commented Apr 30, 2020
ronag commented Apr 30, 2020
Landed in d84f131 |
finished would incorrectly believe that a Duplex is already closed if either the readable or writable side has completed. Fixes: #33130 PR-URL: #33133 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
finished would incorrectly believe that a Duplex is already closed if either the readable or writable side has completed. Fixes: #33130 PR-URL: #33133 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
7 participants
finished would incorrectly believe that a Duplex is already
closed if either the readable or writable side has completed.
Fixes: #33130
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes