fix: improve HTTP/2 server shutdown to prevent race conditions

1. Fix server shutdown race condition - Stop listening for new connections before closing existing ones - Ensure server.close() properly completes in all scenarios 2. Improve HTTP/2 tests - Replace setTimeout with event-based flow control - Simplify test logic for better readability - Add clear state tracking for event ordering - Improve assertions to verify correct shutdown sequence This eliminates a race condition where new sessions could connect between the time existing sessions are closed and the server stops listening, potentially preventing the server from fully shutting down.
nodejs · pandeykushagra51 · Mar 21, 2025 · Mar 28, 2025 · Mar 29, 2025 · Apr 6, 2025
commit 7a852db48479d691305ce3be3641a50af4437634
diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js
@@ -3224,11 +3224,11 @@ class Http2SecureServer extends TLSServer {
   }
 
   close() {
+    ReflectApply(TLSServer.prototype.close, this, arguments);
     if (this[kOptions].allowHTTP1 === true) {
       httpServerPreClose(this);
     }
     closeAllSessions(this);
-    ReflectApply(TLSServer.prototype.close, this, arguments);
   }
 
   closeIdleConnections() {
@@ -3266,8 +3266,8 @@ class Http2Server extends NETServer {
   }
 
   close() {
-    closeAllSessions(this);
     ReflectApply(NETServer.prototype.close, this, arguments);
+    closeAllSessions(this);
   }
 
   async [SymbolAsyncDispose]() {

diff --git a/test/parallel/test-http2-graceful-close.js b/test/parallel/test-http2-graceful-close.js
@@ -0,0 +1,103 @@
+'use strict';
+
+const common = require('../common');
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+const assert = require('assert');
+const http2 = require('http2');
+
+// This test verifies that the server waits for active connections/sessions
+// to complete and all data to be sent before fully closing
+
+// Keep track of the test flow with these flags
+const states = {
+  serverListening: false,
+  responseStarted: false,
+  dataFullySent: false,
+  streamClosed: false,
+  serverClosed: false
+};
+
+// Create a server that will send a large response in chunks
+const server = http2.createServer();
+
+// Track server events
+server.on('stream', common.mustCall((stream, headers) => {
+  assert.strictEqual(states.serverListening, true);
+
+  // Setup the response
+  stream.respond({
+    'content-type': 'text/plain',
+    ':status': 200
+  });
+
+  // Initiate the server close before client data is sent, this will
+  // test if the server properly waits for the stream to finish
+  server.close(common.mustCall(() => {
+    // Stream should be closed before server close callback
+    // Should be called only after the stream has closed
+    assert.strictEqual(states.streamClosed, true);
+    states.serverClosed = true;
+  }));
+
+  // Mark response as started
+  states.responseStarted = true;
+
+  // Create a large response (1MB) to ensure it takes some time to send
+  const chunk = Buffer.alloc(64 * 1024, 'x');
+
+  // Send 16 chunks (1MB total) to simulate a large response
+  for (let i = 0; i < 16; i++) {
+    stream.write(chunk);
+  }
+
+
+  // Stream end should happen after data is written
+  stream.end();
+  states.dataFullySent = true;
+
+  // When stream closes, we can verify order of events
+  stream.on('close', common.mustCall(() => {
+    // Data should be fully sent before stream closes
+    assert.strictEqual(states.dataFullySent, true);
+    states.streamClosed = true;
+  }));
+}));
+
+// Start the server
+server.listen(0, common.mustCall(() => {
+  states.serverListening = true;
+
+  // Create client and request
+  const client = http2.connect(`http://localhost:${server.address().port}`);
+  const req = client.request({ ':path': '/' });
+
+  // Track received data
+  let receivedData = 0;
+  req.on('data', (chunk) => {
+    receivedData += chunk.length;
+  });
+
+  // When the request completes
+  req.on('end', common.mustCall(() => {
+    // All data should be sent before request ends
+    assert.strictEqual(states.dataFullySent, true);
+  }));
+
+  // When request closes
+  req.on('close', common.mustCall(() => {
+    // Check final state
+    assert.strictEqual(states.streamClosed, true);
+    // Should receive all data
+    assert.strictEqual(receivedData, 64 * 1024 * 16);
-    // Should receive all data
-    assert.strictEqual(receivedData, 64 * 1024 * 16);
+    // Should receive all data
+    assert.strictEqual(req.readableEnded, true);
+    assert.strictEqual(receivedData, 64 * 1024 * 16);
+    assert.strictEqual(req.writableFinished, true);
-    // Should receive all data
-    assert.strictEqual(receivedData, 64 * 1024 * 16);
+    // Should receive all data
+    assert.strictEqual(req.readableEnded, true);
+    assert.strictEqual(receivedData, 64 * 1024 * 16);
+    assert.strictEqual(req.writableFinished, true);
+
+    // Wait for the server close confirmation
+    process.on('exit', () => {
+      // Server should be fully closed
+      assert.strictEqual(states.serverClosed, true);
+    });
+  }));
+
+  // Start the request
+  req.end();
+}));
diff --git a/test/parallel/test-http2-request-after-server-close.js b/test/parallel/test-http2-request-after-server-close.js