http2: improve session close/destroy procedures

lib/internal/http2/core.js

@@ -1108,19 +1108,25 @@ function finishSessionClose(session, error) {
   cleanupSession(session);
 
   if (socket && !socket.destroyed) {
+    socket.on('close', () => {
+      emitClose(session, error);
+    });
+    if (session.closed) {
+      // If we're gracefully closing the socket, call resume() so we can detect
+      // the peer closing in case binding.Http2Session is already gone.
+      socket.resume();
+    }
+
     // Always wait for writable side to finish.
     socket.end((err) => {
       debugSessionObj(session, 'finishSessionClose socket end', err, error);
-      // Due to the way the underlying stream is handled in Http2Session we
-      // won't get graceful Readable end from the other side even if it was sent
-      // as the stream is already considered closed and will neither be read
-      // from nor keep the event loop alive.
-      // Therefore destroy the socket immediately.
-      // Fixing this would require some heavy juggling of ReadStart/ReadStop
-      // mostly on Windows as on Unix it will be fine with just ReadStart
-      // after this 'ondone' callback.
-      socket.destroy(error);
-      emitClose(session, error);
+      // If session.destroy() was called, destroy the underlying socket. Delay
+      // it a bit to try to avoid ECONNRESET on Windows.
+      if (!session.closed) {
+        setImmediate(() => {
+          socket.destroy(error);
+        });
+      }
     });
   } else {
     process.nextTick(emitClose, session, error);

src/node_http2.cc

@@ -703,6 +703,11 @@ void Http2Session::Close(uint32_t code, bool socket_closed) {
     Debug(this, "make done session callback");
     HandleScope scope(env()->isolate());
     MakeCallback(env()->ondone_string(), 0, nullptr);
+    if (stream_ != nullptr) {
+      // Start reading again to detect the other end finishing.
+      set_reading_stopped(false);
+      stream_->ReadStart();
+    }
   }
 
   // If there are outstanding pings, those will need to be canceled, do
@@ -1592,6 +1597,11 @@ void Http2Session::OnStreamAfterWrite(WriteWrap* w, int status) {
   if (is_destroyed()) {
     HandleScope scope(env()->isolate());
     MakeCallback(env()->ondone_string(), 0, nullptr);
+    if (stream_ != nullptr) {
+      // Start reading again to detect the other end finishing.
+      set_reading_stopped(false);
+      stream_->ReadStart();
+    }
     return;
   }
 
@@ -1640,7 +1650,9 @@ void Http2Session::MaybeScheduleWrite() {
 }
 
 void Http2Session::MaybeStopReading() {
-  if (is_reading_stopped()) return;
+  // If the session is already closing we don't want to stop reading as we want
+  // to detect when the other peer is actually closed.
+  if (is_reading_stopped() || is_closing()) return;
   int want_read = nghttp2_session_want_read(session_.get());
   Debug(this, "wants read? %d", want_read);
   if (want_read == 0 || is_write_in_progress()) {

test/parallel/test-http2-server-close-callback.js

@@ -13,7 +13,7 @@ let session;
 
 const countdown = new Countdown(2, () => {
   server.close(common.mustSucceed());
-  session.destroy();
+  session.close();
 });
 
 server.listen(0, common.mustCall(() => {

test/parallel/test-http2-server-sessionerror.js

@@ -44,17 +44,21 @@ server.on('sessionError', common.mustCall((err, session) => {
 server.listen(0, common.mustCall(() => {
   const url = `http://localhost:${server.address().port}`;
   http2.connect(url)
-    .on('error', common.expectsError({
-      code: 'ERR_HTTP2_SESSION_ERROR',
-      message: 'Session closed with error code 2',
+    .on('error', common.mustCall((err) => {
+      if (err.code !== 'ECONNRESET') {
+        assert.strictEqual(err.code, 'ERR_HTTP2_SESSION_ERROR');
+        assert.strictEqual(err.message, 'Session closed with error code 2');
+      }
     }))
     .on('close', () => {
       server.removeAllListeners('error');
       http2.connect(url)
-        .on('error', common.expectsError({
-          code: 'ERR_HTTP2_SESSION_ERROR',
-          message: 'Session closed with error code 2',
-        }))
+      .on('error', common.mustCall((err) => {
+        if (err.code !== 'ECONNRESET') {
+          assert.strictEqual(err.code, 'ERR_HTTP2_SESSION_ERROR');
+          assert.strictEqual(err.message, 'Session closed with error code 2');
+        }
+      }))
         .on('close', () => server.close());
     });
 }));

test/parallel/test-http2-too-many-settings.js

@@ -29,9 +29,11 @@ function doTest(session) {
 
   server.listen(0, common.mustCall(() => {
     const client = h2.connect(`http://localhost:${server.address().port}`);
-    client.on('error', common.expectsError({
-      code: 'ERR_HTTP2_SESSION_ERROR',
-      message: 'Session closed with error code 2',
+    client.on('error', common.mustCall((err) => {
+      if (err.code !== 'ECONNRESET') {
+        assert.strictEqual(err.code, 'ERR_HTTP2_SESSION_ERROR');
+        assert.strictEqual(err.message, 'Session closed with error code 2');
+      }
     }));
     client.on('close', common.mustCall(() => server.close()));
   }));