errors,tls_wrap: migrate to use internal/errors.js #13476

bidipyne · 2017-06-05T16:17:51Z

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

Affected core subsystem(s)

tls_wrap.js, internal/errors.js

I read and understood the contribution guidelines, please review and suggest.
@jasnell
Thanks.

jasnell · 2017-06-05T18:17:42Z

lib/internal/errors.js

@@ -154,12 +157,21 @@ E('ERR_UNKNOWN_BUILTIN_MODULE', (id) => `No such built-in module: ${id}`);
 E('ERR_UNKNOWN_SIGNAL', (signal) => `Unknown signal: ${signal}`);
 E('ERR_UNKNOWN_STDIN_TYPE', 'Unknown stdin file type');
 E('ERR_UNKNOWN_STREAM_TYPE', 'Unknown stream file type');
+E('ERR_RENEGOTIATE', 'Failed to renegotiate');


The code should likely be a bit more specific... e.g. ERR_TLS_RENEGOTIATION_FAILED, etc

jasnell · 2017-06-05T18:18:07Z

lib/internal/errors.js

@@ -154,12 +157,21 @@ E('ERR_UNKNOWN_BUILTIN_MODULE', (id) => `No such built-in module: ${id}`);
 E('ERR_UNKNOWN_SIGNAL', (signal) => `Unknown signal: ${signal}`);
 E('ERR_UNKNOWN_STDIN_TYPE', 'Unknown stdin file type');
 E('ERR_UNKNOWN_STREAM_TYPE', 'Unknown stream file type');
