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: fix finished w/ 'close' before 'finish' #31534

Closed
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
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

@ronag
Copy link
Member Author

ronag commented Jan 27, 2020

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

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

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.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

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

@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
Copy link
Collaborator

nodejs-github-bot commented Jan 31, 2020

@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.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

@ronag
Copy link
Member Author

ronag commented Feb 1, 2020

CI passes

addaleax pushed a commit that referenced this pull request Feb 5, 2020
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
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
@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
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
Development

Successfully merging this pull request may close these issues.