stream: use callback to properly propagate error by ronag · Pull Request #29179 · nodejs/node

ronag · 2019-08-17T11:12:30Z

The stream will be destroyed upstream through the proper error flow.

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

lib/internal/stream_base_commons.js

Trott · 2019-08-19T16:43:53Z

@nodejs/streams

nodejs-github-bot · 2019-08-19T16:44:29Z

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

mcollina · 2019-08-19T17:07:40Z

Can you add a unit test for this?

Trott

This appears to break parallel/test-tls-hello-parser-failure on macOS, SmartOS, and possibly others.

Trott · 2019-08-19T17:13:06Z

Here's the test failing on FreeBSD. It also failed on macOS and SmartOS. Might be failing elsewhere too, as CI is still running....

AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:

false !== true

    at Socket.<anonymous> (/usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd11-x64/test/parallel/test-tls-hello-parser-failure.js:64:12)
    at Socket.<anonymous> (/usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd11-x64/test/common/index.js:371:15)
    at Socket.emit (events.js:209:13)
    at TCP.<anonymous> (net.js:588:12) {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: false,
  expected: true,
  operator: 'strictEqual'
}

ronag · 2019-08-19T19:04:51Z

@mcollina: This is a bit over my head in terms of unit tests... I think the observed behaviour will be the same. It's just more "correct" in terms of flow.

ronag · 2019-08-20T23:13:14Z

@Trott: tests pass now, this needed a little extra fiddling to follow the "proper" stream logic.

In particular we need the socket to be automatically destroyed on error. This also means that the tests can't expect the socket to continue to function once an error has been caused. I had to split one of those test into two separate files.

This probably needs a semver-major...

test fixed

nodejs-github-bot · 2019-08-21T03:17:19Z

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

lib/_stream_writable.js

lib/internal/stream_base_commons.js

test/parallel/test-net-connect-buffer.js

test/parallel/test-net-connect-buffer2.js

nodejs-github-bot · 2019-08-21T21:44:38Z

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

ronag · 2020-01-20T08:56:13Z

rebased

nodejs-github-bot · 2020-01-24T23:01:20Z

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

nodejs-github-bot · 2020-01-25T20:01:54Z

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

ronag · 2020-01-26T07:29:03Z

@Trott: Can I get a second opinion on the CI failures on Windows. They seem legit don't they?

ronag · 2020-01-28T07:03:14Z

Anyone on Windows that might be able to help out debugging the Windows timeouts?

nodejs-github-bot · 2020-02-01T11:09:23Z

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

nodejs-github-bot · 2020-02-01T11:53:40Z

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

nodejs-github-bot · 2020-02-02T00:16:35Z

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

Trott · 2020-02-05T19:23:26Z

@Trott: Can I get a second opinion on the CI failures on Windows. They seem legit don't they?

They do seem legit. After we get the security release out and the CI comes out of embargo, we can try a full rebuild to make sure.

nodejs-github-bot · 2020-02-08T22:29:40Z

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

ronag · 2020-02-17T15:58:54Z

This needs help with sorting out Windows failures.

ronag · 2020-02-17T23:47:14Z

This is blocked by #31806 in order to sort out Windows failures

nodejs-github-bot · 2020-02-18T23:48:55Z

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

The stream will be destroyed upstream through the proper error flow.

nodejs-github-bot · 2020-04-03T08:13:10Z

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

ronag · 2020-04-03T08:13:21Z

I think this might finally be landable, @mcollina @jasnell @BridgeAR PTAL

nodejs-github-bot · 2020-04-03T08:42:39Z

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

mcollina

lgtm

ronag · 2020-04-03T17:01:21Z

Landed in 8f86986

ronag force-pushed the stream-use-callback branch from 0068306 to d9fc519 Compare August 17, 2019 11:17

ronag mentioned this pull request Aug 17, 2019

stream: destroy with callback #29180

Closed

5 tasks

mscdex reviewed Aug 17, 2019

View reviewed changes

lib/internal/stream_base_commons.js Outdated Show resolved Hide resolved

ronag force-pushed the stream-use-callback branch from d9fc519 to 7018652 Compare August 17, 2019 14:55

