https: evict cached sessions on error

[indutny]

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

[indutny]

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

[indutny]

cc @nodejs/crypto @nodejs/collaborators

[evanlucas]

Maybe use an arrow function here?

[indutny]

Good point, fixed.

[evanlucas]

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

[shigeki]

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

[indutny]

Ack, agree!

[shigeki]

Just commented minor nits. LGTM.

[indutny]

Landed in 165b33f, thank you!

[tristanls]

\o/

[mikemorris]

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?

[indutny]

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.

[mikemorris]

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?

[indutny]

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

[mikemorris]

@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