Skip to content

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

Closed
ronag wants to merge 1 commit into
nodejs:masterfrom
nxtedition:eos-closed-ended
Closed

stream: fix finished w/ 'close' before 'end'#31545
ronag wants to merge 1 commit into
nodejs:masterfrom
nxtedition:eos-closed-ended

Conversation

@ronag

@ronagronag commented Jan 28, 2020•
edited
Loading

Copy link
Copy Markdown
Member

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

ronag commented Jan 28, 2020

Copy link
Copy Markdown
MemberAuthor

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

@ronagronag added the stream Issues and PRs related to the stream subsystem. label Jan 28, 2020
@mcollinamcollina 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

@mcollinamcollina left a comment

Copy link
Copy Markdown
Member

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 lpincaJanuary 28, 2020 10:21
@nodejs-github-bot

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

Copy link
Copy Markdown
Collaborator

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

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

ronag commented Jan 29, 2020

Copy link
Copy Markdown
MemberAuthor

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

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

ronag commented Jan 29, 2020

Copy link
Copy Markdown
MemberAuthor

rebased to fix conflict

@ronag

ronag commented Jan 29, 2020

Copy link
Copy Markdown
MemberAuthor

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

@nodejs-github-bot

nodejs-github-bot commented Jan 30, 2020

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-bot

nodejs-github-bot commented Jan 31, 2020

Copy link
Copy Markdown
Collaborator

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

@ronag
ronagforce-pushed the eos-closed-ended branch 2 times, most recently from a1d6950 to 88c260eCompareJanuary 31, 2020 20:10
@ronag

ronag commented Jan 31, 2020

Copy link
Copy Markdown
MemberAuthor

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

ronag commented Jan 31, 2020

Copy link
Copy Markdown
MemberAuthor

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

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

Trott commented Feb 1, 2020

Copy link
Copy Markdown
Member

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

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

addaleax commented Feb 5, 2020

Copy link
Copy Markdown
Member

This needs a rebase, sorry

@ronag

ronag commented Feb 5, 2020

Copy link
Copy Markdown
MemberAuthor

rebased

@nodejs-github-bot

nodejs-github-bot commented Feb 8, 2020

Copy link
Copy Markdown
Collaborator

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

@ronag

ronag commented Feb 9, 2020

Copy link
Copy Markdown
MemberAuthor

@addaleax: rebased, approve?

@ronag
ronag requested a review from addaleaxFebruary 14, 2020 09:38
@ronagronag 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

ronag commented Feb 24, 2020

Copy link
Copy Markdown
MemberAuthor

rebased to fix conflicts

@nodejs-github-bot

nodejs-github-bot commented Feb 24, 2020

Copy link
Copy Markdown
Collaborator

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

@ronag

ronag commented Feb 28, 2020

Copy link
Copy Markdown
MemberAuthor

rebased to fix conflicts

@ronag
ronagforce-pushed the eos-closed-ended branch 2 times, most recently from 07c6efd to 6a90b56CompareFebruary 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

ronag commented Feb 29, 2020

Copy link
Copy Markdown
MemberAuthor

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

@Trott

Trott commented Feb 29, 2020

Copy link
Copy Markdown
Member

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

nodejs-github-bot commented Mar 3, 2020

Copy link
Copy Markdown
Collaborator

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

@ronag

ronag commented Mar 3, 2020

Copy link
Copy Markdown
MemberAuthor

Landed in 7cafd5f

@ronagronag 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>
@BethGriggsBethGriggs mentioned this pull request Mar 10, 2020
Merged
@ronagronag mentioned this pull request Apr 25, 2020
Closed
4 tasks
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollinamcollinamcollina approved these changes
@TrottTrottTrott approved these changes
@mafintoshmafintoshAwaiting requested review from mafintosh
@jasnelljasnellAwaiting requested review from jasnell
@lpincalpincaAwaiting requested review from lpinca
@addaleaxaddaleaxAwaiting requested review from addaleax

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.notable-changePRs with changes that should be highlighted in changelogs.semver-majorPRs that contain breaking changes and should be released in the next major version.streamIssues and PRs related to the stream subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@ronag@nodejs-github-bot@Trott@addaleax@mcollina