Skip to content

Commit

Permalink
http2: allow fully synchronous _final()
Browse files Browse the repository at this point in the history
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: nodejs#25609
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
  • Loading branch information
addaleax committed Jan 23, 2019
1 parent d3806f9 commit 2b65399
Show file tree
Hide file tree
Showing 4 changed files with 15 additions and 6 deletions.
4 changes: 3 additions & 1 deletion lib/internal/http2/core.js
Original file line number Diff line number Diff line change
Expand Up @@ -1821,7 +1821,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();
}
Expand Down
4 changes: 3 additions & 1 deletion lib/net.js
Original file line number Diff line number Diff line change
Expand Up @@ -362,7 +362,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'));
};

Expand Down
3 changes: 1 addition & 2 deletions src/node_http2.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1948,8 +1948,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
Expand Down
10 changes: 8 additions & 2 deletions src/stream_base.h
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -274,6 +278,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
Expand Down

0 comments on commit 2b65399

Please sign in to comment.