+E('ERR_RENEGOTIATE', 'Failed to renegotiate');
+E('ERR_REQD_SERVER_NAME',


ERR_TLS_SERVER_NAME_REQUIRED

jasnell · 2017-06-05T18:19:13Z

test/parallel/test-tls-client-mindhsize.js

@@ -54,7 +54,7 @@ function test(size, err, next) {
      client.on('error', function(e) {
        nerror++;
        assert.strictEqual(e.message,
-                           'DH parameter size 1024 is less than 2048');
+                           'ERR_DH_PARAM_SIZE');


this would need to be

assert.strictEqual(e.code, 'ERR_DH_PARAM_SIZE');

In this file, e is of type Object Error. This doesn't have the field e.code which we can access, so I believe, this one will be more appropriate in that case.

I have written a small test-case and verified that:

var u = require('util') var e = new Error('ERR_DH_PARAM_SIZE'); console.log(u.inspect(e, {showHidden: true}))`

Output:

at Object.<anonymous> (/home/bidisha/swe_node/n1/node/foo.js:2:9) at Module._compile (module.js:569:30) at Object.Module._extensions..js (module.js:580:10) at Module.load (module.js:503:32) at tryModuleLoad (module.js:466:12) at Function.Module._load (module.js:458:3) at Function.Module.runMain (module.js:605:10) at startup (bootstrap_node.js:158:16) at bootstrap_node.js:575:3 [stack]: 'Error: ERR_DH_PARAM_SIZE\n at Object.<anonymous> (/home/bidisha/swe_node/n1/node/foo.js:2:9)\n at Module._compile (module.js:569:30)\n at Object.Module._extensions..js (module.js:580:10)\n at Module.load (module.js:503:32)\n at tryModuleLoad (module.js:466:12)\n at Function.Module._load (module.js:458:3)\n at Function.Module.runMain (module.js:605:10)\n at startup (bootstrap_node.js:158:16)\n at bootstrap_node.js:575:3', [message]: 'ERR_DH_PARAM_SIZE' }`

Please suggest on this.

Sorry for the incorrect observation above.
Now, things seems to be resolved.
Thanks!

mhdawson · 2017-06-06T18:19:37Z

lib/internal/errors.js

@@ -154,12 +157,21 @@ E('ERR_UNKNOWN_BUILTIN_MODULE', (id) => `No such built-in module: ${id}`);
 E('ERR_UNKNOWN_SIGNAL', (signal) => `Unknown signal: ${signal}`);
 E('ERR_UNKNOWN_STDIN_TYPE', 'Unknown stdin file type');
 E('ERR_UNKNOWN_STREAM_TYPE', 'Unknown stream file type');
+E('ERR_RENEGOTIATE', 'Failed to renegotiate');


mhdawson · 2017-06-06T18:20:50Z

lib/internal/errors.js

 E('ERR_SOCKET_DGRAM_NOT_RUNNING', 'Not running');
+E('ERR_TLS_HANDSHAKE_TIMEOUT', 'TLS handshake timeout');
+E('ERR_TLS_SESSION_ATTACK', 'TSL session renegotiation attack detected');
+E('ERR_TLS_SESSION_CALLBACK', 'TSL sesssion callback was called 2 times');


maybe 'ERR_TLS_SESSION_CALLBACK_ALREADY_CALLED'

Done.
Thank You!

mhdawson · 2017-06-06T18:21:48Z

lib/internal/errors.js

+E('ERR_TLS_SESSION_ATTACK', 'TSL session renegotiation attack detected');
+E('ERR_TLS_SESSION_CALLBACK', 'TSL sesssion callback was called 2 times');
+E('ERR_TLS_OCSP_CALLBACK', 'TLS OCSP callback was called 2 times');
+E('ERR_TLS_SNI_CALLBACK', 'TLS SNI callback was called 2 times');


Maybe add "_ALREADY_CALLED" to these 2 as well. It does make the code long but it also makes it clearer what's going wrong.

jasnell · 2017-06-15T17:50:16Z

lib/_tls_wrap.js

@@ -552,7 +553,7 @@ TLSSocket.prototype.renegotiate = function(options, callback) {
  }
  if (!this._handle.renegotiate()) {
    if (callback) {
-      process.nextTick(callback, new Error('Failed to renegotiate'));
+      process.nextTick(callback, new errors.Error('ERR_RENEGOTIATE'));


s/ERR_RENEGOTIATE/ERR_TLS_RENEGOTIATE ?

jasnell · 2017-06-15T17:50:45Z

lib/_tls_wrap.js

@@ -949,7 +950,7 @@ Server.prototype.setOptions = function(options) {
 // SNI Contexts High-Level API
 Server.prototype.addContext = function(servername, context) {
  if (!servername) {
-    throw new Error('"servername" is required parameter for Server.addContext');
+    throw new errors.Error('ERR_REQD_SERVER_NAME');


s/ERR_REQD_SERVER_NAME/ERR_TLS_REQUIRED_SERVER_NAME ?

jasnell · 2017-06-15T17:51:19Z

lib/_tls_wrap.js

@@ -1088,8 +1089,7 @@ exports.connect = function(...args /* [port,] [host,] [options,] [cb] */) {
    // specified in options.
    var ekeyinfo = socket.getEphemeralKeyInfo();
    if (ekeyinfo.type === 'DH' && ekeyinfo.size < options.minDHSize) {
-      var err = new Error('DH parameter size ' + ekeyinfo.size +
-                          ' is less than ' + options.minDHSize);
+      var err = new errors.Error('ERR_DH_PARAM_SIZE');


s/ERR_DH_PARAM_SIZE/ERR_TLS_DH_PARAM_SIZE ?

jasnell · 2017-06-15T17:52:26Z

lib/internal/errors.js

@@ -115,6 +115,7 @@ E('ERR_ASSERTION', (msg) => msg);
 E('ERR_CONSOLE_WRITABLE_STREAM',
  (name) => `Console expects a writable stream instance for ${name}`);
 E('ERR_CPU_USAGE', (errMsg) => `Unable to obtain cpu usage ${errMsg}`);
+E('ERR_DH_PARAM_SIZE', 'DH parameter size 1024 is less than 2048');


this is hard coding the param size. it shouldn't be.

E('ERR_DH_PARAM_SIZE', (size) => `DH parameter size ${size} is less than 2048`);

jasnell · 2017-06-15T17:53:14Z

lib/internal/errors.js

+E('ERR_TLS_HANDSHAKE_TIMEOUT', 'TLS handshake timeout');
+E('ERR_TLS_SESSION_ATTACK', 'TSL session renegotiation attack detected');
+E('ERR_TLS_SESSION_CALLBACK_ALREADY_CALLED',
+  'TSL sesssion callback was called 2 times');


I would take the opportunity to reword this a bit:

'The session callback was called more than once'

jasnell · 2017-06-15T17:54:11Z

lib/internal/errors.js

+E('ERR_TLS_SESSION_CALLBACK_ALREADY_CALLED',
+  'TSL sesssion callback was called 2 times');
+E('ERR_TLS_OCSP_CALLBACK_ALREADY_CALLED',
+  'TLS OCSP callback was called 2 times');


Actually... making this a generic error would make more sense, e.g.

E('ERR_MULTIPLE_CALLBACK', (name) => `The ${name} callback was called more than once`);

jasnell

Getting closer but left a few comments that need to be addressed. Thank you!

bidipyne · 2017-06-25T13:51:42Z

Your suggestions are addressed.
Thank You! @jasnell

jasnell

Thank you!

mhdawson · 2017-06-29T21:49:21Z

Needs a rebase and then we can land.

mhdawson

LGTM

mhdawson · 2017-07-07T17:23:42Z

lib/internal/errors.js

@@ -178,8 +178,29 @@ E('ERR_TRANSFORM_WITH_LENGTH_0',
 E('ERR_UNKNOWN_SIGNAL', 'Unknown signal: %s');
 E('ERR_UNKNOWN_STDIN_TYPE', 'Unknown stdin file type');
 E('ERR_UNKNOWN_STREAM_TYPE', 'Unknown stream file type');
+<<<<<<< 3129b2c035a1fb9b1bf07e5ecddcebce4c5fa4b0


There is a merge problem here. @bidipyne can you take a look/fix up

Done that fix. Thanks @mhdawson

refack

LGTM (except for the leaked lines in errors.js)

refack · 2017-07-07T22:47:34Z

lib/internal/errors.js

 E('ERR_V8BREAKITERATOR', 'Full ICU data not installed. ' +
  'See https://github.com/nodejs/node/wiki/Intl');
+E('FALSY_VALUE_REJECTION', 'Promise was rejected with falsy value');


This is a rebase leak...
Could you go over this list and keep only the ERRs you need for this PR

Removed all unrelated ERRs to this PR. Thanks! @refack.

gireeshpunathil · 2017-07-08T06:30:34Z

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

refack · 2017-07-08T16:51:35Z

lib/internal/errors.js

+  `DH parameter size ${size} is less than 2048`);
+E('ERR_TLS_HANDSHAKE_TIMEOUT', 'TLS handshake timeout');
+E('ERR_TLS_SESSION_ATTACK', 'TSL session renegotiation attack detected');
+E('ERR_MULTIPLE_CALLBACK',


This is a redefinition of ERR_MULTIPLE_CALLBACK from L162 this causes the error message to change.

You can just remove this one and use the one in L162

You can remove this one and improve L162 (and update its usages)

[Optional] You could sort this whole block alphabetically (very easy if your IDE supports that, otherwise ignore)

You can open an issue that E() doesn't fail for redefinition, or even better a PR that makes it fail 😉

not ok 1108 parallel/test-stream-transform-callback-twice --- duration_ms: 0.61 severity: fail stack: |- assert.js:55 throw new errors.AssertionError({ ^ AssertionError [ERR_ASSERTION]: 'The undefined callback was called more than once' === 'Callback called multiple times' at Transform.<anonymous> (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/common/index.js:716:14) at emitOne (events.js:115:13) at Transform.emit (events.js:210:7) at Transform.afterTransform (_stream_transform.js:80:17) at Transform.transform [as _transform] (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/parallel/test-stream-transform-callback-twice.js:5:37) at Transform._read (_stream_transform.js:185:10) at Transform._write (_stream_transform.js:173:12) at doWrite (_stream_writable.js:371:12) at writeOrBuffer (_stream_writable.js:357:5) at Transform.Writable.write (_stream_writable.js:274:11) ...

https://ci.nodejs.org/job/node-test-commit-linuxone/7151/

Thanks for the suggestion @refack
I have redefined the error message accordingly but this lead to change of another test file: test-stream-transform-callback-twice.js
Hope that is fine with this PR.
Thank you!

The change to test-stream-transform-callback-twice.js is good by me, but IMHO you should just remote L162, since this line simply overrides it anyway (there's no other magic).

refack · 2017-07-08T18:56:24Z

lib/internal/errors.js

+  `DH parameter size ${size} is less than 2048`);
+E('ERR_TLS_HANDSHAKE_TIMEOUT', 'TLS handshake timeout');
+E('ERR_TLS_SESSION_ATTACK', 'TSL session renegotiation attack detected');
+E('ERR_MULTIPLE_CALLBACK',


The change to test-stream-transform-callback-twice.js is good by me, but IMHO you should just remote L162, since this line simply overrides it anyway (there's no other magic).

mhdawson · 2017-07-11T18:06:29Z

lib/internal/errors.js

@@ -159,7 +159,7 @@ E('ERR_IPC_DISCONNECTED', 'IPC channel is already disconnected');
 E('ERR_IPC_ONE_PIPE', 'Child process can have only one IPC pipe');
 E('ERR_IPC_SYNC_FORK', 'IPC cannot be used with synchronous forks');
 E('ERR_MISSING_ARGS', missingArgs);
-E('ERR_MULTIPLE_CALLBACK', 'Callback called multiple times');
+E('ERR_MULTIPLE_CALLBACK', 'The undefined callback was called more than once');


This does not look right to me. 'The undefined callback...'

I have reverted the message back back. Please have a look!

mhdawson · 2017-07-11T18:07:33Z

lib/internal/errors.js

+E('ERR_TLS_HANDSHAKE_TIMEOUT', 'TLS handshake timeout');
+E('ERR_TLS_SESSION_ATTACK', 'TSL session renegotiation attack detected');
+E('ERR_MULTIPLE_CALLBACK',
+  (name) => `The ${name} callback was called more than once`);


Related to my comment above. Agree with @refack that the duplicate line above needs to be removed.

mhdawson · 2017-07-11T18:08:22Z

test/parallel/test-stream-transform-callback-twice.js

@@ -7,7 +7,7 @@ const stream = new Transform({

 stream.on('error', common.expectsError({
  type: Error,
-  message: 'Callback called multiple times',
+  message: 'The undefined callback was called more than once',


Should we not be updating the code that generates this error so that it does not have 'undefined' but an indication of what the callback is ?

Referring to the suggestions above, error message on this file is also redefined.
Please suggest me on that!
Thank You! @mhdawson

mhdawson

Very close just a few more comments.

bidipyne · 2017-07-16T15:21:01Z

Thank you all, for guiding me on this PR @mhdawson @refack @jasnell @gireeshpunathil
As this PR is hanging there for a long time, it would be really helpful if I can get all the reviews altogether at once in order to make appropriate changes, making things easier to get it landed soon.

jasnell · 2017-07-17T15:51:07Z

lib/internal/errors.js

+E('ERR_TLS_HANDSHAKE_TIMEOUT', 'TLS handshake timeout');
+E('ERR_TLS_SESSION_ATTACK', 'TSL session renegotiation attack detected');
+E('ERR_TLS_RENEGOTIATION_FAILED', 'Failed to renegotiate');
+E('ERR_TLS_REQUIRED_SERVER_NAME',


The only remaining thing I see here is making sure that the error codes are in alphabetical order.

refack · 2017-07-17T21:06:15Z

@bidipyne non-blocking observation. You changed 15 Error generating LOCs, and added 6 new Error codes, but fixed only 3 tests.
Do you have an idea how could we find the other impacted tests? They probably use unanchored RegExps (without start of line ^ and end on line $) so they still pass...
We should open a tracking issue with all those cases.

mhdawson

LGTM once order of messages is fixed. Please comment when you have done that and we can start the CI.

PR-URL: nodejs#13476 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

refack · 2017-07-19T22:36:34Z

Addressed nit and landed
Extra sanity on master: https://ci.nodejs.org/job/node-test-commit-linuxone/7440/nodes=rhel72-s390x/

nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. tls Issues and PRs related to the tls subsystem. labels Jun 5, 2017

jasnell reviewed Jun 5, 2017

View reviewed changes

mhdawson reviewed Jun 6, 2017

View reviewed changes

bidipyne force-pushed the my-branch branch 2 times, most recently from 47f3eff to 9a86f6d Compare June 14, 2017 12:24

bidipyne closed this Jun 14, 2017

bidipyne force-pushed the my-branch branch from 9a86f6d to de762b7 Compare June 14, 2017 12:49

bidipyne reopened this Jun 14, 2017

bidipyne force-pushed the my-branch branch 4 times, most recently from b7dcebb to 45c9e10 Compare June 15, 2017 07:20

jasnell reviewed Jun 15, 2017

View reviewed changes

jasnell requested changes Jun 15, 2017

View reviewed changes

bidipyne force-pushed the my-branch branch 2 times, most recently from cacf0d9 to bec79c7 Compare June 25, 2017 13:49

jasnell approved these changes Jun 26, 2017

View reviewed changes

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

mhdawson reviewed Jun 29, 2017

View reviewed changes

jasnell mentioned this pull request Jun 29, 2017

Tracking Issue: Migrate errors to internal/errors.js #11273

Closed

80 tasks

mhdawson reviewed Jul 7, 2017

View reviewed changes

bidipyne force-pushed the my-branch branch 2 times, most recently from de7f1f7 to dbe7f78 Compare July 7, 2017 18:18

refack suggested changes Jul 7, 2017

View reviewed changes

bidipyne force-pushed the my-branch branch from dbe7f78 to c43548c Compare July 8, 2017 05:54

refack approved these changes Jul 8, 2017

View reviewed changes

refack force-pushed the my-branch branch from c43548c to c68d3a0 Compare July 8, 2017 16:40

refack suggested changes Jul 8, 2017

View reviewed changes

bidipyne force-pushed the my-branch branch from c68d3a0 to 193ba7f Compare July 8, 2017 18:16

refack approved these changes Jul 8, 2017

View reviewed changes

mhdawson reviewed Jul 11, 2017

View reviewed changes

refack mentioned this pull request Jul 15, 2017

meta: semver impact of Error messages #13937

Closed

bidipyne force-pushed the my-branch branch 2 times, most recently from 2b58918 to 32a6d1a Compare July 16, 2017 13:55

refack self-assigned this Jul 16, 2017

jasnell reviewed Jul 17, 2017

View reviewed changes

mhdawson approved these changes Jul 18, 2017

View reviewed changes

errors: migrate tls_wrap to use internal/errors

f67aa56

PR-URL: nodejs#13476 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

refack force-pushed the my-branch branch from 32a6d1a to f67aa56 Compare July 19, 2017 22:25

refack merged commit f67aa56 into nodejs:master Jul 19, 2017

jasnell mentioned this pull request Oct 30, 2017

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

Merged

refack removed their assignment Oct 20, 2018

errors,tls_wrap: migrate to use internal/errors.js #13476

errors,tls_wrap: migrate to use internal/errors.js #13476

Conversation

bidipyne commented Jun 5, 2017

Affected core subsystem(s)

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

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

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

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

jasnell Jun 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell left a comment

Choose a reason for hiding this comment

bidipyne commented Jun 25, 2017

jasnell left a comment

Choose a reason for hiding this comment

mhdawson commented Jun 29, 2017

mhdawson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gireeshpunathil commented Jul 8, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhdawson left a comment

Choose a reason for hiding this comment

bidipyne commented Jul 16, 2017

Choose a reason for hiding this comment

refack commented Jul 17, 2017

mhdawson left a comment

Choose a reason for hiding this comment

refack commented Jul 19, 2017

jasnell Jun 15, 2017 •

edited

Loading