| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
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 |
Sorry, something went wrong.
|
fast-track? |
Sorry, something went wrong.
Sorry, something went wrong.
|
@ronag You need to delay the write to response, so the readable event is emitted after the finish one. See the gist above. |
Sorry, something went wrong.
Sorry, something went wrong.
There was a problem hiding this comment.
lgtm
Sorry, something went wrong.
| (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))
)
);
Sorry, something went wrong.
There was a problem hiding this comment.
They are grouped? See below.
Sorry, something went wrong.
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 ..".
Sorry, something went wrong.
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.
Sorry, something went wrong.
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.
Sorry, something went wrong.
Sorry, something went wrong.
| // 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.
Sorry, something went wrong.
There was a problem hiding this comment.
Fixed
Sorry, something went wrong.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
Ah, it's because we re-assign callback in the disposer.
Sorry, something went wrong.
Sorry, something went wrong.
This reverts commit 882f704.
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
finished would incorrectly believe that a Duplex is already
closed if either the readable or writable side has completed.
Fixes: #33130
Checklist