Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

http2: allow fully synchronous _final() #25609

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 errr code for synchronous failures.
addaleax marked this conversation as resolved.
Show resolved Hide resolved
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