http2: fix several serious bugs

Currently http2 does not properly submit GOAWAY frames when a session
is being destroyed. It also doesn't properly handle when the other
party severs the connection after sending a GOAWAY frame, even though
it should.

Edge, IE & Safari are currently unable to handle empty TRAILERS
frames despite them being correctly to spec. Instead send an empty
DATA frame with END_STREAM flag in those situations.

Fix and adjust several flaky and/or incorrect tests.

PR-URL: https://github.com/nodejs/node/pull/20772
Fixes: https://github.com/nodejs/node/issues/20705
Fixes: https://github.com/nodejs/node/issues/20750
Fixes: https://github.com/nodejs/node/issues/20850
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

15 files changed, 145 insertions, 122 deletions

diff --git a/doc/api/http2.md b/doc/api/http2.md
index ec75dd7f63..72233407d6 100644
--- a/doc/api/http2.md
+++ b/doc/api/http2.md
@@ -880,6 +880,10 @@ The `'trailers'` event is emitted when a block of headers associated with
 trailing header fields is received. The listener callback is passed the
 [HTTP/2 Headers Object][] and flags associated with the headers.
 
+Note that this event might not be emitted if `http2stream.end()` is called
+before trailers are received and the incoming data is not being read or
+listened for.
+
 ```js
 stream.on('trailers', (headers, flags) => {
   console.log(headers);
diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js
index fbccbf0d84..39d456fb75 100644
--- a/lib/internal/http2/core.js
+++ b/lib/internal/http2/core.js
@@ -341,20 +341,25 @@ function onStreamClose(code) {
 
   stream[kState].fd = -1;
   // Defer destroy we actually emit end.
-  if (stream._readableState.endEmitted || code !== NGHTTP2_NO_ERROR) {
+  if (!stream.readable || code !== NGHTTP2_NO_ERROR) {
     // If errored or ended, we can destroy immediately.
-    stream[kMaybeDestroy](null, code);
+    stream[kMaybeDestroy](code);
   } else {
     // Wait for end to destroy.
     stream.on('end', stream[kMaybeDestroy]);
     // Push a null so the stream can end whenever the client consumes
     // it completely.
     stream.push(null);
-    // If the client hasn't tried to consume the stream and there is no
-    // resume scheduled (which would indicate they would consume in the future),
-    // then just dump the incoming data so that the stream can be destroyed.
-    if (!stream[kState].didRead && !stream._readableState.resumeScheduled)
+
+    // If the user hasn't tried to consume the stream (and this is a server
+    // session) then just dump the incoming data so that the stream can
+    // be destroyed.
+    if (stream[kSession][kType] === NGHTTP2_SESSION_SERVER &&
+        !stream[kState].didRead &&
+        stream.readableFlowing === null)
       stream.resume();
+    else
+      stream.read(0);
   }
 }
 
@@ -379,7 +384,7 @@ function onStreamRead(nread, buf) {
         `${sessionName(stream[kSession][kType])}]: ending readable.`);
 
   // defer this until we actually emit end
-  if (stream._readableState.endEmitted) {
+  if (!stream.readable) {
     stream[kMaybeDestroy]();
   } else {
     stream.on('end', stream[kMaybeDestroy]);
@@ -469,8 +474,7 @@ function onGoawayData(code, lastStreamID, buf) {
     // goaway using NGHTTP2_NO_ERROR because there was no error
     // condition on this side of the session that caused the
     // shutdown.
-    session.destroy(new ERR_HTTP2_SESSION_ERROR(code),
-                    { errorCode: NGHTTP2_NO_ERROR });
+    session.destroy(new ERR_HTTP2_SESSION_ERROR(code), NGHTTP2_NO_ERROR);
   }
 }
 
@@ -813,6 +817,21 @@ function emitClose(self, error) {
   self.emit('close');
 }
 
+function finishSessionDestroy(session, error) {
+  const socket = session[kSocket];
+  if (!socket.destroyed)
+    socket.destroy(error);
+
+  session[kProxySocket] = undefined;
+  session[kSocket] = undefined;
+  session[kHandle] = undefined;
+  socket[kSession] = undefined;
+  socket[kServer] = undefined;
+
+  // Finally, emit the close and error events (if necessary) on next tick.
+  process.nextTick(emitClose, session, error);
+}
+
 // Upon creation, the Http2Session takes ownership of the socket. The session
 // may not be ready to use immediately if the socket is not yet fully connected.
 // In that case, the Http2Session will wait for the socket to connect. Once
@@ -869,6 +888,8 @@ class Http2Session extends EventEmitter {
 
     this[kState] = {
       flags: SESSION_FLAGS_PENDING,
+      goawayCode: null,
+      goawayLastStreamID: null,
       streams: new Map(),
       pendingStreams: new Set(),
       pendingAck: 0,
@@ -1171,25 +1192,13 @@ class Http2Session extends EventEmitter {
     if (handle !== undefined)
       handle.destroy(code, socket.destroyed);
 
-    // If there is no error, use setImmediate to destroy the socket on the
+    // If the socket is alive, use setImmediate to destroy the session on the
     // next iteration of the event loop in order to give data time to transmit.
     // Otherwise, destroy immediately.
-    if (!socket.destroyed) {
-      if (!error) {
-        setImmediate(socket.destroy.bind(socket));
-      } else {
-        socket.destroy(error);
-      }
-    }
-
-    this[kProxySocket] = undefined;
-    this[kSocket] = undefined;
-    this[kHandle] = undefined;
-    socket[kSession] = undefined;
-    socket[kServer] = undefined;
-
-    // Finally, emit the close and error events (if necessary) on next tick.
-    process.nextTick(emitClose, this, error);
+    if (!socket.destroyed)
+      setImmediate(finishSessionDestroy, this, error);
+    else
+      finishSessionDestroy(this, error);
   }
 
   // Closing the session will:
@@ -1441,11 +1450,8 @@ function afterDoStreamWrite(status, handle) {
 }
 
 function streamOnResume() {
-  if (!this.destroyed && !this.pending) {
-    if (!this[kState].didRead)
-      this[kState].didRead = true;
+  if (!this.destroyed)
     this[kHandle].readStart();
-  }
 }
 
 function streamOnPause() {
@@ -1460,6 +1466,16 @@ function afterShutdown() {
     stream[kMaybeDestroy]();
 }
 
+function finishSendTrailers(stream, headersList) {
+  stream[kState].flags &= ~STREAM_FLAGS_HAS_TRAILERS;
+
+  const ret = stream[kHandle].trailers(headersList);
+  if (ret < 0)
+    stream.destroy(new NghttpError(ret));
+  else
+    stream[kMaybeDestroy]();
+}
+
 function closeStream(stream, code, shouldSubmitRstStream = true) {
   const state = stream[kState];
   state.flags |= STREAM_FLAGS_CLOSED;
@@ -1521,6 +1537,10 @@ class Http2Stream extends Duplex {
     this[kSession] = session;
     session[kState].pendingStreams.add(this);
 
+    // Allow our logic for determining whether any reads have happened to
+    // work in all situations. This is similar to what we do in _http_incoming.
+    this._readableState.readingMore = true;
+
     this[kTimeout] = null;
 
     this[kState] = {
@@ -1531,7 +1551,6 @@ class Http2Stream extends Duplex {
       trailersReady: false
     };
 
-    this.on('resume', streamOnResume);
     this.on('pause', streamOnPause);
   }
 
@@ -1725,6 +1744,10 @@ class Http2Stream extends Duplex {
       this.push(null);
       return;
     }
+    if (!this[kState].didRead) {
+      this._readableState.readingMore = false;
+      this[kState].didRead = true;
+    }
     if (!this.pending) {
       streamOnResume.call(this);
     } else {
@@ -1773,13 +1796,8 @@ class Http2Stream extends Duplex {
       throw headersList;
     this[kSentTrailers] = headers;
 
-    this[kState].flags &= ~STREAM_FLAGS_HAS_TRAILERS;
-
-    const ret = this[kHandle].trailers(headersList);
-    if (ret < 0)
-      this.destroy(new NghttpError(ret));
-    else
-      this[kMaybeDestroy]();
+    // Send the trailers in setImmediate so we don't do it on nghttp2 stack.
+    setImmediate(finishSendTrailers, this, headersList);
   }
 
   get closed() {
@@ -1866,15 +1884,15 @@ class Http2Stream extends Duplex {
   }
   // The Http2Stream can be destroyed if it has closed and if the readable
   // side has received the final chunk.
-  [kMaybeDestroy](error, code = NGHTTP2_NO_ERROR) {
-    if (error || code !== NGHTTP2_NO_ERROR) {
-      this.destroy(error);
+  [kMaybeDestroy](code = NGHTTP2_NO_ERROR) {
+    if (code !== NGHTTP2_NO_ERROR) {
+      this.destroy();
       return;
     }
 
     // TODO(mcollina): remove usage of _*State properties
-    if (this._writableState.ended && this._writableState.pendingcb === 0) {
-      if (this._readableState.ended && this.closed) {
+    if (!this.writable) {
+      if (!this.readable && this.closed) {
         this.destroy();
         return;
       }
@@ -1887,7 +1905,7 @@ class Http2Stream extends Duplex {
           this[kSession][kType] === NGHTTP2_SESSION_SERVER &&
           !(state.flags & STREAM_FLAGS_HAS_TRAILERS) &&
           !state.didRead &&
-          !this._readableState.resumeScheduled) {
+          this.readableFlowing === null) {
         this.close();
       }
     }
@@ -2477,6 +2495,10 @@ Object.defineProperty(Http2Session.prototype, 'setTimeout', setTimeout);
 function socketOnError(error) {
   const session = this[kSession];
   if (session !== undefined) {
+    // We can ignore ECONNRESET after GOAWAY was received as there's nothing
+    // we can do and the other side is fully within its rights to do so.
+    if (error.code === 'ECONNRESET' && session[kState].goawayCode !== null)
+      return session.destroy();
     debug(`Http2Session ${sessionName(session[kType])}: socket error [` +
           `${error.message}]`);
     session.destroy(error);
diff --git a/src/node_http2.cc b/src/node_http2.cc
index 94f774e2d3..4745707f9a 100644
--- a/src/node_http2.cc
+++ b/src/node_http2.cc
@@ -577,9 +577,9 @@ void Http2Session::EmitStatistics() {
 void Http2Session::Close(uint32_t code, bool socket_closed) {
   DEBUG_HTTP2SESSION(this, "closing session");
 
-  if (flags_ & SESSION_STATE_CLOSED)
+  if (flags_ & SESSION_STATE_CLOSING)
     return;
-  flags_ |= SESSION_STATE_CLOSED;
+  flags_ |= SESSION_STATE_CLOSING;
 
   // Stop reading on the i/o stream
   if (stream_ != nullptr)
@@ -587,16 +587,18 @@ void Http2Session::Close(uint32_t code, bool socket_closed) {
 
   // If the socket is not closed, then attempt to send a closing GOAWAY
   // frame. There is no guarantee that this GOAWAY will be received by
-  // the peer but the HTTP/2 spec recommends sendinng it anyway. We'll
+  // the peer but the HTTP/2 spec recommends sending it anyway. We'll
   // make a best effort.
   if (!socket_closed) {
-    Http2Scope h2scope(this);
     DEBUG_HTTP2SESSION2(this, "terminating session with code %d", code);
     CHECK_EQ(nghttp2_session_terminate_session(session_, code), 0);
+    SendPendingData();
   } else if (stream_ != nullptr) {
     stream_->RemoveStreamListener(this);
   }
 
+  flags_ |= SESSION_STATE_CLOSED;
+
   // If there are outstanding pings, those will need to be canceled, do
   // so on the next iteration of the event loop to avoid calling out into
   // javascript since this may be called during garbage collection.
@@ -1355,25 +1357,32 @@ void Http2Session::MaybeScheduleWrite() {
   }
 }
 
+void Http2Session::MaybeStopReading() {
+  int want_read = nghttp2_session_want_read(session_);
+  DEBUG_HTTP2SESSION2(this, "wants read? %d", want_read);
+  if (want_read == 0)
+    stream_->ReadStop();
+}
+
 // Unset the sending state, finish up all current writes, and reset
 // storage for data and metadata that was associated with these writes.
 void Http2Session::ClearOutgoing(int status) {
   CHECK_NE(flags_ & SESSION_STATE_SENDING, 0);
 
+  flags_ &= ~SESSION_STATE_SENDING;
+
   if (outgoing_buffers_.size() > 0) {
     outgoing_storage_.clear();
 
-    for (const nghttp2_stream_write& wr : outgoing_buffers_) {
+    std::vector<nghttp2_stream_write> current_outgoing_buffers_;
+    current_outgoing_buffers_.swap(outgoing_buffers_);
+    for (const nghttp2_stream_write& wr : current_outgoing_buffers_) {
       WriteWrap* wrap = wr.req_wrap;
       if (wrap != nullptr)
         wrap->Done(status);
     }
-
-    outgoing_buffers_.clear();
   }
 
-  flags_ &= ~SESSION_STATE_SENDING;
-
   // Now that we've finished sending queued data, if there are any pending
   // RstStreams we should try sending again and then flush them one by one.
   if (pending_rst_streams_.size() > 0) {
@@ -1484,8 +1493,7 @@ uint8_t Http2Session::SendPendingData() {
     ClearOutgoing(res.err);
   }
 
-  DEBUG_HTTP2SESSION2(this, "wants data in return? %d",
-                      nghttp2_session_want_read(session_));
+  MaybeStopReading();
 
   return 0;
 }
@@ -1618,8 +1626,7 @@ void Http2Session::OnStreamRead(ssize_t nread, const uv_buf_t& buf) {
       };
       MakeCallback(env()->error_string(), arraysize(argv), argv);
     } else {
-      DEBUG_HTTP2SESSION2(this, "processed %d bytes. wants more? %d", ret,
-                          nghttp2_session_want_read(session_));
+      MaybeStopReading();
     }
   }
 
@@ -1814,6 +1821,7 @@ void Http2Stream::OnTrailers() {
   HandleScope scope(isolate);
   Local<Context> context = env()->context();
   Context::Scope context_scope(context);
+  flags_ &= ~NGHTTP2_STREAM_FLAG_TRAILERS;
   MakeCallback(env()->ontrailers_string(), 0, nullptr);
 }
 
@@ -1822,7 +1830,16 @@ int Http2Stream::SubmitTrailers(nghttp2_nv* nva, size_t len) {
   CHECK(!this->IsDestroyed());
   Http2Scope h2scope(this);
   DEBUG_HTTP2STREAM2(this, "sending %d trailers", len);
-  int ret = nghttp2_submit_trailer(**session_, id_, nva, len);
+  int ret;
+  // Sending an empty trailers frame poses problems in Safari, Edge & IE.
+  // Instead we can just send an empty data frame with NGHTTP2_FLAG_END_STREAM
+  // to indicate that the stream is ready to be closed.
+  if (len == 0) {
+    Http2Stream::Provider::Stream prov(this, 0);
+    ret = nghttp2_submit_data(**session_, NGHTTP2_FLAG_END_STREAM, id_, *prov);
+  } else {
+    ret = nghttp2_submit_trailer(**session_, id_, nva, len);
+  }
   CHECK_NE(ret, NGHTTP2_ERR_NOMEM);
   return ret;
 }
@@ -2351,8 +2368,7 @@ void Http2Stream::Info(const FunctionCallbackInfo<Value>& args) {
 
   Headers list(isolate, context, headers);
   args.GetReturnValue().Set(stream->SubmitInfo(*list, list.length()));
-  DEBUG_HTTP2STREAM2(stream, "%d informational headers sent",
-                     headers->Length());
+  DEBUG_HTTP2STREAM2(stream, "%d informational headers sent", list.length());
 }
 
 // Submits trailing headers on the Http2Stream
@@ -2367,7 +2383,7 @@ void Http2Stream::Trailers(const FunctionCallbackInfo<Value>& args) {
 
   Headers list(isolate, context, headers);
   args.GetReturnValue().Set(stream->SubmitTrailers(*list, list.length()));
-  DEBUG_HTTP2STREAM2(stream, "%d trailing headers sent", headers->Length());
+  DEBUG_HTTP2STREAM2(stream, "%d trailing headers sent", list.length());
 }
 
 // Grab the numeric id of the Http2Stream
diff --git a/src/node_http2.h b/src/node_http2.h
index f473e30286..043449624f 100644
--- a/src/node_http2.h
+++ b/src/node_http2.h
@@ -433,7 +433,8 @@ enum session_state_flags {
   SESSION_STATE_HAS_SCOPE = 0x1,
   SESSION_STATE_WRITE_SCHEDULED = 0x2,
   SESSION_STATE_CLOSED = 0x4,
-  SESSION_STATE_SENDING = 0x8,
+  SESSION_STATE_CLOSING = 0x8,
+  SESSION_STATE_SENDING = 0x10,
 };
 
 // This allows for 4 default-sized frames with their frame headers
@@ -619,7 +620,7 @@ class Http2Stream : public AsyncWrap,
 
   inline bool IsClosed() const {
     return flags_ & NGHTTP2_STREAM_FLAG_CLOSED;
-    }
+  }
 
   inline bool HasTrailers() const {
     return flags_ & NGHTTP2_STREAM_FLAG_TRAILERS;
@@ -827,6 +828,9 @@ class Http2Session : public AsyncWrap, public StreamListener {
   // Schedule a write if nghttp2 indicates it wants to write to the socket.
   void MaybeScheduleWrite();
 
+  // Stop reading if nghttp2 doesn't want to anymore.
+  void MaybeStopReading();
+
   // Returns pointer to the stream, or nullptr if stream does not exist
   inline Http2Stream* FindStream(int32_t id);
 
diff --git a/test/parallel/parallel.status b/test/parallel/parallel.status
index 2bd5db8388..899e6adfac 100644
--- a/test/parallel/parallel.status
+++ b/test/parallel/parallel.status
@@ -8,12 +8,6 @@ prefix parallel
 # Postmortem debugging data is prone to accidental removal during V8 updates.
 test-postmortem-metadata: PASS,FLAKY
 
-# http2 has a few bugs that make these tests flaky and that are currently worked
-# on.
-test-http2-client-upload-reject: PASS,FLAKY
-test-http2-pipe: PASS,FLAKY
-test-http2-client-upload: PASS,FLAKY
-
 [$system==win32]
 
 [$system==linux]
diff --git a/test/parallel/test-http2-client-destroy.js b/test/parallel/test-http2-client-destroy.js
index 6238363511..43fc6819e2 100644
--- a/test/parallel/test-http2-client-destroy.js
+++ b/test/parallel/test-http2-client-destroy.js
@@ -109,9 +109,6 @@ const Countdown = require('../common/countdown');
 
   server.listen(0, common.mustCall(() => {
     const client = h2.connect(`http://localhost:${server.address().port}`);
