FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

stream: don't flush destroyed writable by ronag · Pull Request #29028 · nodejs/node · GitHub

/ node Public

stream: don't flush destroyed writable - #29028

Closed
ronag wants to merge 4 commits into
nodejs:masterfrom
nxtedition:stream-flush-destroýed-writable

Hidden character warning

The head ref may contain hidden characters: "stream-flush-destro\u00fded-writable"
ConversationCommits (4)ChecksFiles changed
Closed

stream: don't flush destroyed writable#29028
ronag wants to merge 4 commits into
nodejs:masterfrom
nxtedition:stream-flush-destroýed-writable

Conversation

ronag commented Aug 6, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

It doesn't make much sense (and is a bit weird) to flush a stream which has been destroyed.

I need help creating a relevant test for this.

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

NOTE TO SELF: Look into needFinish & stream destroyed. errorOrDestroy added in this PR should preferably be async.

nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Aug 6, 2019
ronag force-pushed the stream-flush-destroýed-writable branch 4 times, most recently from 3b3305a to 481e6d1 Compare August 6, 2019 20:20
ronag mentioned this pull request Aug 6, 2019
Closed
4 tasks
mscdex reviewed Aug 6, 2019
View reviewed changes
Comment thread lib/_stream_writable.js Outdated
ronag force-pushed the stream-flush-destroýed-writable branch from 481e6d1 to 44d3d57 Compare August 6, 2019 23:00
jasnell requested review from mafintosh and mcollina August 7, 2019 00:26

jasnell commented Aug 7, 2019

Copy link
Copy Markdown
Member

Hmm.. I'm thinking this is more bug fix than breaking change. What do you think @mcollina?

mcollina requested changes Aug 7, 2019
View reviewed changes

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This will need a unit test.

I'm happy to land this as a bug fix as long as it does not break anything on CITGM.

mafintosh commented Aug 7, 2019

Copy link
Copy Markdown
Member

def bugfix

nodejs-github-bot commented Aug 7, 2019

Copy link
Copy Markdown
Collaborator

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

ronag force-pushed the stream-flush-destroýed-writable branch from 44d3d57 to 7332cb5 Compare August 7, 2019 09:50

ronag commented Aug 7, 2019

Copy link
Copy Markdown
Member Author

test added and fixed

ronag force-pushed the stream-flush-destroýed-writable branch from 7332cb5 to 0906aa6 Compare August 7, 2019 09:56
ronag commented Aug 7, 2019
View reviewed changes
Comment thread lib/_stream_writable.js Outdated
ronag force-pushed the stream-flush-destroýed-writable branch 2 times, most recently from f0ea5f3 to 372588a Compare August 7, 2019 09:58
mcollina reviewed Aug 7, 2019
View reviewed changes

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I would still consider this a bugfix, but we should:

  1. verify that CITGM passes
  2. let it bake for LTS (10.x) for some time before backporting.

Comment thread test/parallel/test-stream-write-destroy.js Outdated

nodejs-github-bot commented Aug 7, 2019

Copy link
Copy Markdown
Collaborator

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

mcollina commented Aug 7, 2019

Copy link
Copy Markdown
Member

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1932/

ronag force-pushed the stream-flush-destroýed-writable branch from 372588a to e1542b7 Compare August 7, 2019 12:44

ronag commented Aug 7, 2019 •
edited
Loading

Copy link
Copy Markdown
Member Author

@mcollina: Sorry, found further issues.

Consider the updated tests. We now will throw ERR_STREAM_DESTROYED on some tests that assume ERR_STREAM_WRITE_AFTER_END also we now send the err on the write callback and don't emit it as an error on the stream.

How should this be?

addaleax reviewed Aug 7, 2019
View reviewed changes

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I wouldn’t mind being careful and labelling this semver-major.

Comment thread lib/_stream_writable.js Outdated
ronag force-pushed the stream-flush-destroýed-writable branch from e1542b7 to d7e8b4c Compare August 7, 2019 14:14

ronag commented Aug 7, 2019 •
edited
Loading

Copy link
Copy Markdown
Member Author

Ok, I think everything (expect @mcollina's comment on !err && chinksWritten++) is fixed in a sensical manner.

ronag mentioned this pull request Aug 8, 2019
Closed
This was referenced Jan 20, 2021
Open
Open
Open
Open
Open
Open
This was referenced Sep 5, 2021
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Closed
kissofire mentioned this pull request Jan 1, 2024
Open
aliceUnhinged613 mentioned this pull request Jan 1, 2024
Open
137717unity mentioned this pull request Jan 2, 2024
Open
molllyn1 mentioned this pull request Jan 2, 2024
Open
wedataintelligence mentioned this pull request Jan 2, 2024
Open
ron-a20 mentioned this pull request Jan 2, 2024
Open
molllyn1 mentioned this pull request Mar 15, 2024
Open
ron-a20 mentioned this pull request Mar 15, 2024
Open
kissofire mentioned this pull request Mar 15, 2024
Open
aliceUnhinged613 mentioned this pull request Mar 15, 2024
Open
137717unity mentioned this pull request Mar 16, 2024
Open
wedataintelligence mentioned this pull request Mar 18, 2024
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Trott Trott left review comments
mcollina mcollina approved these changes
jasnell jasnell approved these changes
addaleax addaleax approved these changes
mafintosh Awaiting requested review from mafintosh
+1 more reviewer
mscdex mscdex left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

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.

8 participants

Back | FazBrowse Home | New Git URL