Migrate _stream_readable errors to internal/errors by benhalverson · Pull Request #15042 · nodejs/node · GitHub

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows [commit guidelines]

Affected core subsystem(s)

_stream_readable

@jasnell I wasn't able to get the tests to pass...

Path: parallel/test-stream-unshift-read-race
ok
assert.js:621
      throw actual;
      ^

Error [ERR_STREAM_PUSH_AFTER_EOF]: stream.push() after EOF
    at readableAddChunk (_stream_readable.js:244:30)
    at Readable.push (_stream_readable.js:212:10)
    at /Users/benhalverson/projects/node/test/parallel/test-stream-unshift-read-race.js:72:7
    at tryBlock (assert.js:591:5)
    at innerThrows (assert.js:610:18)
    at Function.throws (assert.js:637:3)
    at pushError (/Users/benhalverson/projects/node/test/parallel/test-stream-unshift-read-race.js:71:10)
    at Timeout._onTimeout (/Users/benhalverson/projects/node/test/parallel/test-stream-unshift-read-race.js:64:25)
    at ontimeout (timers.js:469:11)
    at tryOnTimeout (timers.js:304:5)
Command: out/Release/node /Users/benhalverson/projects/node/test/parallel/test-stream-unshift-read-race.js
[02:20|% 100|+ 1799|-   1]: Done
make: *** [test] Error 1

my attempt...

function pushError() {
  assert.throws(function() {
    r.push(Buffer.allocUnsafe(1));
  }, /^ERR_STREAM_PUSH_AFTER_EOF$/);
}


const w = stream.Writable();
const written = [];
w._write = function(chunk, encoding, cb) {
  written.push(chunk.toString());
  cb();
};

r.on('end', common.mustCall(function() {
  assert.throws(function() {
    r.unshift(Buffer.allocUnsafe(1));
  }, /^ERR_STREAM_UNSHIFT_AFTER_END_EVENT$/);
  w.end();
}));

Ping @nodejs/streams @mcollina

As stated elsewhere, I'm very 👎 on this change until we have a clear strategy on how to keep the same behavior on readable-stream (it needs to support old browsers were inheriting from Error does not work as expected).

@mcollina thanks for the feedback.

Hello @benhalverson and welcome. Thank you for the contribution 🥇
AFAICT on the face of it this PR is good, it's just a matter of timing and coordination with the readable-stream package.

I've marked it as blocked until a compatibility strategy is finalized.

Can you please put this in alphabetical order :-) thank you!
Also, I believe these need line wrapped at 80 chars. Run make lint to check

s/ERR_UNDERSCORE_READ_NOT_IMPLEMENTED/ERR_STREAM_READ_NOT_IMPLEMENTED

s/ERR_END_READABLE_CALLED_ON_NONEMPTY_STREAM/ERR_STREAM_READABLE_CALLED_ON_NONEMPTY

To explain @refack's comments a bit more. The streams module in core is kept in sync with the standalone readable-streams module. Because of that, any changes to the streams code must be kept in sync with that module. Because the internal/errors bit is very specific to Node.js core, we have not started migrating the streams code over to use it.

At the latest streams wg we decided to unblock this. We would like this (and the equals for Writable, Duplex and Transform) to ship in Node 9.

Ref: nodejs/readable-stream#309 (comment)

@benhalverson I think you can use common.expectsError to check for the error code: https://github.com/nodejs/node/blob/master/test/common/index.js#L696.

See https://github.com/nodejs/node/blob/c2e838ee13244dd3dbd98ed86ef1966fe7cd90f1/test/parallel/test-require-invalid-package.js as an example.

Thanks @mcollina I'll take a look at the example

@benhalverson what is the status of this? Would you mind updating to common.expectsError({ code: 'CODE' })?

why is this commented out?

I was testing and forgot to uncomment

I could use some assistance on this test...
current test

function pushError() {
  assert.throws(function() {
    r.push(Buffer.allocUnsafe(1));
  }, /^ERR_STREAM_PUSH_AFTER_EOF/);
}

What I have tried so far:

function pushError() {
   common.expectsError({
     code: 'ERR_STREAM_PUSH_AFTER_EOF',
     message: 'stream.push() after EOF'
    });
  }

This gives the following error.

Path: parallel/test-stream-unshift-read-race
ok
events.js:182
      throw er; // Unhandled 'error' event
      ^

Error: stream.push() after EOF
    at readableAddChunk (_stream_readable.js:243:30)
    at Readable.push (_stream_readable.js:211:10)
    at pushError (/Users/benhalverson/projects/node/test/parallel/test-stream-unshift-read-race.js:77:9)
    at Timeout._onTimeout (/Users/benhalverson/projects/node/test/parallel/test-stream-unshift-read-race.js:64:25)
    at ontimeout (timers.js:471:11)
    at tryOnTimeout (timers.js:305:5)
    at Timer.listOnTimeout (timers.js:265:5)

