stream: fix stream.finished on Duplex #33133
Conversation
finished would incorrectly believe that a Duplex is already closed if either the readable or writable side has completed. Fixes: nodejs#33130
|
@nodejs/streams @mafintosh @szmarczak |
|
fast-track? |
|
@ronag You need to delay the write to |
| (wState && wState.errorEmitted) || (rState && rState.errorEmitted) || | ||
| (wState && wState.finished) || (rState && rState.endEmitted) || | ||
| (rState && stream.req && stream.aborted); | ||
| const closed = ( |
There was a problem hiding this comment.
Can we simplify this a bit? Perhaps at the very least group together similar dependencies, like:
const closed = (
(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.
They are grouped? See below.
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.
|
|
||
| if (closed) { | ||
| // TODO(ronag): Re-throw error if errorEmitted? | ||
| // TODO(ronag): Throw premature close as if finished was called? | ||
| // before being closed? i.e. if closed but not errored, ended or finished. | ||
| // 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.
This reverts commit 882f704.
|
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]>
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