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 'end' #31545

Closed
wants to merge 1 commit into from
Closed

stream: fix finished w/ 'close' before 'end' #31545

wants to merge 1 commit into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Jan 28, 2020 •
edited

Emitting 'close' before 'end' on a Readable should result in a premature close error.

This is related to the one that caused trouble in #29699. Includes the fix proposed in #29724.

Related discussion: https://github.com/nodejs/node/pull/29724/files#r329128356

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
Copy link
Member Author

ronag commented Jan 28, 2020

Since there was problems with a similar PR before I would suggest a semver-major on this one.

@ronag ronag added the stream Issues and PRs related to the stream subsystem. label Jan 28, 2020
@mcollina mcollina added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jan 28, 2020
mcollina
mcollina approved these changes Jan 28, 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 requested review from jasnell and lpinca January 28, 2020 10:21
@nodejs-github-bot
Copy link
Collaborator

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

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

@ronag ronag added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 28, 2020
@ronag
Copy link
Member Author

ronag commented Jan 29, 2020

CITGM failure on socket.io might be worth looking into.

@ronag ronag removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 29, 2020
@ronag
Copy link
Member Author

ronag commented Jan 29, 2020

rebased to fix conflict

@ronag
Copy link
Member Author

ronag commented Jan 29, 2020

Looked into socket.io and winston CITGM failures. Was not able to find anything strange on MacOS.

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@ronag ronag force-pushed the eos-closed-ended branch 2 times, most recently from a1d6950 to 88c260e Compare January 31, 2020 20:10
@ronag
Copy link
Member Author

ronag commented Jan 31, 2020

CITGM failures worth looking into:

  • citgm.semver-v7.1.1
  • citgm.winston-v3.2.1
  • citgm.socket.io-v2.3.0

I'm pretty much stuck on these. Unable to reproduce.

@ronag
Copy link
Member Author

ronag commented Jan 31, 2020

CI passes, not sure if author-ready label should be applied given CITGM status. Please remove if appropriate.

@ronag ronag added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 31, 2020
@Trott Trott added the notable-change PRs with changes that should be highlighted in changelogs. label Feb 1, 2020
@Trott
Copy link
Member

Trott commented Feb 1, 2020

@nodejs/streams @nodejs/tsc This seems OK to me as a semver-major, but could use more reviews. CI and CITGM both look good.

@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 5, 2020
@addaleax
Copy link
Member

addaleax commented Feb 5, 2020

This needs a rebase, sorry

@ronag
Copy link
Member Author

ronag commented Feb 5, 2020

rebased

@nodejs-github-bot
Copy link
Collaborator

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

@ronag
Copy link
Member Author

ronag commented Feb 9, 2020

@addaleax: rebased, approve?

@ronag ronag requested a review from addaleax February 14, 2020 09:38
@ronag ronag added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 14, 2020
@ronag
Copy link
Member Author

ronag commented Feb 24, 2020

rebased to fix conflicts

@nodejs-github-bot
Copy link
Collaborator

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

@ronag
Copy link
Member Author

ronag commented Feb 28, 2020

rebased to fix conflicts

@ronag ronag force-pushed the eos-closed-ended branch 2 times, most recently from 07c6efd to 6a90b56 Compare February 28, 2020 20:21
@ronag
stream: fix finished w/ 'close' before 'end'
f1cf385 
Emitting 'close' before 'end' on a Readable should
result in a premature close error.
@ronag
Copy link
Member Author

ronag commented Feb 29, 2020

This has 2 TSC approvals but I believe @Trott's approval is conditioned on further review from @nodejs/streams and/or @nodejs/tsc.

@Trott
Copy link
Member

Trott commented Feb 29, 2020

This has 2 TSC approvals but I believe @Trott's approval is conditioned on further review from @nodejs/streams and/or @nodejs/tsc.

You can treat mine the same as any other approval. How about we wait another 72 hours in a last effort to get a few more eyes on this? @nodejs/collaborators

@nodejs-github-bot
Copy link
Collaborator

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

@ronag
Copy link
Member Author

ronag commented Mar 3, 2020

Landed in 7cafd5f

@ronag ronag closed this Mar 3, 2020
ronag added a commit that referenced this pull request Mar 3, 2020
@ronag
stream: fix finished w/ 'close' before 'end'
7cafd5f 
Emitting 'close' before 'end' on a Readable should
result in a premature close error.

PR-URL: #31545
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Mar 10, 2020
Merged
@ronag ronag mentioned this pull request Apr 25, 2020
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcollina mcollina mcollina approved these changes

@Trott Trott Trott approved these changes

@mafintosh mafintosh Awaiting requested review from mafintosh

@jasnell jasnell Awaiting requested review from jasnell

@lpinca lpinca Awaiting requested review from lpinca

@addaleax addaleax Awaiting requested review from addaleax

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. notable-change PRs with changes that should be highlighted in changelogs. semver-major PRs that contain breaking changes and should be released in the next major version. 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.

None yet
5 participants
@ronag @nodejs-github-bot @Trott @addaleax @mcollina