From 6cfca8aa2ed33ff96c324dbd3bab78ac2aa54cbe Mon Sep 17 00:00:00 2001
From: Anatoli Papirovski <apapirovski@mac.com>
Date: Thu, 17 May 2018 23:03:15 +0400
Subject: [PATCH 1/5] http2: submit Goaway frames & handle ECONNRESET

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.
---
 lib/internal/http2/core.js                    | 91 +++++++++++--------
 src/node_http2.cc                             | 33 ++++---
 src/node_http2.h                              |  8 +-
 test/parallel/parallel.status                 |  6 --
 test/parallel/test-http2-client-destroy.js    |  8 +-
 .../test-http2-server-close-callback.js       |  6 +-
 .../test-http2-server-sessionerror.js         | 13 ++-
 ...st-http2-server-shutdown-options-errors.js |  6 --
 .../test-http2-server-socket-destroy.js       | 10 +-
 ...est-http2-server-stream-session-destroy.js |  8 --
 test/parallel/test-http2-server-timeout.js    |  6 --
 test/parallel/test-http2-too-many-settings.js |  7 +-
 test/parallel/test-http2-trailers.js          |  5 +-
 .../test-http2-max-session-memory.js          |  4 -
 14 files changed, 104 insertions(+), 107 deletions(-)

diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js
index fbccbf0d849977..6f89801e586ca9 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() {
@@ -1521,6 +1527,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 +1541,6 @@ class Http2Stream extends Duplex {
       trailersReady: false
     };
 
-    this.on('resume', streamOnResume);
     this.on('pause', streamOnPause);
   }
 
@@ -1725,6 +1734,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 {
@@ -1866,15 +1879,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 +1900,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 +2490,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 94f774e2d3506d..949530d685dc79 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();
     }
   }
 
