Http(s) Agent: handle errors on idle sockets by jfromaniello · Pull Request #4482 · nodejs/node · GitHub

This is my proposed fix for #3595.

This change adds a new error handler on every socket when the socket is idle.

If there is an error on the tcp connection, eg: the server abruptly close the connection while the socket is idle then we destroy the socket and remove it from the pool of free sockets.

@nodejs/http

Thank you very much for submitting this patch. I love the idea of using newListener/removeListener to control this, and this is certainly very interesting. However I would prefer this could to work explicitly, rather than implicitly as you propose. Your patch clearly depends on event listener setup behavior of the current code in core, and it may break in very non-obvious ways if we will change the way things work in a different place.

What do you think about setting and removing error listeners, just as we do it with everything else?

Thank you again,
Fedor.

Thank you for your feedback!

The problem is that we need to set this error handler on sockets that are not being use by the _http_client, as @bnoordhuis points out on the issue we can't do it on the free event because it happens on the next tick:

https://github.com/nodejs/node/blob/master/lib/_http_client.js#L449-L453

I think it will be better to handle this on the _http_client.js, I am working on it!

@indutny I changed the solution, looking forward to your feedback, thanks again

Hey @jfromaniello, I've seen your name around the Node ecosystem a lot and I'm glad to see you now contributing to core (I believe this would be your first commit here if/when it lands). Obviously changes like these come with all sorts of edge-case risk so bear with us while this gets picked over. Please feel free to ping us in here again if things quiet down for an extended period (likely folks have just got distracted with all the other things happening).

Missing newline.

Only a style nits, otherwise LGTM. Thank you!

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

@rvagg thank you very much! I am big fan of your work and yes this is my first PR to node.

I understand this will take some time and I wish we could get more eyes on this to make sure it doesn't break anything.

@indutny thanks for your feedback, I did some changes!

Looking much better now, thank you! Two minor comments. (Just curious, does make lint pass with your changes now?)

I just fixed the linter problems :)

Fantastic! LGTM

Oh... one more nit. I'm really sorry for not noticing it at first. May I ask you to format the commit log according to conform with Contributing to Node.js guidelines?

Namely:

First line should start with a subsytem (http: in this case)
All lines should have proper length as per CONTRIBUTING.md

Thank you, and sorry for not posting it before.

Done, thank you and sorry for not reading the guideline before.

@jfromaniello no worries at all. The first line is longer than 50 characters, so the commit log doesn't completely fit the criteria now. May I ask you to take a look at it, please?

This change adds a new event handler to the `error` event of the socket after it has been used by the http_client. The purpose of this change is to catch errors on *keep alived* connections from idle sockets that otherwise will cause an uncaugh error event on the application. Closes nodejs#3595

oops , done!

Yay, looks great. Landing.

In December we announced that we would be doing a minor release in order to get a number of voted on SEMVER-MINOR changes into LTS. Our ability to release this was delayed due to the unforeseen security release v4.3. We are quickly bumping to v4.4 in order to bring you the features that we had committed to releasing. Notable changes: The SEMVER-MINOR changes include: * **deps**: - An update to v8 that introduces a new flag --perf_basic_prof_only_functions (Ali Ijaz Sheikh) #3609 * **http**: - A new feature in http(s) agent that catches errors on *keep alived* connections (José F. Romaniello) #4482 * **src**: - Better support for Big-Endian systems (Bryon Leung) #3410 * **tls**: - A new feature that allows you to pass common SSL options to `tls.createSecurePair` (Коренберг Марк) #2441 Notable semver patch changes include: * **npm** - upgrade to npm 2.14.19 (Kat Marchán) #5335 * **https**: - A potential fix for #3692) HTTP/HTTPS client requests throwing EPROTO (Fedor Indutny) #4982 * **process**: - Add support for symbols in event emitters. Symbols didn't exist when it was written ¯\_(ツ)_/¯ (cjihrig) #4798 * **querystring**: - querystring.parse() is now 13-22% faster! (Brian White) #4675 PR-URL: #5301