Trott previously requested changes Aug 19, 2019

View reviewed changes

ronag force-pushed the stream-use-callback branch 7 times, most recently from 20fac88 to bef7095 Compare August 20, 2019 23:38

mcollina added net Issues and PRs related to the net subsystem. 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. labels Aug 21, 2019

mcollina reviewed Aug 21, 2019

View reviewed changes

lib/_stream_writable.js Outdated Show resolved Hide resolved

lib/_stream_writable.js Outdated Show resolved Hide resolved

lib/internal/stream_base_commons.js Outdated Show resolved Hide resolved

test/parallel/test-net-connect-buffer.js Outdated Show resolved Hide resolved

jasnell reviewed Aug 21, 2019

View reviewed changes

test/parallel/test-net-connect-buffer2.js Outdated Show resolved Hide resolved

jasnell reviewed Aug 21, 2019

View reviewed changes

test/parallel/test-net-connect-buffer2.js Outdated Show resolved Hide resolved

ronag force-pushed the stream-use-callback branch 3 times, most recently from c8e66f6 to ef9e30f Compare August 21, 2019 22:08

ronag force-pushed the stream-use-callback branch from 7a9f01e to 6c9b39b Compare January 20, 2020 08:56

ronag added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 24, 2020

ronag removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 26, 2020

ronag added the blocked PRs that are blocked by other issues or PRs. label Feb 17, 2020

ronag force-pushed the stream-use-callback branch from 6c9b39b to a89f557 Compare February 18, 2020 23:40

This comment has been minimized.

Sign in to view

ronag force-pushed the stream-use-callback branch from a89f557 to 945c44b Compare February 18, 2020 23:48

ronag mentioned this pull request Mar 1, 2020

stream: make stream.destroy with callback API public #32021

Closed

4 tasks

stream: use callback to properly propagate error

6b86519

The stream will be destroyed upstream through the proper error flow.

ronag force-pushed the stream-use-callback branch from 945c44b to 6b86519 Compare April 3, 2020 06:42

mcollina approved these changes Apr 3, 2020

View reviewed changes

jasnell approved these changes Apr 3, 2020

View reviewed changes

BethGriggs mentioned this pull request Sep 1, 2020

2020-10-20, Version 15.0.0 (Current) #35014

Merged

Uh oh!

Conversation

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

Uh oh!

Checklist

Uh oh!

Uh oh!

Trott commented Aug 19, 2019

Uh oh!

nodejs-github-bot commented Aug 19, 2019

Uh oh!

mcollina commented Aug 19, 2019

Uh oh!

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

Trott commented Aug 19, 2019

Uh oh!

ronag commented Aug 19, 2019

Uh oh!

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

Uh oh!

Uh oh!

nodejs-github-bot commented Aug 21, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Aug 21, 2019

Uh oh!

ronag commented Jan 20, 2020

Uh oh!

nodejs-github-bot commented Jan 24, 2020

Uh oh!

nodejs-github-bot commented Jan 25, 2020

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

nodejs-github-bot commented Feb 1, 2020

Uh oh!

nodejs-github-bot commented Feb 1, 2020

Uh oh!

nodejs-github-bot commented Feb 2, 2020

Uh oh!

Trott commented Feb 5, 2020

Uh oh!

nodejs-github-bot commented Feb 8, 2020

Uh oh!

ronag commented Feb 17, 2020

Uh oh!

ronag commented Feb 17, 2020

Uh oh!

This comment has been minimized.

nodejs-github-bot commented Feb 18, 2020

Uh oh!

nodejs-github-bot commented Apr 3, 2020

Uh oh!

ronag commented Apr 3, 2020

Uh oh!

nodejs-github-bot commented Apr 3, 2020 • edited by ronag Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

ronag commented Apr 3, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

ronag commented Aug 17, 2019 •

edited

Loading

ronag commented Aug 20, 2019 •

edited

Loading

ronag commented Jan 26, 2020 •

edited

Loading

ronag commented Jan 28, 2020 •

edited

Loading

nodejs-github-bot commented Apr 3, 2020 •

edited by ronag

Loading