From 31b0b52a712646c1f65f06bf1c507673877d45e2 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 8ceb3aac582b12..91dd398ce6639c 100644
--- a/lib/_http_client.js
+++ b/lib/_http_client.js
@@ -590,39 +590,26 @@ function parserOnIncomingClient(res, shouldKeepAlive) {
 }
 
 // client
-function responseKeepAlive(res, req) {
+function responseKeepAlive(req) {
   const socket = req.socket;
 
-  if (!req.shouldKeepAlive) {
-    if (socket.writable) {
-      debug('AGENT socket.destroySoon()');
-      if (typeof socket.destroySoon === 'function')
-        socket.destroySoon();
-      else
-        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);
-  }
+  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 res = this;
   const req = this.req;
 
   if (req.socket && req.timeoutCb) {
@@ -630,19 +617,32 @@ function responseOnEnd() {
   }
 
   req._ended = true;
-  if (!req.shouldKeepAlive || req.finished)
-    responseKeepAlive(res, req);
+
+  if (!req.shouldKeepAlive) {
+    const socket = req.socket;
+    if (socket.writable) {
+      debug('AGENT socket.destroySoon()');
+      if (typeof socket.destroySoon === 'function')
+        socket.destroySoon();
+      else
+        socket.end();
+    }
+    assert(!socket.writable);
+  } 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 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) {