-    // On some platforms (e.g. windows), an ECONNRESET may occur at this
-    // point -- or it may not. Do not make this a mustCall
-    client.on('error', () => {});
 
     client.on('close', () => {
       server.close();
@@ -119,10 +116,7 @@ const Countdown = require('../common/countdown');
       client.destroy();
     });
 
-    const req = client.request();
-    // On some platforms (e.g. windows), an ECONNRESET may occur at this
-    // point -- or it may not. Do not make this a mustCall
-    req.on('error', () => {});
+    client.request();
   }));
 }
 
diff --git a/test/parallel/test-http2-no-wanttrailers-listener.js b/test/parallel/test-http2-no-wanttrailers-listener.js
index 7ba25d0e49..87bc21df48 100644
--- a/test/parallel/test-http2-no-wanttrailers-listener.js
+++ b/test/parallel/test-http2-no-wanttrailers-listener.js
@@ -3,7 +3,6 @@
 const common = require('../common');
 if (!common.hasCrypto)
   common.skip('missing crypto');
-const assert = require('assert');
 const h2 = require('http2');
 
 const server = h2.createServer();
@@ -25,9 +24,7 @@ server.on('listening', common.mustCall(function() {
   const client = h2.connect(`http://localhost:${this.address().port}`);
   const req = client.request();
   req.resume();
-  req.on('trailers', common.mustCall((headers) => {
-    assert.strictEqual(Object.keys(headers).length, 0);
-  }));
+  req.on('trailers', common.mustNotCall());
   req.on('close', common.mustCall(() => {
     server.close();
     client.close();
diff --git a/test/parallel/test-http2-server-close-callback.js b/test/parallel/test-http2-server-close-callback.js
index 66887aa62b..f822d8a4a9 100644
--- a/test/parallel/test-http2-server-close-callback.js
+++ b/test/parallel/test-http2-server-close-callback.js
@@ -4,21 +4,24 @@ const common = require('../common');
 if (!common.hasCrypto)
   common.skip('missing crypto');
 
+const Countdown = require('../common/countdown');
 const http2 = require('http2');
 
 const server = http2.createServer();
 
+let session;
+
+const countdown = new Countdown(2, () => {
+  server.close(common.mustCall());
+  session.destroy();
+});
+
 server.listen(0, common.mustCall(() => {
   const client = http2.connect(`http://localhost:${server.address().port}`);
-  client.on('error', (err) => {
-    if (err.code !== 'ECONNRESET')
-      throw err;
-  });
+  client.on('connect', common.mustCall(() => countdown.dec()));
 }));
 
 server.on('session', common.mustCall((s) => {
-  setImmediate(() => {
-    server.close(common.mustCall());
-    s.destroy();
-  });
+  session = s;
+  countdown.dec();
 }));
diff --git a/test/parallel/test-http2-server-sessionerror.js b/test/parallel/test-http2-server-sessionerror.js
index 525eb2e6ef..c50352fcc3 100644
--- a/test/parallel/test-http2-server-sessionerror.js
+++ b/test/parallel/test-http2-server-sessionerror.js
@@ -35,14 +35,17 @@ server.on('session', common.mustCall((session) => {
 server.listen(0, common.mustCall(() => {
   const url = `http://localhost:${server.address().port}`;
   http2.connect(url)
-    // An ECONNRESET error may occur depending on the platform (due largely
-    // to differences in the timing of socket closing). Do not wrap this in
-    // a common must call.
-    .on('error', () => {})
+    .on('error', common.expectsError({
+      code: 'ERR_HTTP2_SESSION_ERROR',
+      message: 'Session closed with error code 2',
+    }))
     .on('close', () => {
       server.removeAllListeners('error');
       http2.connect(url)
-        .on('error', () => {})
+        .on('error', common.expectsError({
+          code: 'ERR_HTTP2_SESSION_ERROR',
+          message: 'Session closed with error code 2',
+        }))
         .on('close', () => server.close());
     });
 }));
diff --git a/test/parallel/test-http2-server-shutdown-options-errors.js b/test/parallel/test-http2-server-shutdown-options-errors.js
index 95849411a2..9dde6660ea 100644
--- a/test/parallel/test-http2-server-shutdown-options-errors.js
+++ b/test/parallel/test-http2-server-shutdown-options-errors.js
@@ -55,13 +55,7 @@ server.listen(
   0,
   common.mustCall(() => {
     const client = http2.connect(`http://localhost:${server.address().port}`);
-    // On certain operating systems, an ECONNRESET may occur. We do not need
-    // to test for it here. Do not make this a mustCall
-    client.on('error', () => {});
     const req = client.request();
-    // On certain operating systems, an ECONNRESET may occur. We do not need
-    // to test for it here. Do not make this a mustCall
-    req.on('error', () => {});
     req.resume();
     req.on('close', common.mustCall(() => {
       client.close();
diff --git a/test/parallel/test-http2-server-socket-destroy.js b/test/parallel/test-http2-server-socket-destroy.js
index 03afc1957b..99595aeb63 100644
--- a/test/parallel/test-http2-server-socket-destroy.js
+++ b/test/parallel/test-http2-server-socket-destroy.js
@@ -41,14 +41,20 @@ server.on('listening', common.mustCall(() => {
   // The client may have an ECONNRESET error here depending on the operating
   // system, due mainly to differences in the timing of socket closing. Do
   // not wrap this in a common mustCall.
-  client.on('error', () => {});
+  client.on('error', (err) => {
+    if (err.code !== 'ECONNRESET')
+      throw err;
+  });
   client.on('close', common.mustCall());
 
   const req = client.request({ ':method': 'POST' });
   // The client may have an ECONNRESET error here depending on the operating
   // system, due mainly to differences in the timing of socket closing. Do
   // not wrap this in a common mustCall.
-  req.on('error', () => {});
+  req.on('error', (err) => {
+    if (err.code !== 'ECONNRESET')
+      throw err;
+  });
 
   req.on('aborted', common.mustCall());
   req.resume();
diff --git a/test/parallel/test-http2-server-stream-session-destroy.js b/test/parallel/test-http2-server-stream-session-destroy.js
index ef4f42fe39..6a262e2736 100644
--- a/test/parallel/test-http2-server-stream-session-destroy.js
+++ b/test/parallel/test-http2-server-stream-session-destroy.js
@@ -44,16 +44,8 @@ server.on('stream', common.mustCall((stream) => {
 
 server.listen(0, common.mustCall(() => {
   const client = h2.connect(`http://localhost:${server.address().port}`);
-  client.on('error', (err) => {
-    if (err.code !== 'ECONNRESET')
-      throw err;
-  });
   const req = client.request();
   req.resume();
   req.on('end', common.mustCall());
   req.on('close', common.mustCall(() => server.close(common.mustCall())));
-  req.on('error', (err) => {
-    if (err.code !== 'ECONNRESET')
-      throw err;
-  });
 }));
diff --git a/test/parallel/test-http2-server-timeout.js b/test/parallel/test-http2-server-timeout.js
index 4410cb51c1..88fc4eab2e 100755
--- a/test/parallel/test-http2-server-timeout.js
+++ b/test/parallel/test-http2-server-timeout.js
@@ -6,10 +6,10 @@ if (!common.hasCrypto)
 const http2 = require('http2');
 
 const server = http2.createServer();
-server.setTimeout(common.platformTimeout(1));
+server.setTimeout(common.platformTimeout(50));
 
 const onServerTimeout = common.mustCall((session) => {
-  session.close(() => session.destroy());
+  session.close();
 });
 
 server.on('stream', common.mustNotCall());
@@ -18,14 +18,8 @@ server.once('timeout', onServerTimeout);
 server.listen(0, common.mustCall(() => {
   const url = `http://localhost:${server.address().port}`;
   const client = http2.connect(url);
-  // Because of the timeout, an ECONNRESET error may or may not happen here.
-  // Keep this as a non-op and do not use common.mustCall()
-  client.on('error', () => {});
   client.on('close', common.mustCall(() => {
     const client2 = http2.connect(url);
-    // Because of the timeout, an ECONNRESET error may or may not happen here.
-    // Keep this as a non-op and do not use common.mustCall()
-    client2.on('error', () => {});
     client2.on('close', common.mustCall(() => server.close()));
   }));
 }));
diff --git a/test/parallel/test-http2-too-many-settings.js b/test/parallel/test-http2-too-many-settings.js
index 0302fe623d..acfd73ada6 100644
--- a/test/parallel/test-http2-too-many-settings.js
+++ b/test/parallel/test-http2-too-many-settings.js
@@ -29,9 +29,10 @@ function doTest(session) {
 
   server.listen(0, common.mustCall(() => {
     const client = h2.connect(`http://localhost:${server.address().port}`);
-    // On some operating systems, an ECONNRESET error may be emitted.
-    // On others it won't be. Do not make this a mustCall
-    client.on('error', () => {});
+    client.on('error', common.expectsError({
+      code: 'ERR_HTTP2_SESSION_ERROR',
+      message: 'Session closed with error code 2',
+    }));
     client.on('close', common.mustCall(() => server.close()));
   }));
 }
diff --git a/test/parallel/test-http2-trailers.js b/test/parallel/test-http2-trailers.js
index 2b7f6158ca..bdc0931157 100644
--- a/test/parallel/test-http2-trailers.js
+++ b/test/parallel/test-http2-trailers.js
@@ -18,6 +18,7 @@ server.on('stream', common.mustCall(onStream));
 function onStream(stream, headers, flags) {
   stream.on('trailers', common.mustCall((headers) => {
     assert.strictEqual(headers[trailerKey], trailerValue);
+    stream.end(body);
   }));
   stream.respond({
     'content-type': 'text/html',
@@ -41,8 +42,6 @@ function onStream(stream, headers, flags) {
       type: Error
     }
   );
-
-  stream.end(body);
 }
 
 server.listen(0);