http2: fix graceful session close

lib/internal/http2/core.js

@@ -1068,6 +1068,7 @@ function setupHandle(socket, type, options) {
   if (typeof options.selectPadding === 'function')
     this[kSelectPadding] = options.selectPadding;
   handle.consume(socket._handle);
+  handle.ongracefulclosecomplete = this[kMaybeDestroy].bind(this, null);
 
   this[kHandle] = handle;
   if (this[kNativeFields]) {
@@ -1589,6 +1590,10 @@ class Http2Session extends EventEmitter {
     if (typeof callback === 'function')
       this.once('close', callback);
     this.goaway();
+    const handle = this[kHandle];
+    if (handle) {
+      handle.setGracefulClose();
+    }
     this[kMaybeDestroy]();
   }
 
@@ -1609,11 +1614,13 @@ class Http2Session extends EventEmitter {
   // * session is closed and there are no more pending or open streams
   [kMaybeDestroy](error) {
     if (error == null) {
+      const handle = this[kHandle];
+      const hasPendingData = !!handle && handle.hasPendingData();
       const state = this[kState];
       // Do not destroy if we're not closed and there are pending/open streams
       if (!this.closed ||
           state.streams.size > 0 ||
-          state.pendingStreams.size > 0) {
+          state.pendingStreams.size > 0 || hasPendingData) {
         return;
       }
     }
@@ -3300,7 +3307,7 @@ function socketOnClose() {
     state.streams.forEach((stream) => stream.close(NGHTTP2_CANCEL));
     state.pendingStreams.forEach((stream) => stream.close(NGHTTP2_CANCEL));
     session.close();
-    session[kMaybeDestroy](err);
+    closeSession(session, NGHTTP2_NO_ERROR, err);
   }
 }
 

src/env_properties.h

@@ -285,6 +285,7 @@
   V(onsignal_string, "onsignal")                                               \
   V(onunpipe_string, "onunpipe")                                               \
   V(onwrite_string, "onwrite")                                                 \
+  V(ongracefulclosecomplete_string, "ongracefulclosecomplete")                 \
   V(openssl_error_stack, "opensslErrorStack")                                  \
   V(options_string, "options")                                                 \
   V(order_string, "order")                                                     \

src/node_http2.cc

@@ -559,7 +559,8 @@ Http2Session::Http2Session(Http2State* http2_state,
     : AsyncWrap(http2_state->env(), wrap, AsyncWrap::PROVIDER_HTTP2SESSION),
       js_fields_(http2_state->env()->isolate()),
       session_type_(type),
-      http2_state_(http2_state) {
+      http2_state_(http2_state),
+      graceful_close_initiated_(false) {
   MakeWeak();
   statistics_.session_type = type;
   statistics_.start_time = uv_hrtime();
@@ -765,6 +766,24 @@ void Http2Stream::EmitStatistics() {
   });
 }
 
+void Http2Session::HasPendingData(const FunctionCallbackInfo<Value>& args) {
+  Http2Session* session;
+  ASSIGN_OR_RETURN_UNWRAP(&session, args.Holder());
+  args.GetReturnValue().Set(session->HasPendingData());
+}
+
+bool Http2Session::HasPendingData() const {
+  nghttp2_session* session = session_.get();
+  int want_write = nghttp2_session_want_write(session);
+  // It is expected that want_read will alway be 0 if graceful
+  // session close is initiated and goaway frame is sent.
+  int want_read = nghttp2_session_want_read(session);
+  if (want_write == 0 && want_read == 0) {
+    return false;
+  }
+  return true;
+}
+
 void Http2Session::EmitStatistics() {
   if (!HasHttp2Observer(env())) [[likely]] {
     return;
@@ -1743,6 +1762,7 @@ void Http2Session::HandleSettingsFrame(const nghttp2_frame* frame) {
 void Http2Session::OnStreamAfterWrite(WriteWrap* w, int status) {
   Debug(this, "write finished with status %d", status);
 
+  MaybeNotifyGracefulCloseComplete();
   CHECK(is_write_in_progress());
   set_write_in_progress(false);
 
@@ -1965,6 +1985,7 @@ uint8_t Http2Session::SendPendingData() {
   if (!res.async) {
     set_write_in_progress(false);
     ClearOutgoing(res.err);
+    MaybeNotifyGracefulCloseComplete();
   }
 
   MaybeStopReading();
@@ -3478,6 +3499,8 @@ void Initialize(Local<Object> target,
   SetProtoMethod(isolate, session, "receive", Http2Session::Receive);
   SetProtoMethod(isolate, session, "destroy", Http2Session::Destroy);
   SetProtoMethod(isolate, session, "goaway", Http2Session::Goaway);
+  SetProtoMethod(
+      isolate, session, "hasPendingData", Http2Session::HasPendingData);
   SetProtoMethod(isolate, session, "settings", Http2Session::Settings);
   SetProtoMethod(isolate, session, "request", Http2Session::Request);
   SetProtoMethod(
@@ -3498,6 +3521,8 @@ void Initialize(Local<Object> target,
       "remoteSettings",
       Http2Session::RefreshSettings<nghttp2_session_get_remote_settings,
                                     false>);
+  SetProtoMethod(
+      isolate, session, "setGracefulClose", Http2Session::SetGracefulClose);
   SetConstructorFunction(context, target, "Http2Session", session);
 
   Local<Object> constants = Object::New(isolate);
@@ -3552,6 +3577,38 @@ void Initialize(Local<Object> target,
   nghttp2_set_debug_vprintf_callback(NgHttp2Debug);
 #endif
 }
+
+void Http2Session::SetGracefulClose(const FunctionCallbackInfo<Value>& args) {
+  Http2Session* session;
+  ASSIGN_OR_RETURN_UNWRAP(&session, args.Holder());
+  CHECK_NOT_NULL(session);
+  // Set the graceful close flag
+  session->SetGracefulCloseInitiated(true);
+
+  Debug(session, "Setting graceful close initiated flag");
+}
+
+void Http2Session::MaybeNotifyGracefulCloseComplete() {
+  nghttp2_session* session = session_.get();
+
+  if (!IsGracefulCloseInitiated()) {
+    return;
+  }
+
+  int want_write = nghttp2_session_want_write(session);
+  int want_read = nghttp2_session_want_read(session);
+  bool should_notify = (want_write == 0 && want_read == 0);
+
+  if (should_notify) {
+    Debug(this, "Notifying JS after write in graceful close mode");
+
+    // Make the callback to JavaScript
+    HandleScope scope(env()->isolate());
+    MakeCallback(env()->ongracefulclosecomplete_string(), 0, nullptr);
+  }
+
+  return;
+}
 }  // namespace http2
 }  // namespace node
 

src/node_http2.h

@@ -712,6 +712,7 @@ class Http2Session : public AsyncWrap,
   static void Consume(const v8::FunctionCallbackInfo<v8::Value>& args);
   static void Receive(const v8::FunctionCallbackInfo<v8::Value>& args);
   static void Destroy(const v8::FunctionCallbackInfo<v8::Value>& args);
+  static void HasPendingData(const v8::FunctionCallbackInfo<v8::Value>& args);
   static void Settings(const v8::FunctionCallbackInfo<v8::Value>& args);
   static void Request(const v8::FunctionCallbackInfo<v8::Value>& args);
   static void SetNextStreamID(const v8::FunctionCallbackInfo<v8::Value>& args);
@@ -723,6 +724,7 @@ class Http2Session : public AsyncWrap,
   static void Ping(const v8::FunctionCallbackInfo<v8::Value>& args);
   static void AltSvc(const v8::FunctionCallbackInfo<v8::Value>& args);
   static void Origin(const v8::FunctionCallbackInfo<v8::Value>& args);
+  static void SetGracefulClose(const v8::FunctionCallbackInfo<v8::Value>& args);
 
   template <get_setting fn, bool local>
   static void RefreshSettings(const v8::FunctionCallbackInfo<v8::Value>& args);
@@ -735,6 +737,7 @@ class Http2Session : public AsyncWrap,
 
   BaseObjectPtr<Http2Ping> PopPing();
   bool AddPing(const uint8_t* data, v8::Local<v8::Function> callback);
+  bool HasPendingData() const;
 
   BaseObjectPtr<Http2Settings> PopSettings();
   bool AddSettings(v8::Local<v8::Function> callback);
@@ -785,6 +788,13 @@ class Http2Session : public AsyncWrap,
 
   Statistics statistics_ = {};
 
+  bool IsGracefulCloseInitiated() const {
+    return graceful_close_initiated_;
+  }
+  void SetGracefulCloseInitiated(bool value) {
+    graceful_close_initiated_ = value;
+  }
+
  private:
   void EmitStatistics();
 
@@ -951,8 +961,13 @@ class Http2Session : public AsyncWrap,
   void CopyDataIntoOutgoing(const uint8_t* src, size_t src_length);
   void ClearOutgoing(int status);
 
+  void MaybeNotifyGracefulCloseComplete();
+
   friend class Http2Scope;
   friend class Http2StreamListener;
+
+  // Flag to indicate that JavaScript has initiated a graceful closure
+  bool graceful_close_initiated_ = false;
 };
 
 struct Http2SessionPerformanceEntryTraits {

test/parallel/test-http2-client-rststream-before-connect.js

@@ -5,16 +5,23 @@ if (!common.hasCrypto)
   common.skip('missing crypto');
 const assert = require('assert');
 const h2 = require('http2');
+let client;
 
 const server = h2.createServer();
 server.on('stream', (stream) => {
-  stream.on('close', common.mustCall());
-  stream.respond();
-  stream.end('ok');
+  stream.on('close', common.mustCall(() => {
+    client.close();
+    server.close();
+  }));
+  stream.on('error', common.expectsError({
+    code: 'ERR_HTTP2_STREAM_ERROR',
+    name: 'Error',
+    message: 'Stream closed with error code NGHTTP2_PROTOCOL_ERROR'
+  }));
 });
 
 server.listen(0, common.mustCall(() => {
-  const client = h2.connect(`http://localhost:${server.address().port}`);
+  client = h2.connect(`http://localhost:${server.address().port}`);
   const req = client.request();
   const closeCode = 1;
 
@@ -52,8 +59,6 @@ server.listen(0, common.mustCall(() => {
   req.on('close', common.mustCall(() => {
     assert.strictEqual(req.destroyed, true);
     assert.strictEqual(req.rstCode, closeCode);
-    server.close();
-    client.close();
   }));
 
   req.on('error', common.expectsError({

test/parallel/test-http2-session-graceful-close.js

@@ -0,0 +1,48 @@
+'use strict';
+
+const common = require('../common');
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+const assert = require('assert');
+const h2 = require('http2');
+
+const server = h2.createServer();
+let session;
+
+server.on('session', common.mustCall(function(s) {
+  session = s;
+  session.on('close', common.mustCall(function() {
+    server.close();
+  }));
+}));
+
+server.listen(0, common.mustCall(function() {
+  const port = server.address().port;
+
+  const url = `http://localhost:${port}`;
+  const client = h2.connect(url, common.mustCall(function() {
+    const headers = {
+      ':path': '/',
+      ':method': 'GET',
+      ':scheme': 'http',
+      ':authority': `localhost:${port}`
+    };
+    const request = client.request(headers);
+    request.on('response', common.mustCall(function(headers) {
+      assert.strictEqual(headers[':status'], 200);
+    }, 1));
+    request.on('end', common.mustCall(function() {
+      client.close();
+    }));
+    request.end();
+    request.resume();
+  }));
+  client.on('goaway', common.mustCallAtLeast(1));
+}));
+
+server.once('request', common.mustCall(function(request, response) {
+  response.on('finish', common.mustCall(function() {
+    session.close();
+  }));
+  response.end();
+}));