In December we announced that we would be doing a minor release in order to get a number of voted on SEMVER-MINOR changes into LTS. Our ability to release this was delayed due to the unforeseen security release v4.3. We are quickly bumping to v4.4 in order to bring you the features that we had committed to releasing. This release also includes security updates to openssl. More information can be found [on nodejs.org](https://nodejs.org/en/blog/vulnerability/openssl-march-2016/) This release also includes over 70 fixes to our docs and over 50 fixes to tests. The SEMVER-MINOR changes include: * deps: - An update to v8 that introduces a new flag --perf_basic_prof_only_functions (Ali Ijaz Sheikh) #3609 * http: - A new feature in http(s) agent that catches errors on *keep alived* connections (José F. Romaniello) #4482 * src: - Better support for Big-Endian systems (Bryon Leung) #3410 * tls: - A new feature that allows you to pass common SSL options to `tls.createSecurePair` (Коренберг Марк) #2441 * tools - a new flag `--prof-process` which will execute the tick processor on the provided isolate files (Matt Loring) #4021 Notable semver patch changes include: * buld: - Support python path that includes spaces. This should be of particular interest to our Windows users who may have python living in `c:/Program Files` (Felix Becker) #4841 * https: - A potential fix for #3692 HTTP/HTTPS client requests throwing EPROTO (Fedor Indutny) #4982 * installer: - More readable profiling information from isolate tick logs (Matt Loring) #3032 * *npm: - upgrade to npm 2.14.20 (Kat Marchán) #5510 * *openssl: - upgrade openssl to 1.0.2g (Ben Noordhuis) #5507 * process: - Add support for symbols in event emitters. Symbols didn't exist when it was written ¯\_(ツ)_/¯ (cjihrig) #4798 * querystring: - querystring.parse() is now 13-22% faster! (Brian White) #4675 * streams: - performance improvements for moving small buffers that shows a 5% throughput gain. IoT projects have been seen to be as much as 10% faster with this change! (Matteo Collina) #4354 * tools: - eslint has been updated to version 2.1.0 (Rich Trott) #5214 PR-URL: #5301

In December we announced that we would be doing a minor release in order to get a number of voted on SEMVER-MINOR changes into LTS. Our ability to release this was delayed due to the unforeseen security release v4.3. We are quickly bumping to v4.4 in order to bring you the features that we had committed to releasing. This release also includes security updates to openssl. More information can be found [on nodejs.org](https://nodejs.org/en/blog/vulnerability/openssl-march-2016/) This release also includes over 70 fixes to our docs and over 50 fixes to tests. The SEMVER-MINOR changes include: * deps: - An update to v8 that introduces a new flag --perf_basic_prof_only_functions (Ali Ijaz Sheikh) nodejs#3609 * http: - A new feature in http(s) agent that catches errors on *keep alived* connections (José F. Romaniello) nodejs#4482 * src: - Better support for Big-Endian systems (Bryon Leung) nodejs#3410 * tls: - A new feature that allows you to pass common SSL options to `tls.createSecurePair` (Коренберг Марк) nodejs#2441 * tools - a new flag `--prof-process` which will execute the tick processor on the provided isolate files (Matt Loring) nodejs#4021 Notable semver patch changes include: * buld: - Support python path that includes spaces. This should be of particular interest to our Windows users who may have python living in `c:/Program Files` (Felix Becker) nodejs#4841 * https: - A potential fix for nodejs#3692 HTTP/HTTPS client requests throwing EPROTO (Fedor Indutny) nodejs#4982 * installer: - More readable profiling information from isolate tick logs (Matt Loring) nodejs#3032 * *npm: - upgrade to npm 2.14.20 (Kat Marchán) nodejs#5510 * *openssl: - upgrade openssl to 1.0.2g (Ben Noordhuis) nodejs#5507 * process: - Add support for symbols in event emitters. Symbols didn't exist when it was written ¯\_(ツ)_/¯ (cjihrig) nodejs#4798 * querystring: - querystring.parse() is now 13-22% faster! (Brian White) nodejs#4675 * streams: - performance improvements for moving small buffers that shows a 5% throughput gain. IoT projects have been seen to be as much as 10% faster with this change! (Matteo Collina) nodejs#4354 * tools: - eslint has been updated to version 2.1.0 (Rich Trott) nodejs#5214 PR-URL: nodejs#5301

This change adds a new event handler to the `error` event of the socket after it has been used by the http_client. The purpose of this change is to catch errors on *keep alived* connections from idle sockets that otherwise will cause an uncaugh error event on the application. Fix: #3595 PR-URL: #4482 Reviewed-By: Fedor Indutny <fedor@indutny.com>

In December we announced that we would be doing a minor release in order to get a number of voted on SEMVER-MINOR changes into LTS. Our ability to release this was delayed due to the unforeseen security release v4.3. We are quickly bumping to v4.4 in order to bring you the features that we had committed to releasing. This release also includes over 70 fixes to our docs and over 50 fixes to tests. The SEMVER-MINOR changes include: * deps: - An update to v8 that introduces a new flag --perf_basic_prof_only_functions (Ali Ijaz Sheikh) #3609 * http: - A new feature in http(s) agent that catches errors on *keep alived* connections (José F. Romaniello) #4482 * src: - Better support for Big-Endian systems (Bryon Leung) #3410 * tls: - A new feature that allows you to pass common SSL options to `tls.createSecurePair` (Коренберг Марк) #2441 * tools - a new flag `--prof-process` which will execute the tick processor on the provided isolate files (Matt Loring) #4021 Notable semver patch changes include: * buld: - Support python path that includes spaces. This should be of particular interest to our Windows users who may have python living in `c:/Program Files` (Felix Becker) #4841 * https: - A potential fix for #3692 HTTP/HTTPS client requests throwing EPROTO (Fedor Indutny) #4982 * installer: - More readable profiling information from isolate tick logs (Matt Loring) #3032 * *npm: - upgrade to npm 2.14.20 (Kat Marchán) #5510 * process: - Add support for symbols in event emitters. Symbols didn't exist when it was written ¯\_(ツ)_/¯ (cjihrig) #4798 * querystring: - querystring.parse() is now 13-22% faster! (Brian White) #4675 * streams: - performance improvements for moving small buffers that shows a 5% throughput gain. IoT projects have been seen to be as much as 10% faster with this change! (Matteo Collina) #4354 * tools: - eslint has been updated to version 2.1.0 (Rich Trott) #5214 PR-URL: #5301

@jfromaniello @indutny @thealphanerd just upgraded our staging stack to v4.4.0 and did a heavy load test. After the load test finished, I saw the following error:

[Wed, 09 Mar 2016 18:50:55 GMT] [fatal] [default] TypeError: this._evictSession is not a function
    at TLSSocket.<anonymous> (https.js:83:12)
    at TLSSocket.g (events.js:260:16)
    at emitOne (events.js:82:20)
    at TLSSocket.emit (events.js:169:7)
    at TCP._onclose (net.js:475:12)

Thinking it could be related to this issue. I'm having a hard time reproducing the error however. I've done 5 more load tests since then and nothing has come up.

/cc @jasnell @nodejs/lts

@indutny ... any thoughts on this?

For what its worth, it seems to happen in the same situation as #3595 where load is applied and then removed.

@indutny I think this bug should be fixed in _http_agent.js.

@fengmk2 which exactly?

After push free socket to freeSockets https://github.com/nodejs/node/blob/master/lib/_http_agent.js#L77 , then add the freeSocketErrorListener to handle the errors.

// lib/_http_agent.js#L77

socket._httpMessage = null;
self.removeSocket(socket, options);
freeSockets.push(socket);
+++ socket.once('error', freeSocketErrorListener);

I dont understand what's the scenario that we are not handling.

The free event is fired after we put the handler for the error. Check this comment

#3595 (comment)

This change adds a new event handler to the `error` event of the socket after it has been used by the http_client. The purpose of this change is to catch errors on *keep alived* connections from idle sockets that otherwise will cause an uncaugh error event on the application. Fix: nodejs#3595 PR-URL: nodejs#4482 Reviewed-By: Fedor Indutny <fedor@indutny.com>

* http: - A new status code was added: 451 - "Unavailable For Legal Reasons" (Max Barinov) nodejs#4377 - Idle sockets that have been kept alive now handle errors (José F. Romaniello) nodejs#4482 * This release also includes several minor performance improvements: - assert: deepEqual is now speedier when comparing TypedArrays (Claudio Rodriguez) nodejs#4330 - lib: Use arrow functions instead of bind where possible (Minwoo Jung) nodejs#3622 - node: Improved accessor perf of process.env (Trevor Norris) nodejs#3780 - node: Improved performance of process.hrtime() (Trevor Norris) nodejs#3780, (Evan Lucas) nodejs#4484 - node: Improved GetActiveHandles performance (Trevor Norris) nodejs#3780 - util: Use faster iteration in util.format() (Jackson Tian) nodejs#3964 Refs: nodejs#4547 PR-URL: nodejs#4623 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

@nodejs/lts It seems that these changes landed in v4.4.0 and later v4.x versions, so I removed the land-on-v4.x label and added the lts-landed-on-v4.x label. Let me know if I'm missing anything.

if i update node version to 6.11.0, is this problem will be resolved?
{[Error: socket hang up] code : 'ECONNRESET'}

mscdex added the http Issues or PRs related to the http subsystem. label Dec 30, 2015

jfromaniello force-pushed the http_agent_handle_errors_on_idle_sockets branch from e93fd3c to 95d1118 Compare December 31, 2015 14:55

jfromaniello force-pushed the http_agent_handle_errors_on_idle_sockets branch 2 times, most recently from 2e473fd to 5771121 Compare December 31, 2015 15:03

indutny reviewed Jan 4, 2016
View reviewed changes

jfromaniello force-pushed the http_agent_handle_errors_on_idle_sockets branch from 5771121 to 9085144 Compare January 4, 2016 17:45

jfromaniello force-pushed the http_agent_handle_errors_on_idle_sockets branch from 9085144 to 0fd720d Compare January 4, 2016 17:52

jfromaniello force-pushed the http_agent_handle_errors_on_idle_sockets branch from 0fd720d to 077ea54 Compare January 4, 2016 18:06

jfromaniello force-pushed the http_agent_handle_errors_on_idle_sockets branch from 077ea54 to 4b75767 Compare January 4, 2016 18:28

fengmk2 reviewed Mar 10, 2016
View reviewed changes

fengmk2 mentioned this pull request Mar 10, 2016

agentkeepalive detecting ECONNRESET node-modules/agentkeepalive#25

Closed

avwo pushed a commit to avwo/whistle that referenced this pull request Mar 11, 2016

fix: nodejs/node#4482

bce1449

fengmk2 mentioned this pull request Mar 13, 2016

pick handle errors on idle sockets node-modules/agentkeepalive#28

Closed

mikemorris mentioned this pull request Mar 23, 2016

https: evict cached sessions on error #4982

Closed

misterdjules added lts-landed-on-v4.x and removed land-on-v4.x labels Jul 28, 2016

+                  socket.once('error', freeSocketErrorListener);
                   // Mark this socket as available, AFTER user-added end
                   // handlers have a chance to run.
                   process.nextTick(emitFreeNT, socket);

Http(s) Agent: handle errors on idle sockets by jfromaniello · Pull Request #4482 · nodejs/node · GitHub

Uh oh!

Uh oh!

Http(s) Agent: handle errors on idle sockets - #4482

Http(s) Agent: handle errors on idle sockets#4482jfromaniello wants to merge 1 commit intonodejs:masternodejs/node:masterfrom jfromaniello:http_agent_handle_errors_on_idle_socketsjfromaniello/node:http_agent_handle_errors_on_idle_socketsCopy head branch name to clipboard

Conversation

jfromaniello commented Dec 30, 2015

Uh oh!

jasnell commented Dec 30, 2015

Uh oh!

indutny commented Dec 31, 2015

Uh oh!

jfromaniello commented Dec 31, 2015

Uh oh!

jfromaniello commented Dec 31, 2015

Uh oh!

rvagg commented Jan 4, 2016

Uh oh!

indutny Jan 4, 2016

Choose a reason for hiding this comment

Uh oh!

indutny commented Jan 4, 2016

Uh oh!

indutny commented Jan 4, 2016

Uh oh!

jfromaniello commented Jan 4, 2016

Uh oh!

jfromaniello commented Jan 4, 2016

Uh oh!

indutny commented Jan 4, 2016

Uh oh!

jfromaniello commented Jan 4, 2016

Uh oh!

indutny commented Jan 4, 2016

Uh oh!

indutny commented Jan 4, 2016

Uh oh!

jfromaniello commented Jan 4, 2016

Uh oh!

indutny commented Jan 4, 2016

Uh oh!

jfromaniello commented Jan 4, 2016

Uh oh!

indutny commented Jan 4, 2016

Uh oh!

bsudekum commented Mar 9, 2016

Uh oh!

MylesBorins commented Mar 9, 2016

Uh oh!

jasnell commented Mar 9, 2016

Uh oh!

bsudekum commented Mar 9, 2016

Uh oh!

fengmk2 commented Mar 10, 2016

Uh oh!

indutny commented Mar 10, 2016

Uh oh!

fengmk2 Mar 10, 2016

Choose a reason for hiding this comment

Uh oh!

jfromaniello commented Mar 10, 2016

Uh oh!

misterdjules commented Jul 28, 2016

Uh oh!

wangliangyu commented Jun 14, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

Http(s) Agent: handle errors on idle sockets#4482
jfromaniello wants to merge 1 commit into
nodejs:masterfrom
jfromaniello:http_agent_handle_errors_on_idle_sockets