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 — https: evict cached sessions on error by indutny · Pull Request #4982 · nodejs/node

Conversation

@indutny

@indutny indutny commented Jan 30, 2016

Copy link
Copy Markdown
Member

Instead of using the same session over and over, evict it when the
socket emits error. This could be used as a mitigation of #3692, until
OpenSSL fix will be merged/released.

See: #3692

Sorry, something went wrong.

Uh oh!

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

All reactions
@indutny
https: evict cached sessions on error
356ae9a
Instead of using the same session over and over, evict it when the
socket emits error. This could be used as a mitigation of nodejs#3692, until
OpenSSL fix will be merged/released.

See: nodejs#3692
@indutny indutny mentioned this pull request Jan 30, 2016
HTTP/HTTPS client requests throwing EPROTO #3692
Closed
@indutny

indutny commented Jan 30, 2016

Copy link
Copy Markdown
Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

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

@indutny

indutny commented Jan 30, 2016

Copy link
Copy Markdown
Member Author

cc @nodejs/crypto @nodejs/collaborators

All reactions

Sorry, something went wrong.

Uh oh!

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

evanlucas
evanlucas reviewed Jan 30, 2016
View reviewed changes
Comment thread lib/https.js Outdated

@evanlucas evanlucas Jan 30, 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.

Maybe use an arrow function here?

Sorry, something went wrong.

Uh oh!

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

All reactions

@indutny indutny Jan 31, 2016

Copy link
Copy Markdown
Member Author

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.

Good point, fixed.

Sorry, something went wrong.

Uh oh!

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

All reactions
@indutny
fix
b9b79fd
@evanlucas

evanlucas commented Jan 31, 2016

Copy link
Copy Markdown
Contributor

LGTM, but you probably want to get someone else from crypto to sign off

All reactions

Sorry, something went wrong.

Uh oh!

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

shigeki
shigeki reviewed Jan 31, 2016
View reviewed changes
Comment thread test/parallel/test-https-agent-session-eviction.js

@shigeki shigeki Jan 31, 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.

How about adding more strict asset check?

--- a/test/parallel/test-https-agent-session-eviction.js
+++ b/test/parallel/test-https-agent-session-eviction.js
@@ -77,7 +77,12 @@ function third(server) {
     rejectUnauthorized: false
   }, function(res) {
     res.resume();
+    assert(!req.socket.isSessionReused());
     server.close();
   });
+  req.on('error', function(err) {
+    // never called
+    assert(false);
+  });
   req.end();
 }

Sorry, something went wrong.

Uh oh!

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

All reactions

@indutny indutny Feb 2, 2016

Copy link
Copy Markdown
Member Author

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.

Ack, agree!

Sorry, something went wrong.

Uh oh!

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

All reactions
@shigeki

shigeki commented Jan 31, 2016

Copy link
Copy Markdown
Contributor

Just commented minor nits. LGTM.

All reactions

Sorry, something went wrong.

Uh oh!

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

@targos targos added the https Issues and PRs related to the https subsystem. label Jan 31, 2016
@indutny

indutny commented Feb 2, 2016

Copy link
Copy Markdown
Member Author

Landed in 165b33f, thank you!

All reactions

Sorry, something went wrong.

Uh oh!

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

@indutny indutny closed this Feb 2, 2016
@indutny
indutny deleted the fix/https-session-eviction-on-error branch February 2, 2016 02:53
indutny added a commit that referenced this pull request Feb 2, 2016
@indutny
https: evict cached sessions on error
165b33f
Instead of using the same session over and over, evict it when the
socket emits error. This could be used as a mitigation of #3692, until
OpenSSL fix will be merged/released.

See: #3692
PR-URL: #4982
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
@tristanls

tristanls commented Feb 2, 2016

Copy link
Copy Markdown
Contributor

\o/

All reactions

Sorry, something went wrong.

Uh oh!

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

@jasnell jasnell added the lts-watch-v4.x label Feb 2, 2016
rvagg pushed a commit that referenced this pull request Feb 8, 2016
@indutny @rvagg
https: evict cached sessions on error
101de9d
Instead of using the same session over and over, evict it when the
socket emits error. This could be used as a mitigation of #3692, until
OpenSSL fix will be merged/released.

See: #3692
PR-URL: #4982
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
@rvagg rvagg mentioned this pull request Feb 8, 2016
Release proposal: v5.5.1 (Stable) #5141
Closed
@Smbc1 Smbc1 mentioned this pull request Feb 10, 2016
DynamoDB write EPROTO aws/aws-sdk-js#862
Closed
MylesBorins pushed a commit that referenced this pull request Feb 18, 2016
@indutny Myles Borins
https: evict cached sessions on error
0afb77b
Instead of using the same session over and over, evict it when the
socket emits error. This could be used as a mitigation of #3692, until
OpenSSL fix will be merged/released.

