Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed

Conversation

jfromaniello
Copy link
Contributor

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.

@nodejs-github-bot nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Jun 30, 2016
@jasnell
Copy link
Member

jasnell commented Jun 30, 2016

LGTM if CI is green

@bengl
Copy link
Member

bengl commented Jun 30, 2016

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

c.end();
});

server.listen(common.PIPE, () => {

Choose a reason for hiding this comment

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

What is common.PIPE in here?

Copy link
Contributor

@Fishrock123 Fishrock123 Jun 30, 2016

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

addaleax commented Jul 4, 2016

@jasnell
Copy link
Member

jasnell commented Aug 5, 2016

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


common.refreshTmpDir();

var server = net.createServer((c) => {
Copy link
Contributor

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 jfromaniello force-pushed the http_econnreset_details branch from b6f2258 to ebc53e0 Compare August 11, 2016 16:17
@jfromaniello
Copy link
Contributor Author

@cjihrig Thank you for the feedback.

I just pushed the fixes.

@cjihrig
Copy link
Contributor

cjihrig commented Aug 11, 2016

@jasnell
Copy link
Member

jasnell commented Aug 11, 2016

looks like possibly some linting issues?

@jfromaniello jfromaniello force-pushed the http_econnreset_details branch from ebc53e0 to fcf4623 Compare August 11, 2016 17:16
@jfromaniello
Copy link
Contributor Author

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

@jasnell
Copy link
Member

jasnell commented Aug 18, 2016

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

@jasnell
Copy link
Member

jasnell commented Aug 18, 2016

@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
Copy link

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

@jasnell
Copy link
Member

jasnell commented Oct 17, 2016

ping @jfromaniello

@jfromaniello
Copy link
Contributor Author

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

What else could I do in this PR?

@jfromaniello jfromaniello force-pushed the http_econnreset_details branch from fcf4623 to 50221cf Compare October 17, 2016 16:23
@rvagg rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:02
@mscdex mscdex added errors Issues and PRs related to JavaScript errors originated in Node.js core. and removed error-handling labels Oct 28, 2016
@jasnell
Copy link
Member

jasnell commented Jan 6, 2017

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

@cdnbacon
Copy link

cdnbacon commented Mar 6, 2017

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

image

@bnoordhuis
Copy link
Member

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

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

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

@fhinkel
Copy link
Member

fhinkel commented Mar 26, 2017

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 jfromaniello force-pushed the http_econnreset_details branch from 50221cf to c995420 Compare March 30, 2017 10:56
@jfromaniello
Copy link
Contributor Author

jfromaniello commented Mar 30, 2017

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

@tniessen
Copy link
Member

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

Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member

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

@tniessen tniessen closed this Jul 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
errors Issues and PRs related to JavaScript errors originated in Node.js core. http Issues or PRs related to the http subsystem. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet