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.

Sign up for GitHub

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

Jump to bottom

stream: fix finished w/ 'close' before 'finish' #31534

Closed
wants to merge 2 commits into from
Closed

stream: fix finished w/ 'close' before 'finish' #31534

wants to merge 2 commits into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Jan 27, 2020

Emitting 'close' before 'finish' on a Writable should result in a premature close error.

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 force-pushed the eos-finished-premature-close branch 2 times, most recently from 46fab48 to 20c9ac6 Compare January 27, 2020 13:55
mcollina
mcollina approved these changes Jan 27, 2020
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@ronag ronag added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. stream Issues and PRs related to the stream subsystem. labels Jan 27, 2020
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jan 27, 2020 •
edited by Trott
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/28717/ ✅

@ronag
Copy link
Member Author

ronag commented Jan 27, 2020 •
edited
Loading

I'm hoping this does not cause problems such as #29699. Which was on the Readable side.

jasnell
jasnell approved these changes Jan 27, 2020
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM but if there is a risk this could break things, we should do some additional verification before landing and/or possibly consider semver-major

@ronag
Copy link
Member Author

ronag commented Jan 28, 2020 •
edited
Loading

Defensive CITGM

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2246/ (queued)

@ronag
Copy link
Member Author

ronag commented Jan 28, 2020

@jasnell's comment raised my concern as well, I've made this check a little less strict. PTAL

@ronag ronag force-pushed the eos-finished-premature-close branch from f916506 to b517e78 Compare January 28, 2020 05:32
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@ronag ronag force-pushed the eos-finished-premature-close branch from b517e78 to 7a59f50 Compare January 28, 2020 05:32
@ronag ronag force-pushed the eos-finished-premature-close branch from 7a59f50 to 7a68bbd Compare January 28, 2020 05:43
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/28723/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/28744/

@ronag
stream: fix finished w/ 'close' before 'finish'
1e9ab30 
Emitting 'close' before 'finish' on a Writable should
result in a premature close error.
@ronag
Copy link
Member Author

ronag commented Jan 29, 2020

rebased to fix conflict

@ronag ronag force-pushed the eos-finished-premature-close branch from 7a68bbd to 1e9ab30 Compare January 29, 2020 08:31
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/28755/

@ronag
Copy link
Member Author

ronag commented Jan 29, 2020

@Trott: That rebase failure is new to me:

09:32:51 error: refs/heads/jenkins-node-test-commit-arm-fanned-b52e259ba37df1f0e58cd95a6bf61f359f80edf1 does not point to a valid object!
09:32:52 remote: error: refs/heads/jenkins-node-test-commit-arm-fanned-b52e259ba37df1f0e58cd95a6bf61f359f80edf1 does not point to a valid object!        
09:32:52 remote: fatal: bad object .alternate        
09:32:52 error: refs/heads/jenkins-node-test-commit-arm-fanned-b52e259ba37df1f0e58cd95a6bf61f359f80edf1 does not point to a valid object!
09:32:53 fatal: bad object .alternate

@ronag
Copy link
Member Author

ronag commented Jan 29, 2020

I would recommend this is semver-major as well to ensure no breakage in ecosystem. These kind of changes seem to be a bit sensitive.

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/28762/

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jan 31, 2020 •
edited by ronag
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/28812/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2253/

@ronag ronag force-pushed the eos-finished-premature-close branch from 01fc4c6 to 6282580 Compare January 31, 2020 20:09
@richardlau
Copy link
Member

@Trott: That rebase failure is new to me:

09:32:51 error: refs/heads/jenkins-node-test-commit-arm-fanned-b52e259ba37df1f0e58cd95a6bf61f359f80edf1 does not point to a valid object!
09:32:52 remote: error: refs/heads/jenkins-node-test-commit-arm-fanned-b52e259ba37df1f0e58cd95a6bf61f359f80edf1 does not point to a valid object!        
09:32:52 remote: fatal: bad object .alternate        
09:32:52 error: refs/heads/jenkins-node-test-commit-arm-fanned-b52e259ba37df1f0e58cd95a6bf61f359f80edf1 does not point to a valid object!
09:32:53 fatal: bad object .alternate

For the record this was fixed in nodejs/build#2153.

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/28844/

@ronag
Copy link
Member Author

ronag commented Feb 1, 2020

CI passes

addaleax pushed a commit that referenced this pull request Feb 5, 2020
@ronag @addaleax
stream: fix finished w/ 'close' before 'finish'
234de6f 
Emitting 'close' before 'finish' on a Writable should
result in a premature close error.

PR-URL: #31534
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@addaleax
Copy link
Member

addaleax commented Feb 5, 2020

Landed in 234de6f

@addaleax addaleax closed this Feb 5, 2020
codebytere pushed a commit that referenced this pull request Feb 17, 2020
@ronag @codebytere
stream: fix finished w/ 'close' before 'finish'
6d0b226 
Emitting 'close' before 'finish' on a Writable should
result in a premature close error.

PR-URL: #31534
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@codebytere codebytere mentioned this pull request Feb 17, 2020
Merged
@MylesBorins
Copy link
Contributor

@ronag should we land this on 12.x? It lands cleanly, just want to make sure if we need to backport a bunch of streams stuff it isn't partially done

@ronag
Copy link
Member Author

ronag commented Apr 2, 2020

@ronag should we land this on 12.x? It lands cleanly, just want to make sure if we need to backport a bunch of streams stuff it isn't partially done

My recommendation is not to land this on 12.x. It's not semver major but it slightly changes the behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lundibundi lundibundi lundibundi left review comments

@addaleax addaleax addaleax approved these changes

@mafintosh mafintosh Awaiting requested review from mafintosh

@mcollina mcollina Awaiting requested review from mcollina

@jasnell jasnell Awaiting requested review from jasnell

@lpinca lpinca Awaiting requested review from lpinca

@Trott Trott Awaiting requested review from Trott

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.