diff --git a/src/node_http2.h b/src/node_http2.h
index f473e30286bc79..043449624fb120 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 2bd5db83889740..899e6adfac060d 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 6238363511a791..43fc6819e21f7a 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-server-close-callback.js b/test/parallel/test-http2-server-close-callback.js
index 66887aa62bebe5..94f46218fb6ce0 100644
--- a/test/parallel/test-http2-server-close-callback.js
+++ b/test/parallel/test-http2-server-close-callback.js
@@ -9,11 +9,7 @@ const http2 = require('http2');
 const server = http2.createServer();
 
 server.listen(0, common.mustCall(() => {
-  const client = http2.connect(`http://localhost:${server.address().port}`);
-  client.on('error', (err) => {
-    if (err.code !== 'ECONNRESET')
-      throw err;
-  });
+  http2.connect(`http://localhost:${server.address().port}`);
 }));
 
 server.on('session', common.mustCall((s) => {
diff --git a/test/parallel/test-http2-server-sessionerror.js b/test/parallel/test-http2-server-sessionerror.js
index 525eb2e6efd11a..c50352fcc35c28 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 95849411a29182..9dde6660ea25ce 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 03afc1957b8af4..99595aeb63004d 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 ef4f42fe39bc3a..6a262e2736e4bc 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 4410cb51c169c5..8e5dc1e3287e3d 100755
--- a/test/parallel/test-http2-server-timeout.js
+++ b/test/parallel/test-http2-server-timeout.js
@@ -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 0302fe623da07c..acfd73ada68416 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 2b7f6158ca3951..682c3e209ad963 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',
@@ -28,7 +29,7 @@ function onStream(stream, headers, flags) {
     common.expectsError(
       () => stream.sendTrailers({}),
       {
-        code: 'ERR_HTTP2_TRAILERS_ALREADY_SENT',
+        code: 'ERR_HTTP2_INVALID_STREAM',
         type: Error
       }
     );
@@ -41,8 +42,6 @@ function onStream(stream, headers, flags) {
       type: Error
     }
   );
-
-  stream.end(body);
 }
 
 server.listen(0);
diff --git a/test/sequential/test-http2-max-session-memory.js b/test/sequential/test-http2-max-session-memory.js
index 644a20a3c88a50..d6d3bf935db801 100644
--- a/test/sequential/test-http2-max-session-memory.js
+++ b/test/sequential/test-http2-max-session-memory.js
@@ -13,10 +13,6 @@ const largeBuffer = Buffer.alloc(1e6);
 const server = http2.createServer({ maxSessionMemory: 1 });
 
 server.on('stream', common.mustCall((stream) => {
-  stream.on('error', (err) => {
-    if (err.code !== 'ECONNRESET')
-      throw err;
-  });
   stream.respond();
   stream.end(largeBuffer);
 }));

From cccee38c55a4a2bc6ca97ad7dbdaa4e69eb44a86 Mon Sep 17 00:00:00 2001
From: Anatoli Papirovski <apapirovski@mac.com>
Date: Sun, 20 May 2018 14:17:11 +0400
Subject: [PATCH 2/5] fixup tests

---
 .../test-http2-server-close-callback.js         | 17 ++++++++++++-----
 test/parallel/test-http2-server-timeout.js      |  4 ++--
 .../sequential/test-http2-max-session-memory.js |  4 ++++
 3 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/test/parallel/test-http2-server-close-callback.js b/test/parallel/test-http2-server-close-callback.js
index 94f46218fb6ce0..f822d8a4a92d78 100644
--- a/test/parallel/test-http2-server-close-callback.js
+++ b/test/parallel/test-http2-server-close-callback.js
@@ -4,17 +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(() => {
-  http2.connect(`http://localhost:${server.address().port}`);
+  const client = http2.connect(`http://localhost:${server.address().port}`);
+  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-timeout.js b/test/parallel/test-http2-server-timeout.js
index 8e5dc1e3287e3d..88fc4eab2e08c0 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());
diff --git a/test/sequential/test-http2-max-session-memory.js b/test/sequential/test-http2-max-session-memory.js
index d6d3bf935db801..644a20a3c88a50 100644
--- a/test/sequential/test-http2-max-session-memory.js
+++ b/test/sequential/test-http2-max-session-memory.js
@@ -13,6 +13,10 @@ const largeBuffer = Buffer.alloc(1e6);
 const server = http2.createServer({ maxSessionMemory: 1 });
 
 server.on('stream', common.mustCall((stream) => {
+  stream.on('error', (err) => {
+    if (err.code !== 'ECONNRESET')
+      throw err;
+  });
   stream.respond();
   stream.end(largeBuffer);
 }));

From 3a4a7ce5f1651ff1b53dec206916af5b3e5d357d Mon Sep 17 00:00:00 2001
From: Anatoli Papirovski <apapirovski@mac.com>
Date: Mon, 21 May 2018 11:41:22 +0400
Subject: [PATCH 3/5] fixup: do not send empty trailer frames

---
 lib/internal/http2/core.js                    | 19 ++++++++++++-------
 src/node_http2.cc                             | 17 +++++++++++++----
 .../test-http2-no-wanttrailers-listener.js    |  5 +----
 test/parallel/test-http2-trailers.js          |  2 +-
 4 files changed, 27 insertions(+), 16 deletions(-)

diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js
index 6f89801e586ca9..39d456fb75905f 100644
--- a/lib/internal/http2/core.js
+++ b/lib/internal/http2/core.js
@@ -1466,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;
@@ -1786,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() {
diff --git a/src/node_http2.cc b/src/node_http2.cc
index 949530d685dc79..4745707f9ac345 100644
--- a/src/node_http2.cc
+++ b/src/node_http2.cc
@@ -1821,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);
 }
 
@@ -1829,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;
 }
@@ -2358,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
@@ -2374,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/test/parallel/test-http2-no-wanttrailers-listener.js b/test/parallel/test-http2-no-wanttrailers-listener.js
index 7ba25d0e491e15..87bc21df48aa2c 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-trailers.js b/test/parallel/test-http2-trailers.js
index 682c3e209ad963..bdc0931157ad58 100644
--- a/test/parallel/test-http2-trailers.js
+++ b/test/parallel/test-http2-trailers.js
@@ -29,7 +29,7 @@ function onStream(stream, headers, flags) {
     common.expectsError(
       () => stream.sendTrailers({}),
       {
-        code: 'ERR_HTTP2_INVALID_STREAM',
+        code: 'ERR_HTTP2_TRAILERS_ALREADY_SENT',
         type: Error
       }
     );

From 77f6aa2c2c75986e72e989501474790edf17d7f1 Mon Sep 17 00:00:00 2001
From: Anatoli Papirovski <apapirovski@mac.com>
Date: Mon, 21 May 2018 11:42:32 +0400
Subject: [PATCH 4/5] test: fix flaky http2-session-unref

It's possible for the connections to take too long and since the
server is already unrefed, the process will just exit. Instead
adjust the test so that server unref only happens after all
session have been successfuly established and unrefed. That still
tests the same condition but will not fail under load.
---
 test/parallel/test-http2-session-unref.js | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/test/parallel/test-http2-session-unref.js b/test/parallel/test-http2-session-unref.js
index f946c2d3377b87..0381971c0eace5 100644
--- a/test/parallel/test-http2-session-unref.js
+++ b/test/parallel/test-http2-session-unref.js
@@ -9,16 +9,20 @@ const common = require('../common');
 if (!common.hasCrypto)
   common.skip('missing crypto');
 const http2 = require('http2');
+const Countdown = require('../common/countdown');
 const makeDuplexPair = require('../common/duplexpair');
 
 const server = http2.createServer();
 const { clientSide, serverSide } = makeDuplexPair();
 
+const counter = new Countdown(3, () => server.unref());
+
 // 'session' event should be emitted 3 times:
 // - the vanilla client
 // - the destroyed client
 // - manual 'connection' event emission with generic Duplex stream
 server.on('session', common.mustCallAtLeast((session) => {
+  counter.dec();
   session.unref();
 }, 3));
 
@@ -54,4 +58,3 @@ server.listen(0, common.mustCall(() => {
   }
 }));
 server.emit('connection', serverSide);
-server.unref();

From 9dff0ca879d4592f1872006e9d23a39419259d10 Mon Sep 17 00:00:00 2001
From: Anatoli Papirovski <apapirovski@mac.com>
Date: Tue, 22 May 2018 10:46:34 +0400
Subject: [PATCH 5/5] fixup: add doc note re: trailers behaviour

---
 doc/api/http2.md | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/doc/api/http2.md b/doc/api/http2.md
index 560e183dca6da1..ea848704ef666a 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);