Skip to content

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

Closed
ronag wants to merge 2 commits intonodejs:masterfrom
nxtedition:eos-finished-premature-close
Closed

stream: fix finished w/ 'close' before 'finish'#31534
ronag wants to merge 2 commits intonodejs:masterfrom
nxtedition:eos-finished-premature-close

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

nodejs-github-bot commented Jan 28, 2020

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jan 29, 2020

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

nodejs-github-bot commented Jan 29, 2020

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

nodejs-github-bot commented Jan 29, 2020

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/

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

richardlau commented Jan 31, 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

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

nodejs-github-bot commented Feb 1, 2020

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

MylesBorins 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

@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

@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

+1 more reviewer

@lundibundi lundibundi lundibundi left review comments

Reviewers whose approvals may not affect merge requirements

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.