Migrate _stream_readable errors to internal/errors #15042

benhalverson · 2017-08-26T08:01:11Z

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

Affected core subsystem(s)

_stream_readable

benhalverson · 2017-08-26T08:53:41Z

@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();
}));

jasnell · 2017-08-26T14:21:51Z

Ping @nodejs/streams @mcollina

mcollina · 2017-08-26T15:51:34Z

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

benhalverson · 2017-08-26T16:15:56Z

@mcollina thanks for the feedback.

refack · 2017-08-26T17:56:35Z

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.

jasnell · 2017-08-29T04:29:40Z

lib/internal/errors.js

+E('ERR_STREAM_UNSHIFT_AFTER_END_EVENT', 'stream.unshift() after end event');
+E('ERR_STREAM_PUSH_AFTER_EOF', 'stream.push() after EOF');
+E('ERR_UNDERSCORE_READ_NOT_IMPLEMENTED', '_read() is not implemented');
+E('ERR_END_READABLE_CALLED_ON_NONEMPTY_STREAM', 'endReadable() called on a non-empty stream');


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

jasnell · 2017-08-29T04:30:26Z

lib/_stream_readable.js

@@ -551,7 +552,7 @@ function maybeReadMore_(stream, state) {
 // for virtual (non-string, non-buffer) streams, "length" is somewhat
 // arbitrary, and perhaps not very meaningful.
 Readable.prototype._read = function(n) {
-  this.emit('error', new Error('_read() is not implemented'));
+  this.emit('error', new errors.Error('ERR_UNDERSCORE_READ_NOT_IMPLEMENTED'));


s/ERR_UNDERSCORE_READ_NOT_IMPLEMENTED/ERR_STREAM_READ_NOT_IMPLEMENTED

jasnell · 2017-08-29T04:31:00Z

lib/_stream_readable.js

@@ -1043,7 +1044,7 @@ function endReadable(stream) {
  // If we get here before consuming all the bytes, then that is a
  // bug in node.  Should never happen.
  if (state.length > 0)
-    throw new Error('"endReadable()" called on non-empty stream');
+    throw new errors.Error('ERR_END_READABLE_CALLED_ON_NONEMPTY_STREAM');


s/ERR_END_READABLE_CALLED_ON_NONEMPTY_STREAM/ERR_STREAM_READABLE_CALLED_ON_NONEMPTY

jasnell · 2017-08-29T04:32:31Z

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.

mcollina · 2017-09-12T17:14:25Z

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)

mcollina · 2017-09-12T17:18:33Z

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

benhalverson · 2017-09-12T23:29:13Z

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

mcollina · 2017-09-18T15:10:38Z

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

mcollina · 2017-09-18T15:09:26Z

lib/internal/errors.js

@@ -270,6 +270,11 @@ E('ERR_SOCKET_CLOSED', 'Socket is closed');
 E('ERR_SOCKET_DGRAM_NOT_RUNNING', 'Not running');
 E('ERR_STDERR_CLOSE', 'process.stderr cannot be closed');
 E('ERR_STDOUT_CLOSE', 'process.stdout cannot be closed');
+E('ERR_STREAM_PUSH_AFTER_EOF', 'stream.push() after EOF');
+// E('ERR_STREAM_READ_NOT_IMPLEMENTED', '_read() is not implemented');


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

mcollina · 2017-09-20T09:44:43Z

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

mcollina · 2017-09-21T06:54:32Z

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

benhalverson · 2017-09-21T15:20:34Z

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 ¯_(ツ)_/¯

BridgeAR · 2017-09-24T15:40:17Z

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

benhalverson · 2017-09-25T06:00:37Z

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

mcollina · 2017-09-25T09:10:00Z

test/parallel/test-stream-readable-with-unimplemented-_read.js

@@ -5,4 +5,4 @@ const assert = require('assert');

 const readable = new stream.Readable();

-assert.throws(() => readable.read(), /not implemented/);
+assert.throws(() => readable.read(), /ERR_STREAM_READ_NOT_IMPLEMENTED/);


can you use common.expectsError here as well?

I've updated this test

mcollina · 2017-09-25T09:10:35Z

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

mcollina

LGTM with a nit

mcollina · 2017-09-25T09:11:28Z

lib/_stream_readable.js

@@ -1043,7 +1045,7 @@ function endReadable(stream) {
  // If we get here before consuming all the bytes, then that is a
  // bug in node.  Should never happen.
  if (state.length > 0)
-    throw new Error('"endReadable()" called on non-empty stream');
+    throw new errors.Error('ERR_STREAM_READABLE_CALLED_ON_NONEMPTY');


This should be ERR_STREAM_ENDREADABLE_CALLED_ON_NONEMPTY.

I've updated this line

mcollina

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

benhalverson · 2017-10-02T00:43:28Z

Oops 😂

benhalverson · 2017-10-04T06:06:55Z

@mcollina is there anything else I need to change?

mcollina

LGTM

addaleax · 2017-10-07T17:35:23Z

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

mhdawson

LGTM

mhdawson · 2017-10-11T18:11:12Z

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.

joyeecheung · 2017-10-28T08:38:13Z

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

joyeecheung · 2017-10-28T08:42:02Z

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.

mcollina · 2017-10-28T09:03:15Z

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

joyeecheung · 2017-10-28T10:55:07Z

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

joyeecheung · 2017-10-28T11:26:01Z

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

refack

FWIW

Trott · 2017-10-28T22:16:40Z

(CI failures are unrelated build issues.)

mcollina · 2017-10-29T11:30:07Z

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

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

Migrate _stream_readable errors to internal/errors #15042

Conversation

benhalverson commented Aug 26, 2017 • edited Loading

Affected core subsystem(s)

benhalverson commented Aug 26, 2017

jasnell commented Aug 26, 2017

mcollina commented Aug 26, 2017

benhalverson commented Aug 26, 2017

refack commented Aug 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Aug 29, 2017

mcollina commented Sep 12, 2017 • edited Loading

mcollina commented Sep 12, 2017

benhalverson commented Sep 12, 2017

mcollina commented Sep 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina commented Sep 20, 2017

mcollina commented Sep 21, 2017

benhalverson commented Sep 21, 2017

BridgeAR commented Sep 24, 2017

benhalverson commented Sep 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina commented Sep 25, 2017

mcollina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

benhalverson commented Oct 2, 2017

benhalverson commented Oct 4, 2017

mcollina left a comment

Choose a reason for hiding this comment

addaleax commented Oct 7, 2017

mhdawson left a comment

Choose a reason for hiding this comment

mhdawson commented Oct 11, 2017

joyeecheung commented Oct 28, 2017

joyeecheung commented Oct 28, 2017

mcollina commented Oct 28, 2017

joyeecheung commented Oct 28, 2017

joyeecheung commented Oct 28, 2017

refack left a comment

Choose a reason for hiding this comment

Trott commented Oct 28, 2017

mcollina commented Oct 29, 2017

benhalverson commented Aug 26, 2017 •

edited

Loading

mcollina commented Sep 12, 2017 •

edited

Loading