From 845e15d930b16f023ca3fa43f37214b852a50685 Mon Sep 17 00:00:00 2001
From: Rich Trott <rtrott@gmail.com>
Date: Wed, 19 Jul 2017 23:12:32 -0700
Subject: [PATCH 1/8] test: fix flaky http(s)-set-server-timeout tests

The tests include a callback that might not be invoked but is wrapped in
common.mustCall(). Remove the common.mustCall() wrapper and add a
comment explaining that it should not be added.

Add a new test case that sets the timeout to 1ms and waits for both the
connection handler and the timeout handler to be invoked. This version
keeps the common.mustCall() wrapper intact around the connection handler
(although it's mostly semantic and not necessary for the test as the
test will certainly fail or time out if that handler isn't invoked).

Fixes: https://github.com/nodejs/node/issues/11768
---
 test/parallel/test-http-set-timeout-server.js | 43 ++++++++++++++-
 .../parallel/test-https-set-timeout-server.js | 54 ++++++++++++++++---
 2 files changed, 88 insertions(+), 9 deletions(-)

diff --git a/test/parallel/test-http-set-timeout-server.js b/test/parallel/test-http-set-timeout-server.js
index 4eb41d01f96b26..5aa6e6815a3549 100644
--- a/test/parallel/test-http-set-timeout-server.js
+++ b/test/parallel/test-http-set-timeout-server.js
@@ -152,11 +152,12 @@ test(function serverResponseTimeoutWithPipeline(cb) {
 });
 
 test(function idleTimeout(cb) {
-  const server = http.createServer(common.mustCall((req, res) => {
+  // Do not wrap the callback in common.mustCall(). It might not be invoked.
+  const server = http.createServer((req, res) => {
     req.on('timeout', common.mustNotCall());
     res.on('timeout', common.mustNotCall());
     res.end();
-  }));
+  });
   const s = server.setTimeout(50, common.mustCall((socket) => {
     socket.destroy();
     server.close();
@@ -174,3 +175,41 @@ test(function idleTimeout(cb) {
     });
   }));
 });
+
+test(function fastTimeout(cb) {
+  let connectionHandlerInvoked = false;
+  let timeoutHandlerInvoked = false;
+  let connectionSocket;
+
+  function invokeCallbackIfDone() {
+    if (connectionHandlerInvoked && timeoutHandlerInvoked) {
+      connectionSocket.destroy();
+      server.close();
+      cb();
+    }
+  }
+
+  const server = http.createServer(common.mustCall((req, res) => {
+    req.on('timeout', common.mustNotCall());
+    res.on('timeout', common.mustNotCall());
+    res.end();
+    connectionHandlerInvoked = true;
+    invokeCallbackIfDone();
+  }));
+  const s = server.setTimeout(1, common.mustCall((socket) => {
+    connectionSocket = socket;
+    timeoutHandlerInvoked = true;
+    invokeCallbackIfDone();
+  }));
+  assert.ok(s instanceof http.Server);
+  server.listen(common.mustCall(() => {
+    const options = {
+      port: server.address().port,
+      allowHalfOpen: true,
+    };
+    const c = net.connect(options, () => {
+      c.write('GET /1 HTTP/1.1\r\nHost: localhost\r\n\r\n');
+      // Keep-Alive
+    });
+  }));
+});
diff --git a/test/parallel/test-https-set-timeout-server.js b/test/parallel/test-https-set-timeout-server.js
index 76425385233dee..907705bf6c490c 100644
--- a/test/parallel/test-https-set-timeout-server.js
+++ b/test/parallel/test-https-set-timeout-server.js
@@ -175,13 +175,12 @@ test(function serverResponseTimeoutWithPipeline(cb) {
 });
 
 test(function idleTimeout(cb) {
-  const server = https.createServer(
-    serverOptions,
-    common.mustCall((req, res) => {
-      req.on('timeout', common.mustNotCall());
-      res.on('timeout', common.mustNotCall());
-      res.end();
-    }));
+  // Do not wrap the callback in common.mustCall(). It might not be invoked.
+  const server = https.createServer(serverOptions, (req, res) => {
+    req.on('timeout', common.mustNotCall());
+    res.on('timeout', common.mustNotCall());
+    res.end();
+  });
   const s = server.setTimeout(50, common.mustCall((socket) => {
     socket.destroy();
     server.close();
@@ -200,3 +199,44 @@ test(function idleTimeout(cb) {
     });
   }));
 });
+
+test(function fastTimeout(cb) {
+  let connectionHandlerInvoked = false;
+  let timeoutHandlerInvoked = false;
+  let connectionSocket;
+
+  function invokeCallbackIfDone() {
+    if (connectionHandlerInvoked && timeoutHandlerInvoked) {
+      connectionSocket.destroy();
+      server.close();
+      cb();
+    }
+  }
+
+  const server = https.createServer(serverOptions, common.mustCall(
+    (req, res) => {
+      req.on('timeout', common.mustNotCall());
+      res.on('timeout', common.mustNotCall());
+      res.end();
+      connectionHandlerInvoked = true;
+      invokeCallbackIfDone();
+    }
+  ));
+  const s = server.setTimeout(1, common.mustCall((socket) => {
+    connectionSocket = socket;
+    timeoutHandlerInvoked = true;
+    invokeCallbackIfDone();
+  }));
+  assert.ok(s instanceof https.Server);
+  server.listen(common.mustCall(() => {
+    const options = {
+      port: server.address().port,
+      allowHalfOpen: true,
+      rejectUnauthorized: false
+    };
+    const c = tls.connect(options, () => {
+      c.write('GET /1 HTTP/1.1\r\nHost: localhost\r\n\r\n');
+      // Keep-Alive
+    });
+  }));
+});

From 18086fdca079cd7d1ce28a277cc78539ef7b80fe Mon Sep 17 00:00:00 2001
From: Rich Trott <rtrott@gmail.com>
Date: Fri, 21 Jul 2017 11:01:33 -0700
Subject: [PATCH 2/8] squash: remove functions that might not be invoked

---
 test/parallel/test-http-set-timeout-server.js  | 12 ++----------
 test/parallel/test-https-set-timeout-server.js | 14 ++------------
 2 files changed, 4 insertions(+), 22 deletions(-)

diff --git a/test/parallel/test-http-set-timeout-server.js b/test/parallel/test-http-set-timeout-server.js
index 5aa6e6815a3549..199bfb29b4306d 100644
--- a/test/parallel/test-http-set-timeout-server.js
+++ b/test/parallel/test-http-set-timeout-server.js
@@ -42,10 +42,7 @@ function run() {
 }
 
 test(function serverTimeout(cb) {
-  const server = http.createServer((req, res) => {
-    // Do nothing. We should get a timeout event.
-    // Might not be invoked. Do not wrap in common.mustCall().
-  });
+  const server = http.createServer();
   server.listen(common.mustCall(() => {
     const s = server.setTimeout(50, common.mustCall((socket) => {
       socket.destroy();
@@ -152,12 +149,7 @@ test(function serverResponseTimeoutWithPipeline(cb) {
 });
 
 test(function idleTimeout(cb) {
-  // Do not wrap the callback in common.mustCall(). It might not be invoked.
-  const server = http.createServer((req, res) => {
-    req.on('timeout', common.mustNotCall());
-    res.on('timeout', common.mustNotCall());
-    res.end();
-  });
+  const server = http.createServer();
   const s = server.setTimeout(50, common.mustCall((socket) => {
     socket.destroy();
     server.close();
diff --git a/test/parallel/test-https-set-timeout-server.js b/test/parallel/test-https-set-timeout-server.js
index 907705bf6c490c..9a7fae8ba8601d 100644
--- a/test/parallel/test-https-set-timeout-server.js
+++ b/test/parallel/test-https-set-timeout-server.js
@@ -52,12 +52,7 @@ function run() {
 }
 
 test(function serverTimeout(cb) {
-  const server = https.createServer(
-    serverOptions,
-    (req, res) => {
-      // Do nothing. We should get a timeout event.
-      // Might not be invoked. Do not wrap in common.mustCall().
-    });
+  const server = https.createServer(serverOptions);
   server.listen(common.mustCall(() => {
     const s = server.setTimeout(50, common.mustCall((socket) => {
       socket.destroy();
@@ -175,12 +170,7 @@ test(function serverResponseTimeoutWithPipeline(cb) {
 });
 
 test(function idleTimeout(cb) {
-  // Do not wrap the callback in common.mustCall(). It might not be invoked.
-  const server = https.createServer(serverOptions, (req, res) => {
-    req.on('timeout', common.mustNotCall());
-    res.on('timeout', common.mustNotCall());
-    res.end();
-  });
+  const server = https.createServer(serverOptions);
   const s = server.setTimeout(50, common.mustCall((socket) => {
     socket.destroy();
     server.close();

From eef98b166303f2693a742ea427ef0ccb070d2094 Mon Sep 17 00:00:00 2001
From: Rich Trott <rtrott@gmail.com>
Date: Fri, 21 Jul 2017 11:15:01 -0700
Subject: [PATCH 3/8] squash: move timeout handler setting to end callback

---
 test/parallel/test-http-set-timeout-server.js  | 3 +--
 test/parallel/test-https-set-timeout-server.js | 3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/test/parallel/test-http-set-timeout-server.js b/test/parallel/test-http-set-timeout-server.js
index 199bfb29b4306d..220dd1f3ccf6d7 100644
--- a/test/parallel/test-http-set-timeout-server.js
+++ b/test/parallel/test-http-set-timeout-server.js
@@ -183,8 +183,7 @@ test(function fastTimeout(cb) {
 
   const server = http.createServer(common.mustCall((req, res) => {
     req.on('timeout', common.mustNotCall());
-    res.on('timeout', common.mustNotCall());
-    res.end();
+    res.end(() => { res.on('timeout', common.mustNotCall()); });
     connectionHandlerInvoked = true;
     invokeCallbackIfDone();
   }));
diff --git a/test/parallel/test-https-set-timeout-server.js b/test/parallel/test-https-set-timeout-server.js
index 9a7fae8ba8601d..7d3204e0d15aaf 100644
--- a/test/parallel/test-https-set-timeout-server.js
+++ b/test/parallel/test-https-set-timeout-server.js
@@ -206,8 +206,7 @@ test(function fastTimeout(cb) {
   const server = https.createServer(serverOptions, common.mustCall(
     (req, res) => {
       req.on('timeout', common.mustNotCall());
-      res.on('timeout', common.mustNotCall());
-      res.end();
+      res.end(() => { res.on('timeout', common.mustNotCall()); });
       connectionHandlerInvoked = true;
       invokeCallbackIfDone();
     }

From 7f7712e87a66f0aed27c72fceda3d2b904e6b060 Mon Sep 17 00:00:00 2001
From: Rich Trott <rtrott@gmail.com>
Date: Fri, 21 Jul 2017 21:12:59 -0700
Subject: [PATCH 4/8] squash: remove erroneous check that timeout is not
 emitted

---
 test/parallel/test-http-set-timeout-server.js  | 2 +-
 test/parallel/test-https-set-timeout-server.js | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/test/parallel/test-http-set-timeout-server.js b/test/parallel/test-http-set-timeout-server.js
index 220dd1f3ccf6d7..6408e850efb6a5 100644
--- a/test/parallel/test-http-set-timeout-server.js
+++ b/test/parallel/test-http-set-timeout-server.js
@@ -183,7 +183,7 @@ test(function fastTimeout(cb) {
 
   const server = http.createServer(common.mustCall((req, res) => {
     req.on('timeout', common.mustNotCall());
-    res.end(() => { res.on('timeout', common.mustNotCall()); });
+    res.end();
     connectionHandlerInvoked = true;
     invokeCallbackIfDone();
   }));
diff --git a/test/parallel/test-https-set-timeout-server.js b/test/parallel/test-https-set-timeout-server.js
index 7d3204e0d15aaf..f4abfb32b0df58 100644
--- a/test/parallel/test-https-set-timeout-server.js
+++ b/test/parallel/test-https-set-timeout-server.js
@@ -206,7 +206,7 @@ test(function fastTimeout(cb) {
   const server = https.createServer(serverOptions, common.mustCall(
     (req, res) => {
       req.on('timeout', common.mustNotCall());
-      res.end(() => { res.on('timeout', common.mustNotCall()); });
+      res.end();
       connectionHandlerInvoked = true;
       invokeCallbackIfDone();
     }

From 5394374aa4ca292ae69b938910cc82a694f3d94f Mon Sep 17 00:00:00 2001
From: Rich Trott <rtrott@gmail.com>
Date: Fri, 21 Jul 2017 22:04:26 -0700
Subject: [PATCH 5/8] squash: allow ECONNRESET in non-deterministic test

---
 test/parallel/test-https-set-timeout-server.js | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/test/parallel/test-https-set-timeout-server.js b/test/parallel/test-https-set-timeout-server.js
index f4abfb32b0df58..d1f849e4b547d1 100644
--- a/test/parallel/test-https-set-timeout-server.js
+++ b/test/parallel/test-https-set-timeout-server.js
@@ -184,6 +184,10 @@ test(function idleTimeout(cb) {
       rejectUnauthorized: false
     };
     const c = tls.connect(options, () => {
+      c.on('error', (e) => {
+        if (e.message !== 'read ECONNRESET')
+          throw e;
+      });
       c.write('GET /1 HTTP/1.1\r\nHost: localhost\r\n\r\n');
       // Keep-Alive
     });

From b02787d48fac79aa512282b63510bbfc62c1508f Mon Sep 17 00:00:00 2001
From: Rich Trott <rtrott@gmail.com>
Date: Fri, 21 Jul 2017 22:23:38 -0700
Subject: [PATCH 6/8] squash: whoops, missed a spot

---
 test/parallel/test-http-set-timeout-server.js | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/test/parallel/test-http-set-timeout-server.js b/test/parallel/test-http-set-timeout-server.js
index 6408e850efb6a5..2e0d3a00c069f7 100644
--- a/test/parallel/test-http-set-timeout-server.js
+++ b/test/parallel/test-http-set-timeout-server.js
@@ -162,6 +162,10 @@ test(function idleTimeout(cb) {
       allowHalfOpen: true,
     };
     const c = net.connect(options, () => {
+      c.on('error', (e) => {
+        if (e.message !== 'read ECONNRESET')
+          throw e;
+      });
       c.write('GET /1 HTTP/1.1\r\nHost: localhost\r\n\r\n');
       // Keep-Alive
     });

From e3dc68fffc6a19cf781bd11b9686f47b355f2722 Mon Sep 17 00:00:00 2001
From: Rich Trott <rtrott@gmail.com>
Date: Fri, 21 Jul 2017 22:42:52 -0700
Subject: [PATCH 7/8] squash: last of the flakiness, I think

---
 test/parallel/test-http-set-timeout-server.js  | 5 ++++-
 test/parallel/test-https-set-timeout-server.js | 5 ++++-
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/test/parallel/test-http-set-timeout-server.js b/test/parallel/test-http-set-timeout-server.js
index 2e0d3a00c069f7..c3a82cdf3573f6 100644
--- a/test/parallel/test-http-set-timeout-server.js
+++ b/test/parallel/test-http-set-timeout-server.js
@@ -122,11 +122,14 @@ test(function serverResponseTimeoutWithPipeline(cb) {
   const server = http.createServer((req, res) => {
     if (req.url === '/2')
       secReceived = true;
+    if (req.url === '/1') {
+      res.end();
+      return;
+    }
     const s = res.setTimeout(50, () => {
       caughtTimeout += req.url;
     });
     assert.ok(s instanceof http.OutgoingMessage);
-    if (req.url === '/1') res.end();
   });
   server.on('timeout', common.mustCall((socket) => {
     if (secReceived) {
diff --git a/test/parallel/test-https-set-timeout-server.js b/test/parallel/test-https-set-timeout-server.js
index d1f849e4b547d1..a73b285939089c 100644
--- a/test/parallel/test-https-set-timeout-server.js
+++ b/test/parallel/test-https-set-timeout-server.js
@@ -142,11 +142,14 @@ test(function serverResponseTimeoutWithPipeline(cb) {
   const server = https.createServer(serverOptions, (req, res) => {
     if (req.url === '/2')
       secReceived = true;
+    if (req.url === '/1') {
+      res.end();
+      return;
+    }
     const s = res.setTimeout(50, () => {
       caughtTimeout += req.url;
     });
     assert.ok(s instanceof http.OutgoingMessage);
-    if (req.url === '/1') res.end();
   });
   server.on('timeout', common.mustCall((socket) => {
     if (secReceived) {

From 340c06ae740ee59b676ddb545dfaaf04b0dd683c Mon Sep 17 00:00:00 2001
From: Rich Trott <rtrott@gmail.com>
Date: Mon, 24 Jul 2017 12:54:30 -0700
Subject: [PATCH 8/8] squash: comments per refack nits

---
 test/parallel/test-http-set-timeout-server.js  | 2 ++
 test/parallel/test-https-set-timeout-server.js | 3 +++
 2 files changed, 5 insertions(+)

diff --git a/test/parallel/test-http-set-timeout-server.js b/test/parallel/test-http-set-timeout-server.js
index c3a82cdf3573f6..7984fdfc0ffbf0 100644
--- a/test/parallel/test-http-set-timeout-server.js
+++ b/test/parallel/test-http-set-timeout-server.js
@@ -152,6 +152,7 @@ test(function serverResponseTimeoutWithPipeline(cb) {
 });
 
 test(function idleTimeout(cb) {
+  // Test that the an idle connection invokes the timeout callback.
   const server = http.createServer();
   const s = server.setTimeout(50, common.mustCall((socket) => {
     socket.destroy();
@@ -165,6 +166,7 @@ test(function idleTimeout(cb) {
       allowHalfOpen: true,
     };
     const c = net.connect(options, () => {
+      // ECONNRESET could happen on a heavily-loaded server.
       c.on('error', (e) => {
         if (e.message !== 'read ECONNRESET')
           throw e;
diff --git a/test/parallel/test-https-set-timeout-server.js b/test/parallel/test-https-set-timeout-server.js
index a73b285939089c..b5cc8a6f5a2664 100644
--- a/test/parallel/test-https-set-timeout-server.js
+++ b/test/parallel/test-https-set-timeout-server.js
@@ -173,6 +173,7 @@ test(function serverResponseTimeoutWithPipeline(cb) {
 });
 
 test(function idleTimeout(cb) {
+  // Test that the an idle connection invokes the timeout callback.
   const server = https.createServer(serverOptions);
   const s = server.setTimeout(50, common.mustCall((socket) => {
     socket.destroy();
@@ -187,6 +188,7 @@ test(function idleTimeout(cb) {
       rejectUnauthorized: false
     };
     const c = tls.connect(options, () => {
+      // ECONNRESET could happen on a heavily-loaded server.
       c.on('error', (e) => {
         if (e.message !== 'read ECONNRESET')
           throw e;
@@ -198,6 +200,7 @@ test(function idleTimeout(cb) {
 });
 
 test(function fastTimeout(cb) {
+  // Test that the socket timeout fires but no timeout fires for the request.
   let connectionHandlerInvoked = false;
   let timeoutHandlerInvoked = false;
   let connectionSocket;