stream: writable write emit all errors async by ronag · Pull Request #29744 · nodejs/node

ronag · 2019-09-28T06:36:07Z

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 · 2019-10-01T03:57:52Z

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

Trott · 2019-10-04T02:17:58Z

@nodejs/streams

ronag · 2019-10-04T20:17:29Z

@benjamingr

mcollina

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

mcollina · 2019-10-05T10:17:15Z

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

ronag · 2019-10-05T11:12:23Z

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

ronag · 2019-10-05T11:14:57Z

rebased and fixed test

mcollina

LGTM

ronag · 2019-11-11T17:12:45Z

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

Trott · 2019-11-12T03:07:05Z

@nodejs/streams

Trott · 2019-11-12T03:07:19Z

/ping @nodejs/tsc this needs another review

ronag · 2019-12-01T11:11:14Z

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

mcollina

Still LGTM

ronag · 2019-12-30T13:19:06Z

@Trott: Another CITGM?

nodejs-github-bot · 2019-12-30T16:45:15Z

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

Trott · 2019-12-30T19:57:52Z

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

errorOrDestroy emits 'error' synchronously due to compat reasons. However, it should be possible to use correct async behaviour for new code.

ronag · 2020-01-01T12:36:09Z

Rebased to fix conflicts. Needs another CI.

ronag · 2020-01-01T13:37:13Z

CI

nodejs-github-bot · 2020-01-01T13:45:34Z

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

ronag · 2020-01-04T17:00:24Z

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

BridgeAR · 2020-01-04T17:19:37Z

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

BridgeAR · 2020-01-04T17:20:27Z

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

ronag · 2020-01-04T17:28:44Z

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 · 2020-01-04T17:34:30Z

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

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 · 2020-01-05T00:22:01Z

Landed in 75b30c6

nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Sep 28, 2019

ronag force-pushed the stream-writable-async-errors branch 3 times, most recently from cc09dcf to 37e138b Compare September 28, 2019 07:04

ronag changed the title ~~stream: writable write emit error async~~ stream: writable write emit all errors async Sep 28, 2019

ronag force-pushed the stream-writable-async-errors branch 3 times, most recently from bfc0730 to bccf4b3 Compare September 28, 2019 14:15

mcollina reviewed Oct 5, 2019

View reviewed changes

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

ronag force-pushed the stream-writable-async-errors branch from fc535b6 to b5ee8a9 Compare October 5, 2019 11:15

mcollina approved these changes Oct 15, 2019

View reviewed changes

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

ronag requested a review from mcollina December 1, 2019 11:11

ronag force-pushed the stream-writable-async-errors branch 2 times, most recently from ce8b7b7 to 35225e4 Compare December 1, 2019 11:13

mcollina approved these changes Dec 1, 2019

View reviewed changes

ronag force-pushed the stream-writable-async-errors branch 2 times, most recently from e2a49e4 to 3e59684 Compare December 1, 2019 11:34

Trott force-pushed the master branch from 5473ceb to 7da6630 Compare December 1, 2019 11:35

ronag force-pushed the stream-writable-async-errors branch from 3e59684 to 6708f17 Compare December 1, 2019 11:35

ronag commented Dec 21, 2019

View reviewed changes

Comment thread lib/_stream_writable.js Outdated

lundibundi mentioned this pull request Dec 23, 2019

stream: refactor Writable buffering #31046

Closed

4 tasks

ronag mentioned this pull request Dec 25, 2019

stream: pipeline should use req.abort() to destroy response #31054

Closed

4 tasks

ronag added 3 commits January 1, 2020 13:35

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.

fixup: slightly refactor

9c52e42

fixup: async ERR_METHOD_NOT_IMPLEMENTED

909706d

ronag force-pushed the stream-writable-async-errors branch from 589e4e2 to 909706d Compare January 1, 2020 12:36

BridgeAR reviewed Jan 1, 2020

View reviewed changes

Comment thread test/parallel/test-zlib-write-after-close.js Outdated

fixup

f17a894

ronag commented Jan 1, 2020

View reviewed changes

Apply suggestions from code review

1869cde

addaleax approved these changes Jan 4, 2020

View reviewed changes

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 approved these changes Jan 5, 2020

View reviewed changes

Trott closed this Jan 5, 2020

BethGriggs mentioned this pull request Mar 10, 2020

2020-04-21, Version 14.0.0 (Current) #32181

Merged

Uh oh!

Conversation

ronag commented Sep 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

nodejs-github-bot commented Oct 1, 2019

Uh oh!

Trott commented Oct 4, 2019

Uh oh!

ronag commented Oct 4, 2019

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mcollina commented Oct 5, 2019

Uh oh!

ronag commented Oct 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ronag commented Oct 5, 2019

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

ronag commented Nov 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott commented Nov 12, 2019

Uh oh!

Trott commented Nov 12, 2019

Uh oh!

ronag commented Dec 1, 2019

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ronag commented Dec 30, 2019

Uh oh!

nodejs-github-bot commented Dec 30, 2019 • edited by lundibundi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott commented Dec 30, 2019

Uh oh!

ronag commented Jan 1, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ronag commented Jan 1, 2020

Uh oh!

nodejs-github-bot commented Jan 1, 2020

Uh oh!

ronag commented Jan 4, 2020

Uh oh!

BridgeAR commented Jan 4, 2020

Uh oh!

BridgeAR commented Jan 4, 2020

Uh oh!

ronag commented Jan 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Jan 4, 2020

Uh oh!

Trott commented Jan 5, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

ronag commented Sep 28, 2019 •

edited

Loading

ronag commented Oct 5, 2019 •

edited

Loading

ronag commented Nov 11, 2019 •

edited

Loading

nodejs-github-bot commented Dec 30, 2019 •

edited by lundibundi

Loading

ronag commented Jan 4, 2020 •

edited

Loading