Without r.push(Buffer.allocUnsafe(1)); I get this error.

Path: parallel/test-stream-unshift-read-race
Mismatched <anonymous> function calls. Expected exactly 1, actual 0.
    at Object.exports.mustCall (/Users/benhalverson/projects/node/test/common/index.js:482:10)
    at Object.expectsError (/Users/benhalverson/projects/node/test/common/index.js:707:27)
    at pushError (/Users/benhalverson/projects/node/test/parallel/test-stream-unshift-read-race.js:78:14)
    at Timeout._onTimeout (/Users/benhalverson/projects/node/test/parallel/test-stream-unshift-read-race.js:64:25)
    at ontimeout (timers.js:471:11)
    at tryOnTimeout (timers.js:305:5)
    at Timer.listOnTimeout (timers.js:265:5)
0: asdfasdfas
1: 1234dfasdf
2: 1234asdfas
3: 1234dfasdf
4: 1234asdfas
5: 1234dfasdf
6: 1234asdfas
7: 1234dfasdf
8: 1234asdfas
9: 1234dfasdf
a: 1234asdfas
b: 1234dfasdf
c: 1234asdfas
d: 1234dfasdf
e: 1234asdfas
f: 1234dfasdf
g: 1234asdfa
h: 1234

I figured it out @mcollina. Reading the docs helped 👍 https://github.com/nodejs/node/blob/master/doc/guides/using-internal-errors.md

Something went wrong with your latest git operations. Can you please rebase?

I would recommend that you take the changes you have done, start with a fresh branch, and do a force push here.

I was thinking the same... I did the rebase got a bunch of conflicts and it ended up like this after I fixed the conflicts ¯_(ツ)_/¯

@benhalverson against what branch did you rebase? I am not sure how you set up your clone but I guess you should rebase like git rebase -i upstream/master.

@BridgeAR I used my local version of node and rebased against upstream/master.
After I committed my local changes I typed git rebase upstream master git applied all the upstream changes then added mine and also showed a bunch of conflicts on files I didn't touch. I used my IDE (webstorm) to resolve the conflicts by accepting their changes and then pushed to my local branch.

can you use common.expectsError here as well?

I've updated this test

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

LGTM with a nit

This should be ERR_STREAM_ENDREADABLE_CALLED_ON_NONEMPTY.

I've updated this line

There is the error code to be changed, I'm moving my feedback to "request changes" instead of approved.

Please remove the empty template string.

Just as a suggestion - it would be nicer to use

common.expectsError(throwingFn, errorObj)
// instead of
assert.throws(throwingFn, common.expectsError(errorObj))

A empty template string?

Oops 😂

@mcollina is there anything else I need to change?

LGTM

CI: https://ci.nodejs.org/job/node-test-commit/12849/

LGTM

Looks like the CI job had conflicts even though the PR says it can be merge automatically.

