http2: fix socketOnTimeout and a segfault

Fixes: nodejs/http2#179 Was fixing issue #179 and encountered a segault that was happening somewhat randomly on session destruction. Both should be fixed Backport-PR-URL: #14813 Backport-Reviewed-By: Anna Henningsen <[email protected]> Backport-Reviewed-By: Timothy Gu <[email protected]> PR-URL: #14239 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
nodejs · Aug 14, 2017 · 61696f1 · 61696f1
1 parent 2620769
commit 61696f1
Show file tree

Hide file tree

Showing 6 changed files with 83 additions and 9 deletions.
diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js
@@ -670,7 +670,7 @@ function finishSessionDestroy(socket) {
     debug(`[${sessionName(this[kType])}] nghttp2session handle destroyed`);
   }
 
-  this.emit('close');
+  process.nextTick(emit.bind(this, 'close'));
   debug(`[${sessionName(this[kType])}] nghttp2session destroyed`);
 }
 
@@ -953,6 +953,9 @@ class Http2Session extends EventEmitter {
     state.destroyed = true;
     state.destroying = false;
 
+    if (this[kHandle] !== undefined)
+      this[kHandle].destroying();
+
     setImmediate(finishSessionDestroy.bind(this, socket));
   }
 
@@ -1985,6 +1988,7 @@ function socketDestroy(error) {
   const type = this[kSession][kType];
   debug(`[${sessionName(type)}] socket destroy called`);
   delete this[kServer];
+  this.removeListener('timeout', socketOnTimeout);
   // destroy the session first so that it will stop trying to
   // send data while we close the socket.
   this[kSession].destroy();
@@ -2046,14 +2050,18 @@ function socketOnError(error) {
   this.destroy(error);
 }
 
-// When the socket times out, attempt a graceful shutdown
-// of the session
+// When the socket times out on the server, attempt a graceful shutdown
+// of the session.
 function socketOnTimeout() {
   debug('socket timeout');
   const server = this[kServer];
-  // server can be null if the socket is a client
-  if (server === undefined || !server.emit('timeout', this)) {
-    this[kSession].shutdown(
+  const session = this[kSession];
+  // If server or session are undefined, then we're already in the process of
+  // shutting down, do nothing.
+  if (server === undefined || session === undefined)
+    return;
+  if (!server.emit('timeout', session, this)) {
+    session.shutdown(
       {
         graceful: true,
         errorCode: NGHTTP2_NO_ERROR
@@ -2105,6 +2113,7 @@ function connectionListener(socket) {
   socket.on('resume', socketOnResume);
   socket.on('pause', socketOnPause);
   socket.on('drain', socketOnDrain);
+  socket.on('close', socketOnClose);
 
   // Set up the Session
   const session = new ServerHttp2Session(options, socket, this);
@@ -2197,6 +2206,13 @@ function setupCompat(ev) {
   }
 }
 
+function socketOnClose(hadError) {
+  const session = this[kSession];
+  if (session !== undefined && !session.destroyed) {
+    session.destroy();
+  }
+}
+
 // If the session emits an error, forward it to the socket as a sessionError;
 // failing that, destroy the session, remove the listener and re-emit the error
 function clientSessionOnError(error) {
@@ -2244,6 +2260,7 @@ function connect(authority, options, listener) {
   socket.on('resume', socketOnResume);
   socket.on('pause', socketOnPause);
   socket.on('drain', socketOnDrain);
+  socket.on('close', socketOnClose);
 
   const session = new ClientHttp2Session(options, socket);
 

diff --git a/src/node_http2.cc b/src/node_http2.cc
@@ -401,13 +401,21 @@ void Http2Session::Consume(const FunctionCallbackInfo<Value>& args) {
 }
 
 void Http2Session::Destroy(const FunctionCallbackInfo<Value>& args) {
-  DEBUG_HTTP2("Http2Session: destroying session\n");
   Http2Session* session;
   ASSIGN_OR_RETURN_UNWRAP(&session, args.Holder());
+  DEBUG_HTTP2("Http2Session: destroying session %d\n", session->type());
   session->Unconsume();
   session->Free();
 }
 
+void Http2Session::Destroying(const FunctionCallbackInfo<Value>& args) {
+  Http2Session* session;
+  ASSIGN_OR_RETURN_UNWRAP(&session, args.Holder());
+  DEBUG_HTTP2("Http2Session: preparing to destroy session %d\n",
+              session->type());
+  session->MarkDestroying();
+}
+
 void Http2Session::SubmitPriority(const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
   Http2Session* session;
@@ -816,11 +824,11 @@ void Http2Session::AllocateSend(size_t recommended, uv_buf_t* buf) {
 }
 
 void Http2Session::Send(uv_buf_t* buf, size_t length) {
+  DEBUG_HTTP2("Http2Session: Attempting to send data\n");
   if (stream_ == nullptr || !stream_->IsAlive() || stream_->IsClosing()) {
     return;
   }
   HandleScope scope(env()->isolate());
-
   auto AfterWrite = [](WriteWrap* req_wrap, int status) {
     req_wrap->Dispose();
   };
@@ -1191,6 +1199,8 @@ void Initialize(Local<Object> target,
                       Http2Session::Consume);
   env->SetProtoMethod(session, "destroy",
                       Http2Session::Destroy);
+  env->SetProtoMethod(session, "destroying",
+                      Http2Session::Destroying);
   env->SetProtoMethod(session, "sendHeaders",
                       Http2Session::SendHeaders);
   env->SetProtoMethod(session, "submitShutdownNotice",

diff --git a/src/node_http2.h b/src/node_http2.h
@@ -429,6 +429,7 @@ class Http2Session : public AsyncWrap,
   static void New(const FunctionCallbackInfo<Value>& args);
   static void Consume(const FunctionCallbackInfo<Value>& args);
   static void Unconsume(const FunctionCallbackInfo<Value>& args);
+  static void Destroying(const FunctionCallbackInfo<Value>& args);
   static void Destroy(const FunctionCallbackInfo<Value>& args);
   static void SubmitSettings(const FunctionCallbackInfo<Value>& args);
   static void SubmitRstStream(const FunctionCallbackInfo<Value>& args);

diff --git a/src/node_http2_core-inl.h b/src/node_http2_core-inl.h
@@ -136,6 +136,12 @@ inline void Nghttp2Session::HandleGoawayFrame(const nghttp2_frame* frame) {
 
 // Prompts nghttp2 to flush the queue of pending data frames
 inline void Nghttp2Session::SendPendingData() {
+  DEBUG_HTTP2("Nghttp2Session %d: Sending pending data\n", session_type_);
+  // Do not attempt to send data on the socket if the destroying flag has
+  // been set. That means everything is shutting down and the socket
+  // will not be usable.
+  if (IsDestroying())
+    return;
   const uint8_t* data;
   ssize_t len = 0;
   size_t ncopy = 0;
@@ -167,6 +173,7 @@ inline int Nghttp2Session::Init(uv_loop_t* loop,
   DEBUG_HTTP2("Nghttp2Session %d: initializing session\n", type);
   loop_ = loop;
   session_type_ = type;
+  destroying_ = false;
   int ret = 0;
 
   nghttp2_session_callbacks* callbacks
@@ -211,6 +218,9 @@ inline int Nghttp2Session::Init(uv_loop_t* loop,
   return ret;
 }
 
+inline void Nghttp2Session::MarkDestroying() {
+  destroying_ = true;
+}
 
 inline int Nghttp2Session::Free() {
   assert(session_ != nullptr);
@@ -224,11 +234,11 @@ inline int Nghttp2Session::Free() {
     session->OnFreeSession();
   };
   uv_close(reinterpret_cast<uv_handle_t*>(&prep_), PrepClose);
-
   nghttp2_session_terminate_session(session_, NGHTTP2_NO_ERROR);
   nghttp2_session_del(session_);
   session_ = nullptr;
   loop_ = nullptr;
+  DEBUG_HTTP2("Nghttp2Session %d: session freed\n", session_type_);
   return 1;
 }
 

diff --git a/src/node_http2_core.h b/src/node_http2_core.h
@@ -100,6 +100,10 @@ class Nghttp2Session {
 
   // Frees this session instance
   inline int Free();
+  inline void MarkDestroying();
+  bool IsDestroying() {
+    return destroying_;
+  }
 
   // Returns the pointer to the identified stream, or nullptr if
   // the stream does not exist
@@ -128,6 +132,10 @@ class Nghttp2Session {
   // Returns the nghttp2 library session
   inline nghttp2_session* session() { return session_; }
 
+  nghttp2_session_type type() {
+    return session_type_;
+  }
+
  protected:
   // Adds a stream instance to this session
   inline void AddStream(Nghttp2Stream* stream);
@@ -240,6 +248,7 @@ class Nghttp2Session {
   uv_prepare_t prep_;
   nghttp2_session_type session_type_;
   std::unordered_map<int32_t, Nghttp2Stream*> streams_;
+  bool destroying_ = false;
 
   friend class Nghttp2Stream;
 };

diff --git a/test/parallel/test-http2-server-timeout.js b/test/parallel/test-http2-server-timeout.js
@@ -0,0 +1,27 @@
+// Flags: --expose-http2
+'use strict';
+
+const common = require('../common');
+const http2 = require('http2');
+
+const server = http2.createServer();
+server.setTimeout(common.platformTimeout(1));
+
+const onServerTimeout = common.mustCall((session) => {
+  session.destroy();
+  server.removeListener('timeout', onServerTimeout);
+});
+
+server.on('stream', common.mustNotCall());
+server.on('timeout', onServerTimeout);
+
+server.listen(0, common.mustCall(() => {
+  const url = `http://localhost:${server.address().port}`;
+  const client = http2.connect(url);
+  client.on('close', common.mustCall(() => {
+
+    const client2 = http2.connect(url);
+    client2.on('close', common.mustCall(() => server.close()));
+
+  }));
+}));