fix: keep shared server alive for a request built before another finishes

lib/test.js

@@ -21,7 +21,35 @@ try {
 
 // Tracks only servers started by SuperTest.  WeakMap keeps the bookkeeping
 // private and lets user-owned servers remain untouched.
-const serverStates = new WeakMap();
+const serverStates = new WeakMap(); // eslint-disable-line no-undef
+
+// Listen on the same address the client connects to. A wildcard bind can
+// share its port with another process's 127.0.0.1 listener on macOS, and the
+// request would then reach that process instead.
+function startOwnedServer(app) {
+  const state = {
+    pending: 0,
+    starting: true,
+    closing: false,
+    startError: null
+  };
+  serverStates.set(app, state);
+
+  function onError(err) {
+    state.starting = false;
+    state.startError = err;
+    app.removeListener('listening', onListening);
+  }
+  function onListening() {
+    state.starting = false;
+    app.removeListener('error', onError);
+  }
+
+  app.once('listening', onListening);
+  app.once('error', onError);
+  app.listen(0, '127.0.0.1');
+  return state;
+}
 
 class Test extends Request {
   /**
@@ -65,46 +93,22 @@ class Test extends Request {
     let addr = app.address();
     let state = serverStates.get(app);
 
-    if (!addr) {
-      if (!state) {
-        state = {
-          pending: 0,
-          starting: true,
-          closing: false,
-          startError: null
-        };
-        serverStates.set(app, state);
-
-        const onListening = function () {
-          state.starting = false;
-          app.removeListener('error', onError);
-        };
-        const onError = function (err) {
-          state.starting = false;
-          state.startError = err;
-          app.removeListener('listening', onListening);
-        };
-
-        app.once('listening', onListening);
-        app.once('error', onError);
-        // Bind to the same address the client connects to. A wildcard bind can
-        // share its port with another process's 127.0.0.1 listener on macOS,
-        // and the request would then reach that process instead.
-        app.listen(0, '127.0.0.1');
-      }
+    if (!addr && !state) {
+      state = startOwnedServer(app);
       addr = app.address();
     }
 
     // A previously started SuperTest server is shared by every request created
-    // from the same request factory and closes after the final request settles.
+    // from the same app and closes after the final in-flight request. The path
+    // is retained so end() can rebuild the URL when that server has to restart.
     if (state) {
       this._server = app;
+      this._serverPath = path;
     }
 
     // Listening with a host resolves asynchronously, so the port is unknown
     // until the server emits listening. end() fills it in before dispatch.
     if (!addr) {
-      this._serverPath = path;
       const protocol = app instanceof Server ? 'https' : 'http';
       return protocol + '://127.0.0.1' + path;
     }
@@ -120,7 +124,7 @@ class Test extends Request {
    * @param {String} path
    * @returns {String} URL address
    * @api private
-   */
+   */// eslint-disable-next-line class-methods-use-this
   formatServerUrl(app, addr, path) {
     // Map wildcard addresses of servers passed in already listening to the
     // IPv4 loopback so SuperAgent does not construct an IPv6 loopback URL;
@@ -198,6 +202,24 @@ class Test extends Request {
    */
   end(fn) {
     const server = this._server;
+
+    // pending is counted here, not when the Test is built. A request constructed
+    // earlier can therefore outlive the shared server: the last in-flight
+    // request sees pending === 0, closes it, and drops the state. Re-acquire
+    // before dispatch. An unsent Test still does not pin the port.
+    if (server) {
+      const current = serverStates.get(server);
+      if (current && current.closing) {
+        server.once('close', () => {
+          this.end(fn);
+        });
+        return this;
+      }
+      if (!current && !server.address()) {
+        startOwnedServer(server);
+      }
+    }
+
     const state = server && serverStates.get(server);
     let complete = false;
     let dispatch;
@@ -223,11 +245,15 @@ class Test extends Request {
         if (state.pending === 0 && !state.closing && server._handle) {
           state.closing = true;
           return server.close(function () {
-            serverStates.delete(server);
+            // A request sent while this close was in flight may already own
+            // a replacement state. Only drop the state that initiated close.
+            if (serverStates.get(server) === state) serverStates.delete(server);
             localAssert();
           });
         }
-        if (state.pending === 0 && !server._handle) serverStates.delete(server);
+        if (state.pending === 0 && !server._handle && serverStates.get(server) === state) {
+          serverStates.delete(server);
+        }
       }
 
       localAssert();

test/supertest.js

@@ -1,5 +1,6 @@
 'use strict';
 
+const http = require('http');
 const https = require('https');
 let http2;
 try {
@@ -1016,6 +1017,57 @@ describe('request(app)', function () {
   });
 });
 
+describe('shared server late dispatch', function () {
+  function createServer() {
+    return http.createServer(function (req, res) {
+      res.end(req.headers.cookie || '');
+    });
+  }
+
+  const clients = [
+    {
+      name: 'request(server)',
+      create: function (server) { return request(server); }
+    },
+    {
+      name: 'request.agent(server)',
+      create: function (server) { return request.agent(server); }
+    }
+  ];
+
+  clients.forEach(function (client) {
+    // Outer Test is built first. The awaited inner request is the only one
+    // that reaches end(), so it closes the shared server before the outer send.
+    it(client.name + ' should send a request built before another finishes', function () {
+      const server = createServer();
+      const api = client.create(server);
+      const account = api.get('/account');
+
+      return api.post('/login').expect(200).then(function () {
+        server.listening.should.equal(false);
+        return account.set('Cookie', 'session=1').expect(200, 'session=1');
+      });
+    });
+
+    // Same gap, but the outer Test is built only after listen() has resolved,
+    // so its URL already contains the port that the inner request then closes.
+    it(client.name + ' should retarget a request built while the server is listening', function () {
+      const server = createServer();
+      const api = client.create(server);
+      const held = api.get('/hold');
+
+      return new global.Promise(function (resolve, reject) {
+        server.once('listening', function () {
+          const account = api.get('/account');
+          held.expect(200).then(function () {
+            return account.set('Cookie', 'session=1').expect(200, 'session=1');
+          }).then(resolve, reject);
+        });
+      });
+    });
+  });
+});
+
 describe('request.agent(app)', function () {
   const app = express();
   const agent = request.agent(app)