GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Http(s) Agent: handle errors on idle sockets by jfromaniello · Pull Request #4482 · nodejs/node

Conversation

@jfromaniello

@jfromaniello jfromaniello commented Dec 30, 2015

Copy link
Copy Markdown
Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@mscdex mscdex added the http Issues and PRs related to the http subsystem. label Dec 30, 2015
@jasnell

jasnell commented Dec 30, 2015

Copy link
Copy Markdown
Member

@nodejs/http

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@indutny

indutny commented Dec 31, 2015

Copy link
Copy Markdown
Member

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@jfromaniello

jfromaniello commented Dec 31, 2015

Copy link
Copy Markdown
Contributor Author

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!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

jfromaniello commented Dec 31, 2015

Copy link
Copy Markdown
Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@jfromaniello
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
@rvagg

rvagg commented Jan 4, 2016

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

indutny
indutny reviewed Jan 4, 2016
View reviewed changes
Comment thread test/parallel/test-http-agent-error-on-idle.js Outdated

@indutny indutny Jan 4, 2016

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Missing newline.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@indutny

indutny commented Jan 4, 2016

Copy link
Copy Markdown
Member

Only a style nits, otherwise LGTM. Thank you!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@indutny

indutny commented Jan 4, 2016

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@jfromaniello

jfromaniello commented Jan 4, 2016

Copy link
Copy Markdown
Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

jfromaniello commented Jan 4, 2016

Copy link
Copy Markdown
Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@indutny

indutny commented Jan 4, 2016

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

jfromaniello commented Jan 4, 2016

Copy link
Copy Markdown
Contributor Author

I just fixed the linter problems :)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@indutny

indutny commented Jan 4, 2016

Copy link
Copy Markdown
Member

Fantastic! LGTM

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@indutny

indutny commented Jan 4, 2016

Copy link
Copy Markdown
Member

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:

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

Thank you, and sorry for not posting it before.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

jfromaniello commented Jan 4, 2016

Copy link
Copy Markdown
Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@indutny

indutny commented Jan 4, 2016

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@jfromaniello
http: handle errors on idle sockets
4b75767
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
@jfromaniello
jfromaniello force-pushed the http_agent_handle_errors_on_idle_sockets branch from 077ea54 to 4b75767 Compare January 4, 2016 18:28
@jfromaniello

jfromaniello commented Jan 4, 2016

Copy link
Copy Markdown
Contributor Author

oops , done!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@indutny

indutny commented Jan 4, 2016

Copy link
Copy Markdown
Member

Yay, looks great. Landing.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

MylesBorins pushed a commit that referenced this pull request Feb 23, 2016
Myles Borins
2016-02-22, Version 4.4.0 'Argon' (LTS)
75805d3
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
MylesBorins pushed a commit that referenced this pull request Mar 1, 2016
Myles Borins
2016-03-01, Version 4.4.0 'Argon' (LTS)
f4f397f
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
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Mar 1, 2016
Myles Borins
2016-03-01, Version 4.4.0 'Argon' (LTS)
607aaea
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
MylesBorins pushed a commit that referenced this pull request Mar 2, 2016
@jfromaniello Myles Borins
http: handle errors on idle sockets
ebb9f0d
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 <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 2, 2016
Myles Borins
2016-03-01, Version 4.4.0 'Argon' (LTS)
e2521c1
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
MylesBorins pushed a commit that referenced this pull request Mar 3, 2016
Myles Borins
2016-03-03, Version 4.4.0 'Argon' (LTS)
4030a2b
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
MylesBorins pushed a commit that referenced this pull request Mar 8, 2016
Myles Borins
2016-03-08, Version 4.4.0 'Argon' (LTS)
83ca8fa
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
MylesBorins pushed a commit that referenced this pull request Mar 9, 2016
Myles Borins
2016-03-08, Version 4.4.0 'Argon' (LTS)
9277aed
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
@bsudekum

bsudekum commented Mar 9, 2016

Copy link
Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@MylesBorins

MylesBorins commented Mar 9, 2016

Copy link
Copy Markdown
Contributor

/cc @jasnell @nodejs/lts

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@jasnell

jasnell commented Mar 9, 2016

Copy link
Copy Markdown
Member

@indutny ... any thoughts on this?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bsudekum

bsudekum commented Mar 9, 2016

Copy link
Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@fengmk2

fengmk2 commented Mar 10, 2016

Copy link
Copy Markdown
Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@indutny

indutny commented Mar 10, 2016

Copy link
Copy Markdown
Member

@fengmk2 which exactly?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

fengmk2
fengmk2 reviewed Mar 10, 2016
View reviewed changes
Comment thread lib/_http_client.js
socket.once('error', freeSocketErrorListener);
// Mark this socket as available, AFTER user-added end
// handlers have a chance to run.
process.nextTick(emitFreeNT, socket);

@fengmk2 fengmk2 Mar 10, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@fengmk2 fengmk2 mentioned this pull request Mar 10, 2016
agentkeepalive detecting ECONNRESET node-modules/agentkeepalive#25
Closed
@jfromaniello

jfromaniello commented Mar 10, 2016

Copy link
Copy Markdown
Contributor Author

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)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

avwo pushed a commit to avwo/whistle that referenced this pull request Mar 11, 2016
avenwu.wwb
fix: nodejs/node#4482
bce1449
@fengmk2 fengmk2 mentioned this pull request Mar 13, 2016
pick handle errors on idle sockets node-modules/agentkeepalive#28
Closed
@mikemorris mikemorris mentioned this pull request Mar 23, 2016
https: evict cached sessions on error #4982
Closed
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
@jfromaniello Michael Scovetta
http: handle errors on idle sockets
966c110
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 <[email protected]>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
@Fishrock123 Michael Scovetta
2016-01-06, Version 5.4.0 (Stable)
3991aa8
* 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 (José 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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@misterdjules misterdjules added lts-landed-on-v4.x and removed land-on-v4.x labels Jul 28, 2016
@misterdjules

misterdjules commented Jul 28, 2016

Copy link
Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@wangliangyu

wangliangyu commented Jun 14, 2017

Copy link
Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

http Issues and 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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

11 participants

@jfromaniello @jasnell @indutny @rvagg @mikemorris @bsudekum @MylesBorins @fengmk2 @misterdjules @wangliangyu @mscdex
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.