See: #3692
PR-URL: #4982
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
@MylesBorins MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Feb 18, 2016
MylesBorins pushed a commit that referenced this pull request Feb 18, 2016
@indutny Myles Borins
https: evict cached sessions on error
de5177c
Instead of using the same session over and over, evict it when the
socket emits error. This could be used as a mitigation of #3692, until
OpenSSL fix will be merged/released.

See: #3692
PR-URL: #4982
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Feb 18, 2016
V4.4.0 proposal #5301
Merged
MylesBorins pushed a commit that referenced this pull request Feb 18, 2016
Myles Borins
2016-02-18, Version 4.4.0 'Argon' (LTS)
5c5479e
Notable changes:

This update to the LTS line includes a number of semver minor changes
that have been staged for a number of months. This includes:

  * deps: backport 9da3ab6 from V8 upstream (Ali Ijaz Sheikh)
    - #3609
  * http: handle errors on idle sockets (José F. Romaniello)
    - #4482
  * src: add BE support to StringBytes::Encode() (Bryon Leung)
    - #3410
  * tls: add `options` argument to createSecurePair (Коренберг Марк)
    - #2441

There are also quite a large number of semver patch changes
including over 20 doc fixes and almost 50 test fixes.

Notable semver patch changes include:

  * deps: upgrade to npm 2.14.18 (Kat Marchán)
    - #5245
  * https: evict cached sessions on error (Fedor Indutny)
    - #4982
  * process: support symbol events (cjihrig)
    - #4798
  * querystring: improve parse() performance (Brian White)
    - #4675

PR-URL: #5301
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
@indutny Myles Borins
https: evict cached sessions on error
51941a1
Instead of using the same session over and over, evict it when the
socket emits error. This could be used as a mitigation of #3692, until
OpenSSL fix will be merged/released.

See: #3692
PR-URL: #4982
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Shigeki Ohtsu <[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
@mikemorris

mikemorris commented Mar 23, 2016

Copy link
Copy Markdown

Moving the discussion from #4482 (comment) over to here, as I think this is the relevant pull request that introduced the problematic code.

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

We're seeing this error relatively frequently on active servers running Node.js v4.4.0. Previously, we were seeing unhandled errors on idle sockets, but that was addressed in #4482. I'm curious if the volume of idle sockets could have been exacerbated prior to the fix in 2e1ae3e too.

I'm guessing could be as simple as an incorrectly bound this context. I initially thought it could be the fault of the arrow function in the socket.once call, but I think I disproved that theory. I haven't been able to distill a simple test case to reproduce this yet, but it looks like it originates from the attempted destruction of an idle socket initially created with a KeepAlive session.

Wondering if #2534 could be at all relevant here?

All reactions

Sorry, something went wrong.

Uh oh!

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

@indutny

indutny commented Mar 23, 2016

Copy link
Copy Markdown
Member Author

Indeed it is most likely some incorrectly propagated this. I suspect that you are using some module that does call agent.createConnection without the context, because core itself sets up the context properly.

All reactions

Sorry, something went wrong.

Uh oh!

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

@mikemorris

mikemorris commented Mar 23, 2016

Copy link
Copy Markdown

Thanks @indutny - it looks like it was node-modules/agentkeepalive#34. Do you think #2534 would eliminate the need for the agentkeepalive dependency if/when it lands?

All reactions

Sorry, something went wrong.

Uh oh!

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

@indutny

indutny commented Mar 24, 2016

Copy link
Copy Markdown
Member Author

@mikemorris I don't think that #2534 has anything to do with the http agent. What is the problem that agentkeepalive solves?

All reactions

Sorry, something went wrong.

Uh oh!

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

@mikemorris

mikemorris commented Mar 24, 2016

Copy link
Copy Markdown

@indutny I'm not too familiar with the details here, but my understanding is that this might be a solved problem in Node.js v4+ that we just haven't tested yet. agentkeepalive was being used to keep sockets available for reuse instead of immediately closing them if there was not already another request pending.

node 0.10.x does need it, and until a certain point, node 0.12 needed it as well
in a nutshell, the way the agent worked in 0.10 was that whenever the agent would make a request, it would only ever reuse a socket if, in the same tick, there were other requests in the request queue. agentkeepalive changed this by allowing sockets to be reused up until the specified timeout.
so this greatly reduced the number of sockets that are ever being opened and closed, and increased the number of sockets that'd get reused

All reactions

Sorry, something went wrong.

Uh oh!

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

scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
@indutny Michael Scovetta
https: evict cached sessions on error
aa93c8e
Instead of using the same session over and over, evict it when the
socket emits error. This could be used as a mitigation of nodejs#3692, until
OpenSSL fix will be merged/released.

See: nodejs#3692
PR-URL: nodejs#4982
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
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

https Issues and PRs related to the https subsystem.

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.

8 participants

@indutny @evanlucas @shigeki @tristanls @mikemorris @jasnell @MylesBorins @targos
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.