tls: add host and port info to ECONNRESET errors #7476

jfromaniello · 2016-06-29T13:38:13Z

Checklist

make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

TLS

Description of change

Add more information (host, port, path) to the "ECONNRESET" errors generated when the
socket hang ups before establishing the secure connection.

These kind of errors are really hard to troubleshoot without this info.

cjihrig · 2016-06-29T13:53:36Z

Is there potential to reuse util._exceptionWithHostPort() here?

santigimeno · 2016-06-29T13:54:41Z

test/parallel/test-tls-wrap-econnreset.js

+const net = require('net');
+
+net.createServer(c => {
+  setTimeout(() => c.end(), 300);


Is the timeout necessary?

jfromaniello · 2016-06-29T14:25:11Z

@santigimeno thanks for all the feedback in the test case :)

@cjihrig good point! I will try

jfromaniello · 2016-06-29T14:38:21Z

@cjihrig I can't find a way to call util._exceptionWithHostPort without getting this error:

util.js:1003
  var errname = uv.errname(err);
                   ^

Error: err >= 0
    at Error (native)
    at Object.exports._errnoException (util.js:1003:20)
    at Object.exports._exceptionWithHostPort (util.js:1030:20)
    at TLSSocket.onHangUp (_tls_wrap.js:1094:21)
    at TLSSocket.g (events.js:286:16)
    at emitNone (events.js:91:20)
    at TLSSocket.emit (events.js:185:7)
    at endReadableNT (_stream_readable.js:926:12)
    at _combinedTickCallback (internal/process/next_tick.js:74:11)
    at process._tickCallback (internal/process/next_tick.js:98:9)

I see this function is being used in two places, the first argument being the error returned from bind.

cjihrig · 2016-06-29T15:05:31Z

Ah, sorry, it expects an error code, and we're creating an error object here. Carry on :-)

jasnell · 2016-06-29T22:47:29Z

Don't have any particular issue with this but I'd like to get some more opinions. @nodejs/ctc

indutny · 2016-06-30T00:46:17Z

LGTM

bnoordhuis · 2016-06-30T07:15:54Z

test/parallel/test-tls-wrap-econnreset.js

+  https.get(`https://localhost:${port}`)
+    .on('error', common.mustCall(e => {
+      assert.equal(e.host, 'localhost');
+      assert.equal(e.port, port);


Can you use assert.strictEqual here?

good catch, I am not sure why it ends up being an string at _tls_wrap.connect

bnoordhuis · 2016-06-30T07:18:23Z

It would be nice to have test coverage for options.socket and options.path as well.

jfromaniello · 2016-06-30T12:46:25Z

@bnoordhuis thank you very much for the feedback. I added tests for options.socket and options.path as well.

jfromaniello · 2016-06-30T15:42:45Z

I did some changes to the PR to include the address when using options.socket.

bnoordhuis · 2016-06-30T19:17:55Z

lib/_tls_wrap.js

+      };
+    }
+  }
+


This adds a closure and makes the closure for onHangup() bigger (i.e., more memory, more gc'ing.) I would like to avoid that.

@bnoordhuis ok, I am going to revert to what I had before, but I won't be able to handle the options.socket case properly, because it seems that socket.remoteAddress is deleted before the end event.

jfromaniello · 2016-06-30T19:53:15Z

@bnoordhuis updated with your suggestion. thanks again

bnoordhuis · 2016-07-01T10:29:01Z

test/parallel/test-tls-wrap-econnreset-pipe.js

+      assert.strictEqual(e.port, undefined);
+      assert.strictEqual(e.host, undefined);
+      server.close();
+    }, 'the request must emit error'));


common.mustCall doesn't take a string as its second argument.

jasnell · 2017-01-06T19:54:18Z

Looks like this one slipped through the cracks

jasnell · 2017-01-06T19:54:53Z

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

jfromaniello · 2017-01-11T14:49:29Z

Is there anything I can help with to get this one and #7498 merged? Thanks

trevnorris

looks like there are enough approvals for this to land if tests are happy.

fhinkel · 2017-03-26T12:27:31Z

It's semver-minor. Can we just land it?

jasnell · 2017-03-26T13:45:11Z

Yep. It's on my list to review on Monday but if you'd like to go ahead, go for it..

tniessen · 2017-06-14T17:20:06Z

Last CI was half a year ago, I will land this today if CI passes: https://ci.nodejs.org/job/node-test-pull-request/8655/

Edit: Retrying osx only: https://ci.nodejs.org/job/node-test-commit-osx/10491/

Add more information to the "ECONNRESET" errors generated when the socket hang ups before establishing the secure connection. These kind of errors are really hard to troubleshoot without this info. PR-URL: #7476 Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: Yazhong Liu <[email protected]>

tniessen · 2017-06-14T19:53:40Z

CI is green, landed in 3ee3732! Thank you for your contribution! 🎉

Add more information to the "ECONNRESET" errors generated when the socket hang ups before establishing the secure connection. These kind of errors are really hard to troubleshoot without this info. PR-URL: #7476 Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: Yazhong Liu <[email protected]>

gibfahn · 2018-01-15T21:53:16Z

Release team decided not to land on v6.x, if you disagree let us know.

bnoordhuis · 2018-01-17T11:06:31Z

@gibfahn What was the motivation? It's pretty useful.

gibfahn · 2018-01-18T16:55:47Z

@gibfahn What was the motivation? It's pretty useful.

IIRC the motivation was that this changes an error message, which could break people. If that's not true we can reconsider.

jfromaniello · 2018-01-18T19:07:59Z

IIRC the motivation was that this changes an error message, which could break people. If that's not true we can reconsider.

It is not true, it doesn't change the error message. It just add new properties to the error object:

error.path
error.host
error.port
error.localAddress

