http: add host and port info to ECONNRESET errors by jfromaniello · Pull Request #7498 · nodejs/node

jfromaniello · 2016-06-30T14:32:38Z

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)

http

Description of change

Add more information to the "ECONNRESET" errors generated on HTTP when the
socket hang ups before receiving a response.

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

This change is similar to #7476.

jasnell · 2016-06-30T17:14:12Z

LGTM if CI is green

bengl · 2016-06-30T20:38:07Z

Documentation for these new properties should probably be added here: https://github.com/nodejs/node/blame/master/doc/api/errors.md#L481-L484

frontconnect · 2016-06-30T21:09:50Z

test/parallel/test-http-econnreset-pipe.js

What is common.PIPE in here?

@ankakusu

node/test/common.js

Lines 179 to 186 in 15157c3

if (exports.isWindows) {

exports.PIPE = '\\\\.\\pipe\\libuv-test';

if (process.env.TEST_THREAD_ID) {

exports.PIPE += '.' + process.env.TEST_THREAD_ID;

}

} else {

exports.PIPE = exports.tmpDir + '/test.sock';

}

(It's a little obscure at first glance, I agree)

Can you add common.mustCall() to this callback.

addaleax · 2016-07-04T19:18:42Z

LGTM with CI: https://ci.nodejs.org/job/node-test-commit/3968/

jasnell · 2016-08-05T23:03:30Z

Looks like this fell by the wayside .... New CI run: https://ci.nodejs.org/job/node-test-pull-request/3549/

cjihrig · 2016-08-11T15:59:06Z

test/parallel/test-http-econnreset-pipe.js

Can you use const here.

jfromaniello · 2016-08-11T16:18:13Z

@cjihrig Thank you for the feedback.

I just pushed the fixes.

cjihrig · 2016-08-11T16:21:28Z

Thanks, LGTM. CI: https://ci.nodejs.org/job/node-test-pull-request/3623/

jasnell · 2016-08-11T16:37:46Z

looks like possibly some linting issues?

jfromaniello · 2016-08-11T17:16:38Z

@jasnell thanks for the heads up. I was able to run the linter on my side and fixed the issue.

jasnell · 2016-08-18T22:15:20Z

New CI: https://ci.nodejs.org/job/node-test-pull-request/3740/ (sorry for the delay)

jasnell · 2016-08-18T22:18:13Z

@jfromaniello ... can I ask you to make one additional change here to make it consistent with the change you did in your other PR ==> #7476 (comment)

cdnbacon · 2016-10-15T19:02:24Z

Looking forward to this one landing, anything I can do to help?

jasnell · 2016-10-17T16:11:10Z

ping @jfromaniello

jfromaniello · 2016-10-17T16:22:51Z

@jasnell Just fixed the indentation as you suggested on the other PR.

What else could I do in this PR?

jasnell · 2017-01-06T19:55:52Z

Looks like this one slipped through the cracks! New CI: https://ci.nodejs.org/job/node-test-pull-request/5735/

cdnbacon · 2017-03-06T06:51:36Z

@jasnell @jfromaniello it looks like the linter and test/arm failure Details links go to 404 pages. Any chance we can get another run to replace the 404? I'd love to know what's left to fix to get this merged (and help if I can).

bnoordhuis · 2017-03-06T10:09:38Z

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

cjihrig

@jfromaniello can you run make lint locally? That's the only thing that failed on the CI. Other than that, LGTM.

fhinkel · 2017-03-26T12:28:04Z

ping @jfromaniello Do you want to look into the the linter errors? Thanks

Add more information to the "ECONNRESET" errors generated when the socket hang ups before receiving the response. These kind of errors are really hard to troubleshoot without this info.

jfromaniello · 2017-03-30T10:56:54Z

@fhinkel thank you for the heads up! I just fixed the linting errors. I missed the first comment from @cjihrig

tniessen · 2017-06-14T17:22:07Z

Last CI was three months ago, I will land this today if CI passes: https://ci.nodejs.org/job/node-test-pull-request/8656/

tniessen

This PR needs to be adapted to some recent changes in _http_client.js (see CI failures). Namely, the variable self is not defined anymore.

tniessen · 2017-07-16T14:32:26Z

I am closing this due to inactivity, feel free to reopen (or ping me).

nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Jun 30, 2016

frontconnect reviewed Jun 30, 2016
View reviewed changes

addaleax added semver-minor PRs that contain new features and should be released in the next minor version. error-handling labels Jul 4, 2016

cjihrig reviewed Aug 11, 2016
View reviewed changes

test/parallel/test-http-econnreset-pipe.js Outdated

Copy link

Contributor

cjihrig Aug 11, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use const here.

jfromaniello force-pushed the http_econnreset_details branch from b6f2258 to ebc53e0 Compare August 11, 2016 16:17

jfromaniello force-pushed the http_econnreset_details branch from ebc53e0 to fcf4623 Compare August 11, 2016 17:16

Trott force-pushed the master branch from b0df363 to c5ce7f4 Compare September 21, 2016 00:09

jfromaniello force-pushed the http_econnreset_details branch from fcf4623 to 50221cf Compare October 17, 2016 16:23

rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:02

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

jfromaniello mentioned this pull request Jan 11, 2017

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

Closed

3 tasks

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

cjihrig approved these changes Mar 6, 2017

View reviewed changes

http: add host and port info to ECONNRESET errors

c995420

Add more information to the "ECONNRESET" errors generated when the socket hang ups before receiving the response. These kind of errors are really hard to troubleshoot without this info.

jfromaniello force-pushed the http_econnreset_details branch from 50221cf to c995420 Compare March 30, 2017 10:56

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

tniessen self-assigned this Jun 14, 2017

tniessen requested changes Jun 14, 2017

View reviewed changes

tniessen closed this Jul 16, 2017

	if (exports.isWindows) {
	exports.PIPE = '\\\\.\\pipe\\libuv-test';
	if (process.env.TEST_THREAD_ID) {
	exports.PIPE += '.' + process.env.TEST_THREAD_ID;
	}
	} else {
	exports.PIPE = exports.tmpDir + '/test.sock';
	}

Uh oh!

Comments

Conversation

jfromaniello commented Jun 30, 2016

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

jasnell commented Jun 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bengl commented Jun 30, 2016

Uh oh!

frontconnect Jun 30, 2016

Choose a reason for hiding this comment

Uh oh!

Fishrock123 Jun 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjihrig Aug 11, 2016

Choose a reason for hiding this comment

Uh oh!

addaleax commented Jul 4, 2016

Uh oh!

jasnell commented Aug 5, 2016

Uh oh!

cjihrig Aug 11, 2016

Choose a reason for hiding this comment

Uh oh!

jfromaniello commented Aug 11, 2016

Uh oh!

cjihrig commented Aug 11, 2016

Uh oh!

jasnell commented Aug 11, 2016

Uh oh!

jfromaniello commented Aug 11, 2016

Uh oh!

jasnell commented Aug 18, 2016

Uh oh!

jasnell commented Aug 18, 2016

Uh oh!

cdnbacon commented Oct 15, 2016

Uh oh!

jasnell commented Oct 17, 2016

Uh oh!

jfromaniello commented Oct 17, 2016

Uh oh!

jasnell commented Jan 6, 2017

Uh oh!

cdnbacon commented Mar 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bnoordhuis commented Mar 6, 2017

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

fhinkel commented Mar 26, 2017

Uh oh!

jfromaniello commented Mar 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tniessen commented Jun 14, 2017

Uh oh!

tniessen left a comment

Choose a reason for hiding this comment

Uh oh!

tniessen commented Jul 16, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

jasnell commented Jun 30, 2016 •

edited

Loading

Fishrock123 Jun 30, 2016 •

edited

Loading

cdnbacon commented Mar 6, 2017 •

edited

Loading

jfromaniello commented Mar 30, 2017 •

edited

Loading