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: writable write emit all errors async #29744

Closed
wants to merge 5 commits into from
Closed

stream: writable write emit all errors async #29744

wants to merge 5 commits into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Sep 28, 2019 •
edited
Loading

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 the stream subsystem. 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
Copy link
Collaborator

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

@Trott
Copy link
Member

Trott commented Oct 4, 2019

@nodejs/streams

@ronag
Copy link
Member Author

ronag commented Oct 4, 2019

@benjamingr

mcollina
mcollina reviewed Oct 5, 2019
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.

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

lib/internal/streams/destroy.js Outdated Show resolved Hide resolved
@mcollina
Copy link
Member

mcollina commented Oct 5, 2019

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

@ronag
Copy link
Member Author

ronag commented Oct 5, 2019 •
edited
Loading

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

@ronag
Copy link
Member Author

ronag commented Oct 5, 2019

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

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

ronag commented Nov 11, 2019 •
edited
Loading

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

@Trott
Copy link
Member

Trott commented Nov 12, 2019

@nodejs/streams

@Trott
Copy link
Member

Trott commented Nov 12, 2019

/ping @nodejs/tsc this needs another review

@ronag
Copy link
Member Author

ronag commented Dec 1, 2019

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

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
@lundibundi lundibundi mentioned this pull request Dec 23, 2019
Closed
4 tasks
@ronag ronag mentioned this pull request Dec 25, 2019
Closed
4 tasks
@ronag
Copy link
Member Author

ronag commented Dec 30, 2019

@Trott: Another CITGM?

@nodejs-github-bot
Copy link
Collaborator

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

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

@Trott
Copy link
Member

Trott commented Dec 30, 2019

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

ronag commented Jan 1, 2020

Rebased to fix conflicts. Needs another CI.

ronag
ronag commented Jan 1, 2020
View reviewed changes
test/parallel/test-zlib-object-write.js Outdated Show resolved Hide resolved
test/parallel/test-zlib-object-write.js Outdated Show resolved Hide resolved
test/parallel/test-zlib-write-after-close.js Outdated Show resolved Hide resolved
test/parallel/test-zlib-write-after-close.js Outdated Show resolved Hide resolved
test/parallel/test-net-socket-write-error.js Outdated Show resolved Hide resolved
test/parallel/test-net-socket-write-error.js Outdated Show resolved Hide resolved
test/parallel/test-net-write-arguments.js Outdated Show resolved Hide resolved
test/parallel/test-net-write-arguments.js Outdated Show resolved Hide resolved
@ronag
Copy link
Member Author

ronag commented Jan 1, 2020

CI

@nodejs-github-bot
Copy link
Collaborator

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

@ronag
Copy link
Member Author

ronag commented Jan 4, 2020

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

@BridgeAR
Copy link
Member

BridgeAR commented Jan 4, 2020

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

@BridgeAR
Copy link
Member

BridgeAR commented Jan 4, 2020

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

@ronag
Copy link
Member Author

ronag commented Jan 4, 2020 •
edited
Loading

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

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

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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 <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@Trott
Copy link
Member

Trott commented Jan 5, 2020

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

@lundibundi lundibundi lundibundi left review comments

@BridgeAR BridgeAR BridgeAR left review comments

@mcollina mcollina mcollina approved these changes

@benjamingr benjamingr benjamingr approved these changes

@addaleax addaleax addaleax approved these changes

@Trott Trott Trott approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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
@ronag @nodejs-github-bot @Trott @mcollina @BridgeAR @addaleax @benjamingr @lundibundi