Skip to content

stream: writable write emit all errors async - #29744

Closed
ronag wants to merge 5 commits into
nodejs:masterfrom
nxtedition:stream-writable-async-errors
Closed

stream: writable write emit all errors async#29744
ronag wants to merge 5 commits into
nodejs:masterfrom
nxtedition:stream-writable-async-errors

Conversation

@ronag

@ronag ronag commented Sep 28, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

Currently write sometimes throws/emits synchronously and sometimes asynchronously. Also, the callback is sometimes called before and sometimes after the 'error'.

This PR brings consistency in "correctly" emitting all errors in write asynchronously after the callback.

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

@nodejs-github-bot nodejs-github-bot added the stream Issues and PRs related to Node.js streams. label Sep 28, 2019
@ronag
ronag force-pushed the stream-writable-async-errors branch 3 times, most recently from cc09dcf to 37e138b Compare September 28, 2019 07:04
@ronag ronag changed the title stream: writable write emit error async stream: writable write emit all errors async Sep 28, 2019
@ronag
ronag force-pushed the stream-writable-async-errors branch 3 times, most recently from bfc0730 to bccf4b3 Compare September 28, 2019 14:15
@nodejs-github-bot

nodejs-github-bot commented Oct 1, 2019

Copy link
Copy Markdown
Collaborator

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

@Trott

Trott commented Oct 4, 2019

Copy link
Copy Markdown
Member

@nodejs/streams

@ronag

ronag commented Oct 4, 2019

Copy link
Copy Markdown
Member Author

@benjamingr

mcollina
mcollina reviewed Oct 5, 2019
View reviewed changes

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

I agree in principle, but I fear this might break some code.

Comment thread lib/internal/streams/destroy.js Outdated
@mcollina

mcollina commented Oct 5, 2019

Copy link
Copy Markdown
Member

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

@ronag

ronag commented Oct 5, 2019 •
edited
Loading

Copy link
Copy Markdown
Member Author

New failure after pushing cosmetic fixup. I guess Travis does a rebase when running and something new in master made it fail. Investigating.

@ronag

ronag commented Oct 5, 2019

Copy link
Copy Markdown
Member Author

rebased and fixed test

@ronag
ronag force-pushed the stream-writable-async-errors branch from fc535b6 to b5ee8a9 Compare October 5, 2019 11:15
mcollina
mcollina approved these changes Oct 15, 2019
View reviewed changes

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

LGTM

@mcollina mcollina added the semver-major PRs that contain breaking changes and should be released in the next major version. label Oct 15, 2019
@ronag

ronag commented Nov 11, 2019 •
edited
Loading

Copy link
Copy Markdown
Member Author

@Trott @benjamingr This needs another review, ping nodejs/streams?

@Trott

Trott commented Nov 12, 2019

Copy link
Copy Markdown
Member

@nodejs/streams

@Trott

Trott commented Nov 12, 2019

Copy link
Copy Markdown
Member

/ping @nodejs/tsc this needs another review

@ronag

ronag commented Dec 1, 2019

Copy link
Copy Markdown
Member Author

@mcollina: I've brought this up to date with recent changes in master. Please take another look.

@ronag
ronag requested a review from mcollina December 1, 2019 11:11
@ronag
ronag force-pushed the stream-writable-async-errors branch 2 times, most recently from ce8b7b7 to 35225e4 Compare December 1, 2019 11:13
mcollina
mcollina approved these changes Dec 1, 2019
View reviewed changes

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

Still LGTM

@ronag
ronag force-pushed the stream-writable-async-errors branch 2 times, most recently from e2a49e4 to 3e59684 Compare December 1, 2019 11:34
@ronag
ronag force-pushed the stream-writable-async-errors branch from 3e59684 to 6708f17 Compare December 1, 2019 11:35
ronag
ronag commented Dec 21, 2019
View reviewed changes
Comment thread lib/_stream_writable.js Outdated
@lundibundi lundibundi mentioned this pull request Dec 23, 2019
Closed
4 tasks done
@ronag ronag mentioned this pull request Dec 25, 2019
Closed
4 tasks done
@ronag

ronag commented Dec 30, 2019

Copy link
Copy Markdown
Member Author

@Trott: Another CITGM?

@nodejs-github-bot

nodejs-github-bot commented Dec 30, 2019 •
edited by lundibundi
Loading

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/28022/
CITGM: https://ci.nodejs.org/job/citgm-smoker/2167/

