http2: allow fully synchronous _final()

HTTP/2 streams do not use the fact that the native `StreamBase::Shutdown()` is asynchronous by default and always finish synchronously. Adding a status code for this scenario allows skipping an expensive `MakeCallback()` C++/JS boundary crossing. PR-URL: #25609 Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
nodejs · May 16, 2019 · decba1c · decba1c
1 parent fbf8e60
commit decba1c
Show file tree

Hide file tree

Showing 4 changed files with 15 additions and 6 deletions.
diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js
@@ -1833,7 +1833,9 @@ class Http2Stream extends Duplex {
       req.oncomplete = afterShutdown;
       req.callback = cb;
       req.handle = handle;
-      handle.shutdown(req);
+      const err = handle.shutdown(req);
+      if (err === 1)  // synchronous finish
+        return afterShutdown.call(req, 0);
     } else {
       cb();
     }

diff --git a/lib/net.js b/lib/net.js
@@ -357,7 +357,9 @@ Socket.prototype._final = function(cb) {
   req.callback = cb;
   var err = this._handle.shutdown(req);
 
-  if (err)
+  if (err === 1)  // synchronous finish
+    return afterShutdown.call(req, 0);
+  else if (err !== 0)
     return this.destroy(errnoException(err, 'shutdown'));
 };
 

diff --git a/src/node_http2.cc b/src/node_http2.cc
@@ -1956,8 +1956,7 @@ int Http2Stream::DoShutdown(ShutdownWrap* req_wrap) {
              NGHTTP2_ERR_NOMEM);
     Debug(this, "writable side shutdown");
   }
-  req_wrap->Done(0);
-  return 0;
+  return 1;
 }
 
 // Destroy the Http2Stream and render it unusable. Actual resources for the

diff --git a/src/stream_base.h b/src/stream_base.h
@@ -205,12 +205,16 @@ class StreamResource {
   // All of these methods may return an error code synchronously.
   // In that case, the finish callback should *not* be called.
 
-  // Perform a shutdown operation, and call req_wrap->Done() when finished.
+  // Perform a shutdown operation, and either call req_wrap->Done() when
+  // finished and return 0, return 1 for synchronous success, or
+  // a libuv error code for synchronous failures.
   virtual int DoShutdown(ShutdownWrap* req_wrap) = 0;
   // Try to write as much data as possible synchronously, and modify
   // `*bufs` and `*count` accordingly. This is a no-op by default.
+  // Return 0 for success and a libuv error code for failures.
   virtual int DoTryWrite(uv_buf_t** bufs, size_t* count);
-  // Perform a write of data, and call req_wrap->Done() when finished.
+  // Perform a write of data, and either call req_wrap->Done() when finished
+  // and return 0, or return a libuv error code for synchronous failures.
   virtual int DoWrite(WriteWrap* w,
                       uv_buf_t* bufs,
                       size_t count,
@@ -272,6 +276,8 @@ class StreamBase : public StreamResource {
 
   // Shut down the current stream. This request can use an existing
   // ShutdownWrap object (that was created in JS), or a new one will be created.
+  // Returns 1 in case of a synchronous completion, 0 in case of asynchronous
+  // completion, and a libuv error case in case of synchronous failure.
   int Shutdown(v8::Local<v8::Object> req_wrap_obj = v8::Local<v8::Object>());
 
   // Write data to the current stream. This request can use an existing