From decba1c59bbe3d2d78c6a9e9a01161eb46fa742b Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 21 Jan 2019 14:54:01 +0100 Subject: [PATCH] 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: https://github.com/nodejs/node/pull/25609 Reviewed-By: Sam Roberts Reviewed-By: Matteo Collina --- lib/internal/http2/core.js | 4 +++- lib/net.js | 4 +++- src/node_http2.cc | 3 +-- src/stream_base.h | 10 ++++++++-- 4 files changed, 15 insertions(+), 6 deletions(-) diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index edc06e0ef870ae..a57e63c235398a 100644 --- 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 index b10208ea5a02cf..094685af00957a 100644 --- 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 index 71fdf398e29577..6bd282593e28f5 100644 --- 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 index d8e6df960f4f54..65abd4dcf4d84e 100644 --- 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 req_wrap_obj = v8::Local()); // Write data to the current stream. This request can use an existing