@benhalverson could you squash the commits down 1 one (I'm thinking maybe something that is added/them removed is the source of the conflicts) and then we can run the CI again.

I have went ahead and squashed the commits in this PR. CI: https://ci.nodejs.org/job/node-test-pull-request/11041/

Oh, this feature is nice:

https://ci.nodejs.org/job/node-test-linter/13033/console

not ok 11 - /usr/home/iojs/build/workspace/node-test-linter/lib/internal/errors.js
  ---
  message: '"ERR_INVALID_URI" is not documented in doc/api/errors.md'
  severity: error
  data:
    line: 291
    column: 1
    ruleId: documented-errors
  messages:
    - message: doc/api/errors.md does not have an anchor for "ERR_INVALID_URI"
      severity: error
      data:
        line: 291
        column: 1
        ruleId: documented-errors
  ...

@benhalverson Do you have time to document this error code? If not I can do that, just don't want to delay this PR for too long.

@joyeecheung that error code is not present in readable. I think this should land asap before 9.

@mcollina Oh, I think I posted in the wrong thread, this error is from the querystring error migration PR. The errors for this one is

not ok 11 - /usr/home/iojs/build/workspace/node-test-linter/lib/internal/errors.js
  ---
  message: >-
    "ERR_STREAM_ENDREADABLE_CALLED_ON_NONEMPTY" is not documented in
    doc/api/errors.md
  severity: error
  data:
    line: 326
    column: 1
    ruleId: documented-errors
  messages:
    - message: >-
        doc/api/errors.md does not have an anchor for
        "ERR_STREAM_ENDREADABLE_CALLED_ON_NONEMPTY"
      severity: error
      data:
        line: 326
        column: 1
        ruleId: documented-errors
    - message: '"ERR_STREAM_PUSH_AFTER_EOF" is not documented in doc/api/errors.md'
      severity: error
      data:
        line: 328
        column: 1
        ruleId: documented-errors
    - message: doc/api/errors.md does not have an anchor for "ERR_STREAM_PUSH_AFTER_EOF"
      severity: error
      data:
        line: 328
        column: 1
        ruleId: documented-errors
    - message: '"ERR_STREAM_READ_NOT_IMPLEMENTED" is not documented in doc/api/errors.md'
      severity: error
      data:
        line: 329
        column: 1
        ruleId: documented-errors
    - message: >-
        doc/api/errors.md does not have an anchor for
        "ERR_STREAM_READ_NOT_IMPLEMENTED"
      severity: error
      data:
        line: 329
        column: 1
        ruleId: documented-errors
    - message: >-
        "ERR_STREAM_UNSHIFT_AFTER_END_EVENT" is not documented in
        doc/api/errors.md
      severity: error
      data:
        line: 330
        column: 1
        ruleId: documented-errors
    - message: >-
        doc/api/errors.md does not have an anchor for
        "ERR_STREAM_UNSHIFT_AFTER_END_EVENT"
      severity: error
      data:
        line: 330
        column: 1
        ruleId: documented-errors
  ...

Also there are some incorrect use of common.expectsError, I will fix those as well.

I have fixed the errors and added documentation. New CI: https://ci.nodejs.org/job/node-test-pull-request/11044/

@jasnell @BridgeAR @mcollina @addaleax @gireeshpunathil @mhdawson PTAL

FWIW

(CI failures are unrelated build issues.)

Landed as 88fb359

PR-URL: #15042 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

PR-URL: nodejs/node#15042 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. stream Issues and PRs related to the stream subsystem. labels Aug 26, 2017

refack added the blocked PRs that are blocked by other issues or PRs. label Aug 26, 2017

jasnell reviewed Aug 29, 2017

View reviewed changes

mcollina mentioned this pull request Sep 12, 2017

Stream WG meeting - September 2017 nodejs/readable-stream#309

Closed

mcollina removed the blocked PRs that are blocked by other issues or PRs. label Sep 12, 2017

mcollina reviewed Sep 18, 2017

View reviewed changes

mcollina reviewed Sep 25, 2017

View reviewed changes

mcollina approved these changes Sep 25, 2017

View reviewed changes

mcollina reviewed Sep 25, 2017

View reviewed changes

mcollina requested changes Sep 25, 2017

View reviewed changes

jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 25, 2017

benhalverson mentioned this pull request Oct 1, 2017

streams testing not passing nodejs/help#852

Closed

BridgeAR reviewed Oct 1, 2017

View reviewed changes

mcollina approved these changes Oct 6, 2017

View reviewed changes

mcollina requested a review from a team October 6, 2017 21:08

addaleax approved these changes Oct 7, 2017

View reviewed changes

mhdawson approved these changes Oct 11, 2017

View reviewed changes

stream: migrate _stream_readable use error codes

b635d1a

gireeshpunathil approved these changes Oct 28, 2017

View reviewed changes

joyeecheung added 3 commits October 28, 2017 18:56

fix common.expectsError usage in stream tests

eafcac4

delete removed error

cb38372

add documentations for stream errors

0ccc7e4

refack approved these changes Oct 28, 2017

View reviewed changes

gireeshpunathil approved these changes Oct 28, 2017

View reviewed changes

mcollina closed this Oct 29, 2017

jasnell mentioned this pull request Oct 30, 2017

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged

benhalverson deleted the stream_readable_error branch December 15, 2017 04:57

Migrate _stream_readable errors to internal/errors by benhalverson · Pull Request #15042 · nodejs/node · GitHub

Uh oh!

Uh oh!

Migrate _stream_readable errors to internal/errors - #15042

Migrate _stream_readable errors to internal/errors#15042benhalverson wants to merge 4 commits intonodejs:masternodejs/node:masterfrom benhalverson:stream_readable_errorCopy head branch name to clipboard

Conversation

benhalverson commented Aug 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Affected core subsystem(s)

Uh oh!

benhalverson commented Aug 26, 2017

Uh oh!

jasnell commented Aug 26, 2017

Uh oh!

mcollina commented Aug 26, 2017

Uh oh!

benhalverson commented Aug 26, 2017

Uh oh!

refack commented Aug 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Aug 29, 2017

Uh oh!

mcollina commented Sep 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollina commented Sep 12, 2017

Uh oh!

benhalverson commented Sep 12, 2017

Uh oh!

mcollina commented Sep 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcollina commented Sep 20, 2017

Uh oh!

mcollina commented Sep 21, 2017

Uh oh!

benhalverson commented Sep 21, 2017

Uh oh!

BridgeAR commented Sep 24, 2017

Uh oh!

benhalverson commented Sep 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcollina commented Sep 25, 2017

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benhalverson commented Oct 2, 2017

Uh oh!

Migrate _stream_readable errors to internal/errors#15042
benhalverson wants to merge 4 commits into
nodejs:masterfrom
benhalverson:stream_readable_error

benhalverson commented Aug 26, 2017 •
edited
Loading

mcollina commented Sep 12, 2017 •
edited
Loading