@Trott

Trott commented Dec 30, 2019

Copy link
Copy Markdown
Member

CI: https://ci.nodejs.org/job/node-test-pull-request/28022/
CITGM: https://ci.nodejs.org/job/citgm-smoker/2167/

CITGM looks good. CI does not, but I think that's a problem on CI right now and not a problem with this PR. I'll ping the other Build folks via IRC or an issue in the build repo....

ronag added 3 commits January 1, 2020 13:35
@ronag
stream: 'error' should be emitted asynchronously
6d8dcbc 
errorOrDestroy emits 'error' synchronously due to
compat reasons. However, it should be possible to
use correct async behaviour for new code.
@ronag
fixup: slightly refactor
9c52e42
@ronag
fixup: async ERR_METHOD_NOT_IMPLEMENTED
909706d
@ronag
ronag force-pushed the stream-writable-async-errors branch from 589e4e2 to 909706d Compare January 1, 2020 12:36
@ronag

ronag commented Jan 1, 2020

Copy link
Copy Markdown
Member Author

Rebased to fix conflicts. Needs another CI.

BridgeAR
BridgeAR reviewed Jan 1, 2020
View reviewed changes
Comment thread test/parallel/test-zlib-write-after-close.js Outdated
ronag
ronag commented Jan 1, 2020
View reviewed changes
Comment thread test/parallel/test-zlib-object-write.js Outdated
Comment thread test/parallel/test-zlib-object-write.js Outdated
Comment thread test/parallel/test-zlib-write-after-close.js Outdated
Comment thread test/parallel/test-zlib-write-after-close.js Outdated
Comment thread test/parallel/test-net-socket-write-error.js Outdated
Comment thread test/parallel/test-net-socket-write-error.js Outdated
Comment thread test/parallel/test-net-write-arguments.js Outdated
Comment thread test/parallel/test-net-write-arguments.js Outdated
@ronag

ronag commented Jan 1, 2020

Copy link
Copy Markdown
Member Author

CI

@nodejs-github-bot

nodejs-github-bot commented Jan 1, 2020

Copy link
Copy Markdown
Collaborator

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

@ronag

ronag commented Jan 4, 2020

Copy link
Copy Markdown
Member Author

@BridgeAR: Is this waiting for anything at the moment or is it just baking?

@BridgeAR

BridgeAR commented Jan 4, 2020

Copy link
Copy Markdown
Member

The CI must be green. Our process is relatively well documented. I suggest you could just check some of our markdown files :)

@BridgeAR

BridgeAR commented Jan 4, 2020

Copy link
Copy Markdown
Member

Since this is semver-major it also requires two TSC LGs.

@ronag

ronag commented Jan 4, 2020 •
edited
Loading

Copy link
Copy Markdown
Member Author

The CI must be green.

I've never seen all CI's green (nor CITGM) and I don't quite understand the output enough to determine what is relevant and "good enough". I'll see if I can ask someone to explain it to me.

Since this is semver-major it also requires two TSC LGs.

Ah yes, this I did know. Maybe @addaleax?

@nodejs-github-bot

nodejs-github-bot commented Jan 4, 2020

Copy link
Copy Markdown
Collaborator

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

@addaleax addaleax added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Jan 4, 2020
Trott pushed a commit that referenced this pull request Jan 5, 2020
@ronag @Trott
stream: emit 'error' asynchronously
75b30c6 
errorOrDestroy emits 'error' synchronously due to
compat reasons. However, it should be possible to
use correct async behaviour for new code.

PR-URL: #29744
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@Trott

Trott commented Jan 5, 2020

Copy link
Copy Markdown
Member

Landed in 75b30c6

@Trott Trott closed this Jan 5, 2020
@BethGriggs BethGriggs mentioned this pull request Mar 10, 2020
Merged
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
@BridgeAR BridgeAR BridgeAR left review comments
@Trott Trott Trott approved these changes
@addaleax addaleax addaleax approved these changes
@benjamingr benjamingr benjamingr approved these changes
+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 with CI started, the required approvals, and no outstanding review comments. semver-major PRs that contain breaking changes and should be released in the next major version. stream Issues and PRs related to Node.js streams.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@ronag @nodejs-github-bot @Trott @mcollina @BridgeAR @addaleax @benjamingr @lundibundi