Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

stream: pipeline don't destroy Duplex src before 'finish' #32966

Closed
wants to merge 2 commits into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Apr 21, 2020

pipeline was too agressive with destroying Duplex
streams which were the first argument into pipeline.

Just because it's !writable does not mean that it
is safe to be destroyed, unless it has also emitted
'finish'.

Fixes: #32955

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@ronag ronag added the stream Issues and PRs related to the stream subsystem. label Apr 21, 2020
@ronag ronag requested a review from mafintosh April 21, 2020 10:25
@ronag ronag force-pushed the pipeline-destroy branch 2 times, most recently from fcb17b5 to 229f5d2 Compare April 21, 2020 10:26
pipeline was too agressive with destroying Duplex
streams which were the first argument into pipeline.

Just because it's !writable does not mean that it
is safe to be destroyed, unless it has also emitted
'finish'.

Fixes: nodejs#32955
@ronag ronag changed the title stream: pipeline should not destroy Duplex src/dst stream: pipeline don't destroy Duplex src before 'finish' Apr 21, 2020
@ronag ronag added the v13.x label Apr 21, 2020
Comment on lines 53 to 73
const wState = stream._writableState;

const writableEnded = stream.writableEnded ||
(wState && wState.ended);
const writableFinished = stream.writableFinished ||
(wState && wState.finished);

const willFinish = stream.writable ||
(writableEnded && !writableFinished);
const willEnd = stream.readable;

if (err || !final || !stream.readable) {
destroyImpl.destroyer(stream, err);
if (!err) {
// First
if (reading && !writing && willFinish) {
return callback();
}

// Last
if (!reading && writing && willEnd) {
return callback();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: it is possible to reduce the code branches a bit:

Suggested change
const wState = stream._writableState;
const writableEnded = stream.writableEnded ||
(wState && wState.ended);
const writableFinished = stream.writableFinished ||
(wState && wState.finished);
const willFinish = stream.writable ||
(writableEnded && !writableFinished);
const willEnd = stream.readable;
if (err || !final || !stream.readable) {
destroyImpl.destroyer(stream, err);
if (!err) {
// First
if (reading && !writing && willFinish) {
return callback();
}
// Last
if (!reading && writing && willEnd) {
return callback();
}
if (!err) {
// Check if the last stream will end:
if (!reading) {
if (writing && stream.readable) {
return callback();
}
// First stream
} else if (!writing) {
if (stream.writable) {
return callback();
}
const wState = stream._writableState;
const writableEnded = stream.writableEnded ||
(wState && wState.ended);
const writableFinished = stream.writableFinished ||
(wState && wState.finished);
if (writableEnded && !writableFinished)
return callback();
}
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BridgeAR: Any specific reason? This does make it a bit harder to read in my opinon.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code should IMO always only run when necessary. Right now some code is executed that is only needed in some situations.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a hot path so readability should IMO come first. I think I can make a compromise 😄. PTAL.

@ronag ronag mentioned this pull request Apr 21, 2020
4 tasks
destroyImpl.destroyer(stream, err);
if (!err) {
// First
if (reading && !writing && willFinish) {
Copy link
Member Author

@ronag ronag Apr 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The willFinish part here is the significant change. Before we only checked stream.writable.

The rest of the changes should be logically the same.

@ronag ronag added the blocked PRs that are blocked by other issues or PRs. label Apr 21, 2020
@ronag
Copy link
Member Author

ronag commented Apr 21, 2020

blocked pending #32954

@ronag
Copy link
Member Author

ronag commented Apr 21, 2020

closed in favor of #32968

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked PRs that are blocked by other issues or PRs. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Duplex stream pipeline regression in 13
2 participants