From 627bb6bf8716131c942ea8a274390d09d1984923 Mon Sep 17 00:00:00 2001
From: Robert Nagy <ronagy@icloud.com>
Date: Mon, 15 Jul 2019 19:19:12 +0200
Subject: [PATCH] http: refactor responseKeepAlive()

This tries to simplify the code and make it easier to understand. Took
me a while to get my head around the previous implementation.

Also minor perf improvement.

PR-URL: https://github.com/nodejs/node/pull/28700
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
---
 lib/_http_client.js | 72 ++++++++++++++++++++++-----------------------
 1 file changed, 36 insertions(+), 36 deletions(-)

diff --git a/lib/_http_client.js b/lib/_http_client.js
index 1ab4dc95c5..0c1c9a22f2 100644
--- a/lib/_http_client.js
+++ b/lib/_http_client.js
@@ -594,10 +594,36 @@ function parserOnIncomingClient(res, shouldKeepAlive) {
 }
 
 // client
-function responseKeepAlive(res, req) {
+function responseKeepAlive(req) {
   const socket = req.socket;
 
+  debug('AGENT socket keep-alive');
+  if (req.timeoutCb) {
+    socket.setTimeout(0, req.timeoutCb);
+    req.timeoutCb = null;
+  }
+  socket.removeListener('close', socketCloseListener);
+  socket.removeListener('error', socketErrorListener);
+  socket.once('error', freeSocketErrorListener);
+  // There are cases where _handle === null. Avoid those. Passing null to
+  // nextTick() will call getDefaultTriggerAsyncId() to retrieve the id.
+  const asyncId = socket._handle ? socket._handle.getAsyncId() : undefined;
+  // Mark this socket as available, AFTER user-added end
+  // handlers have a chance to run.
+  defaultTriggerAsyncIdScope(asyncId, process.nextTick, emitFreeNT, socket);
+}
+
+function responseOnEnd() {
+  const req = this.req;
+
+  if (req.socket && req.timeoutCb) {
+    req.socket.removeListener('timeout', emitRequestTimeout);
+  }
+
+  req._ended = true;
+
   if (!req.shouldKeepAlive) {
+    const socket = req.socket;
     if (socket.writable) {
       debug('AGENT socket.destroySoon()');
       if (typeof socket.destroySoon === 'function')
@@ -606,47 +632,21 @@ function responseKeepAlive(res, req) {
         socket.end();
     }
     assert(!socket.writable);
-  } else {
-    debug('AGENT socket keep-alive');
-    if (req.timeoutCb) {
-      socket.setTimeout(0, req.timeoutCb);
-      req.timeoutCb = null;
-    }
-    socket.removeListener('close', socketCloseListener);
-    socket.removeListener('error', socketErrorListener);
-    socket.removeListener('drain', ondrain);
-    socket.once('error', freeSocketErrorListener);
-    // There are cases where _handle === null. Avoid those. Passing null to
-    // nextTick() will call getDefaultTriggerAsyncId() to retrieve the id.
-    const asyncId = socket._handle ? socket._handle.getAsyncId() : undefined;
-    // Mark this socket as available, AFTER user-added end
-    // handlers have a chance to run.
-    defaultTriggerAsyncIdScope(asyncId, process.nextTick, emitFreeNT, socket);
+  } else if (req.finished) {
+    // We can assume `req.finished` means all data has been written since:
+    // - `'responseOnEnd'` means we have been assigned a socket.
+    // - when we have a socket we write directly to it without buffering.
+    // - `req.finished` means `end()` has been called and no further data.
+    //   can be written
+    responseKeepAlive(req);
   }
 }
 
-function responseOnEnd() {
-  const res = this;
-  const req = this.req;
-
-  if (req.socket && req.timeoutCb) {
-    req.socket.removeListener('timeout', emitRequestTimeout);
-  }
-
-  req._ended = true;
-  if (!req.shouldKeepAlive || req.finished)
-    responseKeepAlive(res, req);
-}
-
 function requestOnPrefinish() {
   const req = this;
-  const res = this.res;
 
-  if (!req.shouldKeepAlive)
-    return;
-
-  if (req._ended)
-    responseKeepAlive(res, req);
+  if (req.shouldKeepAlive && req._ended)
+    responseKeepAlive(req);
 }
 
 function emitFreeNT(socket) {