From 5af8b39c968fc03d4aa0134ef6f2ce9ea621f463 Mon Sep 17 00:00:00 2001
From: Rusty Conover <rusty@conover.me>
Date: Sun, 26 Jan 2020 14:19:39 -0500
Subject: [PATCH 1/2] http: Change free sockets behavior to LIFO from FIFO.

Sockets are added to the free list with .push() but they were
being removed with .shift().  This meant the sockets where being
removed in FIFO order, but this changes it to LIFO.  Since older
sockets may be closed based due to inactivity on the server it is
more likely that a socket that is recently used will be able to
successfully process the next request.

Rather than destroying the last used socket destroy
the oldest socket in the free list in push() on the
last recently used socket.
---
 lib/_http_agent.js | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/lib/_http_agent.js b/lib/_http_agent.js
index 5e3298b594d949..eff456cf38b089 100644
--- a/lib/_http_agent.js
+++ b/lib/_http_agent.js
@@ -112,11 +112,16 @@ function Agent(options) {
         if (this.sockets[name])
           count += this.sockets[name].length;
 
-        if (count > this.maxSockets || freeLen >= this.maxFreeSockets) {
-          socket.destroy();
-        } else if (this.keepSocketAlive(socket)) {
-          freeSockets = freeSockets || [];
-          this.freeSockets[name] = freeSockets;
+        if (this.keepSocketAlive(socket) &&
+            this.maxFreeSockets > 0 &&
+            count <= this.maxSockets) {
+          if (freeLen >= this.maxFreeSockets) {
+            const oldest = this.freeSockets[name].shift();
+            oldest.destroy();
+          } else {
+            freeSockets = freeSockets || [];
+            this.freeSockets[name] = freeSockets;
+          }
           socket[async_id_symbol] = -1;
           socket._httpMessage = null;
           this.removeSocket(socket, options);
@@ -207,7 +212,7 @@ Agent.prototype.addRequest = function addRequest(req, options, port/* legacy */,
 
   if (freeLen) {
     // We have a free socket, so use that.
-    const socket = this.freeSockets[name].shift();
+    const socket = this.freeSockets[name].pop();
     // Guard against an uninitialized or user supplied Socket.
     const handle = socket._handle;
     if (handle && typeof handle.asyncReset === 'function') {

From b43479fecc57ad51a73ba1f4521acccbeb05a44a Mon Sep 17 00:00:00 2001
From: Rusty Conover <rusty@conover.me>
Date: Sat, 29 Feb 2020 00:06:09 -0500
Subject: [PATCH 2/2] test: Add test http agent keepalive LIFO behavior

Add a test that ensures the HTTP agent reuses sockets
in a LIFO fashion rather than FIFO.
---
 .../test-http-keepalive-lifo-sockets.js       | 95 +++++++++++++++++++
 1 file changed, 95 insertions(+)
 create mode 100644 test/sequential/test-http-keepalive-lifo-sockets.js

diff --git a/test/sequential/test-http-keepalive-lifo-sockets.js b/test/sequential/test-http-keepalive-lifo-sockets.js
new file mode 100644
index 00000000000000..af369cf75fac6c
--- /dev/null
+++ b/test/sequential/test-http-keepalive-lifo-sockets.js
@@ -0,0 +1,95 @@
+'use strict';
+const common = require('../common');
+const assert = require('assert');
+const http = require('http');
+
+// To test the LIFO behavior of the agent:
+// Send a request and store the local TCP port
+// number used to send a request to the server.
+
+// Store a list of port numbers used to process requests...
+let port_list = [];
+
+const server = http.createServer(common.mustCallAtLeast((req, res) => {
+  // Return the remote port number used for this connection.
+  res.end(req.socket.remotePort.toString(10));
+}), 6);
+
+server.listen(0, function() {
+  const agent = http.Agent({
+    keepAlive: true,
+    maxSockets: 4
+  });
+
+  // Fill the agent with connections
+  const fill_request_count = agent.maxSockets;
+
+  makeReqs(fill_request_count, function(er) {
+    assert.ifError(er);
+    // Assert that different ports were used.
+    assert.strictEqual(port_list.length, fill_request_count);
+
+    // Now the agent is fully populated with connections to the server
+    // just waiting to be reused.
+
+    // For LIFO behavior the last used port should always be used
+    // to send a serial string of requests.
+
+    // If there was FIFO behavior for each serial request a
+    // the oldest used port should be used to send the request.
+
+    const fill_ports = port_list;
+    port_list = [];
+    makeReq(common.mustCall(() => {
+      // Since the agent's free socket list isn't updated
+      // until after the response is processed. Send the next
+      // request completely after the first one is finished processing.
+      process.nextTick(common.mustCall(() => {
+        makeReq(common.mustCall(() => {
+          // LIFO should always use the last used port to send the next
+          // request, so verify that behavior.
+          const lastPort = fill_ports[fill_ports.length - 1];
+          assert.deepStrictEqual(port_list, [lastPort, lastPort]);
+          agent.destroy();
+          server.close();
+        }));
+      }));
+    }));
+  });
+
+  process.on('exit', function() {
+    console.log('ok');
+  });
+
+  function makeReqs(n, cb) {
+    for (let i = 0; i < n; i++) makeReq(then);
+
+    function then(er) {
+      if (er) return cb(er);
+      else if (--n === 0) process.nextTick(cb);
+    }
+  }
+
+  function makeReq(cb) {
+    http
+      .request(
+        {
+          port: server.address().port,
+          path: '/',
+          agent: agent
+        },
+        function(res) {
+          let data = '';
+          res.setEncoding('ascii');
+          res.on('data', function(c) {
+            data += c;
+          });
+          res.on('end', function() {
+            port_list.push(data);
+            cb();
+          });
+        }
+      )
+      .end();
+  }
+});