This is currently being done in other places with the same property names (I can't remember now where) and I think is very useful to diagnose issues with tcp connections.

gibfahn · 2018-01-19T09:52:10Z

cc/ @nodejs/lts let's reconsider.

nodejs-github-bot added the tls Issues and PRs related to the tls subsystem. label Jun 29, 2016

santigimeno reviewed Jun 29, 2016
View reviewed changes

jfromaniello force-pushed the add_more_info_tls_error branch from 27b73ad to cbd133d Compare June 29, 2016 14:24

jasnell added semver-minor PRs that contain new features and should be released in the next minor version. error-handling labels Jun 29, 2016

bnoordhuis reviewed Jun 30, 2016
View reviewed changes

jfromaniello force-pushed the add_more_info_tls_error branch from cbd133d to 539ebd7 Compare June 30, 2016 12:45

jfromaniello mentioned this pull request Jun 30, 2016

http: add host and port info to ECONNRESET errors #7498

Closed

3 tasks

jfromaniello force-pushed the add_more_info_tls_error branch from 539ebd7 to 7681074 Compare June 30, 2016 15:41

jfromaniello force-pushed the add_more_info_tls_error branch from 7681074 to 7ce5824 Compare June 30, 2016 15:47

bnoordhuis reviewed Jun 30, 2016
View reviewed changes

jfromaniello force-pushed the add_more_info_tls_error branch from 7ce5824 to bd4dc97 Compare June 30, 2016 19:52

bnoordhuis reviewed Jul 1, 2016
View reviewed changes

jfromaniello force-pushed the add_more_info_tls_error branch from bd4dc97 to 6652492 Compare July 1, 2016 15:41

mscdex added errors Issues and PRs related to JavaScript errors originated in Node.js core. and removed error-handling labels Oct 28, 2016

trevnorris approved these changes Jan 11, 2017

View reviewed changes

MylesBorins force-pushed the master branch from 8df7ee0 to 54fef67 Compare February 1, 2017 01:00

cjihrig approved these changes Mar 26, 2017

View reviewed changes

refack force-pushed the master branch from 16073c0 to fbe946b Compare April 14, 2017 04:11

tniessen self-assigned this Jun 14, 2017

tniessen closed this Jun 14, 2017

addaleax mentioned this pull request Jun 17, 2017

v8.2.0 proposal #13744

Merged

gibfahn added the dont-land-on-v6.x label Jan 15, 2018

gibfahn added the lts-agenda label Jan 22, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tls: add host and port info to ECONNRESET errors #7476

tls: add host and port info to ECONNRESET errors #7476

jfromaniello commented Jun 29, 2016

cjihrig commented Jun 29, 2016

santigimeno Jun 29, 2016

jfromaniello commented Jun 29, 2016

jfromaniello commented Jun 29, 2016

cjihrig commented Jun 29, 2016

jasnell commented Jun 29, 2016

indutny commented Jun 30, 2016

bnoordhuis Jun 30, 2016

jfromaniello Jun 30, 2016

bnoordhuis commented Jun 30, 2016

jfromaniello commented Jun 30, 2016

jfromaniello commented Jun 30, 2016

bnoordhuis Jun 30, 2016

jfromaniello Jun 30, 2016

jfromaniello commented Jun 30, 2016

bnoordhuis Jul 1, 2016

jasnell commented Jan 6, 2017

jasnell commented Jan 6, 2017

jfromaniello commented Jan 11, 2017

trevnorris left a comment

fhinkel commented Mar 26, 2017

jasnell commented Mar 26, 2017

tniessen commented Jun 14, 2017 •

edited

Loading

tniessen commented Jun 14, 2017 •

edited

Loading

gibfahn commented Jan 15, 2018

bnoordhuis commented Jan 17, 2018

gibfahn commented Jan 18, 2018

jfromaniello commented Jan 18, 2018

gibfahn commented Jan 19, 2018

tls: add host and port info to ECONNRESET errors #7476

tls: add host and port info to ECONNRESET errors #7476

Conversation

jfromaniello commented Jun 29, 2016

Checklist

Affected core subsystem(s)

Description of change

cjihrig commented Jun 29, 2016

santigimeno Jun 29, 2016

Choose a reason for hiding this comment

jfromaniello commented Jun 29, 2016

jfromaniello commented Jun 29, 2016

cjihrig commented Jun 29, 2016

jasnell commented Jun 29, 2016

indutny commented Jun 30, 2016

bnoordhuis Jun 30, 2016

Choose a reason for hiding this comment

jfromaniello Jun 30, 2016

Choose a reason for hiding this comment

bnoordhuis commented Jun 30, 2016

jfromaniello commented Jun 30, 2016

jfromaniello commented Jun 30, 2016

bnoordhuis Jun 30, 2016

Choose a reason for hiding this comment

jfromaniello Jun 30, 2016

Choose a reason for hiding this comment

jfromaniello commented Jun 30, 2016

bnoordhuis Jul 1, 2016

Choose a reason for hiding this comment

jasnell commented Jan 6, 2017

jasnell commented Jan 6, 2017

jfromaniello commented Jan 11, 2017

trevnorris left a comment

Choose a reason for hiding this comment

fhinkel commented Mar 26, 2017

jasnell commented Mar 26, 2017

tniessen commented Jun 14, 2017 • edited Loading

tniessen commented Jun 14, 2017 • edited Loading

gibfahn commented Jan 15, 2018

bnoordhuis commented Jan 17, 2018

gibfahn commented Jan 18, 2018

jfromaniello commented Jan 18, 2018

gibfahn commented Jan 19, 2018

tniessen commented Jun 14, 2017 •

edited

Loading

tniessen commented Jun 14, 2017 •

edited

Loading