http2: wait for session to finish writing before destroy

PR-URL: #30854 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]>
nodejs · Jan 3, 2020 · 86f2e86 · 86f2e86
1 parent 69d6e97
commit 86f2e86
Show file tree

Hide file tree

Showing 2 changed files with 19 additions and 8 deletions.
diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js
@@ -986,6 +986,8 @@ function emitClose(self, error) {
 }
 
 function finishSessionDestroy(session, error) {
+  debugSessionObj(session, 'finishSessionDestroy');
+
   const socket = session[kSocket];
   if (!socket.destroyed)
     socket.destroy(error);
@@ -1354,16 +1356,12 @@ class Http2Session extends EventEmitter {
     const handle = this[kHandle];
 
     // Destroy the handle if it exists at this point
-    if (handle !== undefined)
+    if (handle !== undefined) {
+      handle.ondone = finishSessionDestroy.bind(null, this, error);
       handle.destroy(code, socket.destroyed);
-
-    // 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)
-      setImmediate(finishSessionDestroy, this, error);
-    else
+    } else {
       finishSessionDestroy(this, error);
+    }
   }
 
   // Closing the session will:

diff --git a/src/node_http2.cc b/src/node_http2.cc
@@ -683,6 +683,13 @@ void Http2Session::Close(uint32_t code, bool socket_closed) {
 
   flags_ |= SESSION_STATE_CLOSED;
 
+  // If we are writing we will get to make the callback in OnStreamAfterWrite.
+  if ((flags_ & SESSION_STATE_WRITE_IN_PROGRESS) == 0) {
+    Debug(this, "make done session callback");
+    HandleScope scope(env()->isolate());
+    MakeCallback(env()->ondone_string(), 0, nullptr);
+  }
+
   // 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.
@@ -1485,6 +1492,12 @@ void Http2Session::OnStreamAfterWrite(WriteWrap* w, int status) {
     stream_->ReadStart();
   }
 
+  if ((flags_ & SESSION_STATE_CLOSED) != 0) {
+    HandleScope scope(env()->isolate());
+    MakeCallback(env()->ondone_string(), 0, nullptr);
+    return;
+  }
+
   // If there is more incoming data queued up, consume it.
   if (stream_buf_offset_ > 0) {
     ConsumeHTTP2Data();