From 078ee27f13864cc5d435913a0fb10fc046e8558a Mon Sep 17 00:00:00 2001 From: James M Snell Date: Fri, 29 Sep 2017 14:14:27 -0700 Subject: [PATCH] http2: refactor method arguments to avoid bools PR-URL: https://github.com/nodejs/node/pull/15693 Reviewed-By: Matteo Collina Reviewed-By: Anatoli Papirovski --- lib/internal/http2/core.js | 67 +++++++++++++++++++++----------------- src/node_http2.cc | 32 +++++++++--------- src/node_http2_core-inl.h | 34 +++++++++---------- src/node_http2_core.h | 18 +++++----- 4 files changed, 80 insertions(+), 71 deletions(-) diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js index 34753b53b2411e..0dd8d5e100c0f2 100644 --- a/lib/internal/http2/core.js +++ b/lib/internal/http2/core.js @@ -126,7 +126,10 @@ const { HTTP_STATUS_OK, HTTP_STATUS_NO_CONTENT, HTTP_STATUS_NOT_MODIFIED, - HTTP_STATUS_SWITCHING_PROTOCOLS + HTTP_STATUS_SWITCHING_PROTOCOLS, + + STREAM_OPTION_EMPTY_PAYLOAD, + STREAM_OPTION_GET_TRAILERS } = constants; // Top level to avoid creating a closure @@ -411,20 +414,22 @@ function requestOnConnect(headers, options) { return; } - let getTrailers = false; + let streamOptions = 0; + if (options.endStream) + streamOptions |= STREAM_OPTION_EMPTY_PAYLOAD; + if (typeof options.getTrailers === 'function') { - getTrailers = true; + streamOptions |= STREAM_OPTION_GET_TRAILERS; this[kState].getTrailers = options.getTrailers; } // ret will be either the reserved stream ID (if positive) // or an error code (if negative) const ret = handle.submitRequest(headersList, - !!options.endStream, + streamOptions, options.parent | 0, options.weight | 0, - !!options.exclusive, - getTrailers); + !!options.exclusive); // In an error condition, one of three possible response codes will be // possible: @@ -1567,7 +1572,7 @@ function processHeaders(headers) { } function processRespondWithFD(fd, headers, offset = 0, length = -1, - getTrailers = false) { + streamOptions = 0) { const session = this[kSession]; const state = this[kState]; state.headersSent = true; @@ -1577,7 +1582,7 @@ function processRespondWithFD(fd, headers, offset = 0, length = -1, const handle = session[kHandle]; const ret = - handle.submitFile(this[kID], fd, headers, offset, length, getTrailers); + handle.submitFile(this[kID], fd, headers, offset, length, streamOptions); let err; switch (ret) { case NGHTTP2_ERR_NOMEM: @@ -1592,7 +1597,7 @@ function processRespondWithFD(fd, headers, offset = 0, length = -1, } } -function doSendFD(session, options, fd, headers, getTrailers, err, stat) { +function doSendFD(session, options, fd, headers, streamOptions, err, stat) { if (this.destroyed || session.destroyed) { abort(this); return; @@ -1622,10 +1627,10 @@ function doSendFD(session, options, fd, headers, getTrailers, err, stat) { processRespondWithFD.call(this, fd, headersList, statOptions.offset, statOptions.length, - getTrailers); + streamOptions); } -function doSendFileFD(session, options, fd, headers, getTrailers, err, stat) { +function doSendFileFD(session, options, fd, headers, streamOptions, err, stat) { if (this.destroyed || session.destroyed) { abort(this); return; @@ -1680,10 +1685,10 @@ function doSendFileFD(session, options, fd, headers, getTrailers, err, stat) { processRespondWithFD.call(this, fd, headersList, options.offset, options.length, - getTrailers); + streamOptions); } -function afterOpen(session, options, headers, getTrailers, err, fd) { +function afterOpen(session, options, headers, streamOptions, err, fd) { const state = this[kState]; const onError = options.onError; if (this.destroyed || session.destroyed) { @@ -1701,7 +1706,8 @@ function afterOpen(session, options, headers, getTrailers, err, fd) { state.fd = fd; fs.fstat(fd, - doSendFileFD.bind(this, session, options, fd, headers, getTrailers)); + doSendFileFD.bind(this, session, options, fd, + headers, streamOptions)); } function streamOnError(err) { @@ -1785,9 +1791,9 @@ class ServerHttp2Stream extends Http2Stream { throw headersList; } - const ret = handle.submitPushPromise(this[kID], - headersList, - options.endStream); + const streamOptions = options.endStream ? STREAM_OPTION_EMPTY_PAYLOAD : 0; + + const ret = handle.submitPushPromise(this[kID], headersList, streamOptions); let err; switch (ret) { case NGHTTP2_ERR_NOMEM: @@ -1843,14 +1849,17 @@ class ServerHttp2Stream extends Http2Stream { options = Object.assign(Object.create(null), options); options.endStream = !!options.endStream; - let getTrailers = false; + let streamOptions = 0; + if (options.endStream) + streamOptions |= STREAM_OPTION_EMPTY_PAYLOAD; + if (options.getTrailers !== undefined) { if (typeof options.getTrailers !== 'function') { throw new errors.TypeError('ERR_INVALID_OPT_VALUE', 'getTrailers', options.getTrailers); } - getTrailers = true; + streamOptions |= STREAM_OPTION_GET_TRAILERS; state.getTrailers = options.getTrailers; } @@ -1880,10 +1889,7 @@ class ServerHttp2Stream extends Http2Stream { const handle = session[kHandle]; const ret = - handle.submitResponse(this[kID], - headersList, - options.endStream, - getTrailers); + handle.submitResponse(this[kID], headersList, streamOptions); let err; switch (ret) { case NGHTTP2_ERR_NOMEM: @@ -1936,14 +1942,14 @@ class ServerHttp2Stream extends Http2Stream { options.statCheck); } - let getTrailers = false; + let streamOptions = 0; if (options.getTrailers !== undefined) { if (typeof options.getTrailers !== 'function') { throw new errors.TypeError('ERR_INVALID_OPT_VALUE', 'getTrailers', options.getTrailers); } - getTrailers = true; + streamOptions |= STREAM_OPTION_GET_TRAILERS; state.getTrailers = options.getTrailers; } @@ -1962,7 +1968,8 @@ class ServerHttp2Stream extends Http2Stream { if (options.statCheck !== undefined) { fs.fstat(fd, - doSendFD.bind(this, session, options, fd, headers, getTrailers)); + doSendFD.bind(this, session, options, fd, + headers, streamOptions)); return; } @@ -1976,7 +1983,7 @@ class ServerHttp2Stream extends Http2Stream { processRespondWithFD.call(this, fd, headersList, options.offset, options.length, - getTrailers); + streamOptions); } // Initiate a file response on this Http2Stream. The path is passed to @@ -2018,14 +2025,14 @@ class ServerHttp2Stream extends Http2Stream { options.statCheck); } - let getTrailers = false; + let streamOptions = 0; if (options.getTrailers !== undefined) { if (typeof options.getTrailers !== 'function') { throw new errors.TypeError('ERR_INVALID_OPT_VALUE', 'getTrailers', options.getTrailers); } - getTrailers = true; + streamOptions |= STREAM_OPTION_GET_TRAILERS; state.getTrailers = options.getTrailers; } @@ -2039,7 +2046,7 @@ class ServerHttp2Stream extends Http2Stream { } fs.open(path, 'r', - afterOpen.bind(this, session, options, headers, getTrailers)); + afterOpen.bind(this, session, options, headers, streamOptions)); } // Sends a block of informational headers. In theory, the HTTP/2 spec diff --git a/src/node_http2.cc b/src/node_http2.cc index c1e178f7175935..8f666feaf0955f 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -479,7 +479,7 @@ void Http2Session::SubmitRstStream(const FunctionCallbackInfo& args) { void Http2Session::SubmitRequest(const FunctionCallbackInfo& args) { // args[0] Array of headers - // args[1] endStream boolean + // args[1] options int // args[2] parentStream ID (for priority spec) // args[3] weight (for priority spec) // args[4] exclusive boolean (for priority spec) @@ -492,15 +492,14 @@ void Http2Session::SubmitRequest(const FunctionCallbackInfo& args) { Isolate* isolate = env->isolate(); Local headers = args[0].As(); - bool endStream = args[1]->BooleanValue(context).ToChecked(); + int options = args[1]->IntegerValue(context).ToChecked(); int32_t parent = args[2]->Int32Value(context).ToChecked(); int32_t weight = args[3]->Int32Value(context).ToChecked(); bool exclusive = args[4]->BooleanValue(context).ToChecked(); - bool getTrailers = args[5]->BooleanValue(context).ToChecked(); - DEBUG_HTTP2("Http2Session: submitting request: headers: %d, end-stream: %d, " + DEBUG_HTTP2("Http2Session: submitting request: headers: %d, options: %d, " "parent: %d, weight: %d, exclusive: %d\n", headers->Length(), - endStream, parent, weight, exclusive); + options, parent, weight, exclusive); nghttp2_priority_spec prispec; nghttp2_priority_spec_init(&prispec, parent, weight, exclusive ? 1 : 0); @@ -509,8 +508,7 @@ void Http2Session::SubmitRequest(const FunctionCallbackInfo& args) { int32_t ret = session->Nghttp2Session::SubmitRequest(&prispec, *list, list.length(), - nullptr, endStream, - getTrailers); + nullptr, options); DEBUG_HTTP2("Http2Session: request submitted, response: %d\n", ret); args.GetReturnValue().Set(ret); } @@ -529,11 +527,10 @@ void Http2Session::SubmitResponse(const FunctionCallbackInfo& args) { int32_t id = args[0]->Int32Value(context).ToChecked(); Local headers = args[1].As(); - bool endStream = args[2]->BooleanValue(context).ToChecked(); - bool getTrailers = args[3]->BooleanValue(context).ToChecked(); + int options = args[2]->IntegerValue(context).ToChecked(); DEBUG_HTTP2("Http2Session: submitting response for stream %d: headers: %d, " - "end-stream: %d\n", id, headers->Length(), endStream); + "options: %d\n", id, headers->Length(), options); if (!(stream = session->FindStream(id))) { return args.GetReturnValue().Set(NGHTTP2_ERR_INVALID_STREAM_ID); @@ -542,7 +539,7 @@ void Http2Session::SubmitResponse(const FunctionCallbackInfo& args) { Headers list(isolate, context, headers); args.GetReturnValue().Set( - stream->SubmitResponse(*list, list.length(), endStream, getTrailers)); + stream->SubmitResponse(*list, list.length(), options)); } void Http2Session::SubmitFile(const FunctionCallbackInfo& args) { @@ -566,7 +563,7 @@ void Http2Session::SubmitFile(const FunctionCallbackInfo& args) { int64_t offset = args[3]->IntegerValue(context).ToChecked(); int64_t length = args[4]->IntegerValue(context).ToChecked(); - bool getTrailers = args[5]->BooleanValue(context).ToChecked(); + int options = args[5]->IntegerValue(context).ToChecked(); CHECK_GE(offset, 0); @@ -580,7 +577,7 @@ void Http2Session::SubmitFile(const FunctionCallbackInfo& args) { Headers list(isolate, context, headers); args.GetReturnValue().Set(stream->SubmitFile(fd, *list, list.length(), - offset, length, getTrailers)); + offset, length, options)); } void Http2Session::SendHeaders(const FunctionCallbackInfo& args) { @@ -719,10 +716,10 @@ void Http2Session::SubmitPushPromise(const FunctionCallbackInfo& args) { Nghttp2Stream* parent; int32_t id = args[0]->Int32Value(context).ToChecked(); Local headers = args[1].As(); - bool endStream = args[2]->BooleanValue(context).ToChecked(); + int options = args[2]->IntegerValue(context).ToChecked(); DEBUG_HTTP2("Http2Session: submitting push promise for stream %d: " - "end-stream: %d, headers: %d\n", id, endStream, + "options: %d, headers: %d\n", id, options, headers->Length()); if (!(parent = session->FindStream(id))) { @@ -732,7 +729,7 @@ void Http2Session::SubmitPushPromise(const FunctionCallbackInfo& args) { Headers list(isolate, context, headers); int32_t ret = parent->SubmitPushPromise(*list, list.length(), - nullptr, endStream); + nullptr, options); DEBUG_HTTP2("Http2Session: push promise submitted, ret: %d\n", ret); args.GetReturnValue().Set(ret); } @@ -1250,6 +1247,9 @@ void Initialize(Local target, NODE_DEFINE_HIDDEN_CONSTANT(constants, NGHTTP2_ERR_STREAM_CLOSED); NODE_DEFINE_CONSTANT(constants, NGHTTP2_ERR_FRAME_SIZE_ERROR); + NODE_DEFINE_HIDDEN_CONSTANT(constants, STREAM_OPTION_EMPTY_PAYLOAD); + NODE_DEFINE_HIDDEN_CONSTANT(constants, STREAM_OPTION_GET_TRAILERS); + NODE_DEFINE_CONSTANT(constants, NGHTTP2_FLAG_NONE); NODE_DEFINE_CONSTANT(constants, NGHTTP2_FLAG_END_STREAM); NODE_DEFINE_CONSTANT(constants, NGHTTP2_FLAG_END_HEADERS); diff --git a/src/node_http2_core-inl.h b/src/node_http2_core-inl.h index a3efbe119191f5..29f5b063801d10 100644 --- a/src/node_http2_core-inl.h +++ b/src/node_http2_core-inl.h @@ -614,10 +614,10 @@ inline Nghttp2Stream* Nghttp2Stream::Init( int32_t id, Nghttp2Session* session, nghttp2_headers_category category, - bool getTrailers) { + int options) { DEBUG_HTTP2("Nghttp2Stream %d: initializing stream\n", id); Nghttp2Stream* stream = stream_free_list.pop(); - stream->ResetState(id, session, category, getTrailers); + stream->ResetState(id, session, category, options); session->AddStream(stream); return stream; } @@ -628,7 +628,7 @@ inline void Nghttp2Stream::ResetState( int32_t id, Nghttp2Session* session, nghttp2_headers_category category, - bool getTrailers) { + int options) { DEBUG_HTTP2("Nghttp2Stream %d: resetting stream state\n", id); session_ = session; queue_head_ = nullptr; @@ -644,7 +644,7 @@ inline void Nghttp2Stream::ResetState( prev_local_window_size_ = 65535; queue_head_index_ = 0; queue_head_offset_ = 0; - getTrailers_ = getTrailers; + getTrailers_ = options & STREAM_OPTION_GET_TRAILERS; } @@ -735,7 +735,7 @@ inline int32_t Nghttp2Stream::SubmitPushPromise( nghttp2_nv* nva, size_t len, Nghttp2Stream** assigned, - bool emptyPayload) { + int options) { CHECK_GT(len, 0); DEBUG_HTTP2("Nghttp2Stream %d: sending push promise\n", id_); int32_t ret = nghttp2_submit_push_promise(session_->session(), @@ -744,7 +744,8 @@ inline int32_t Nghttp2Stream::SubmitPushPromise( nullptr); if (ret > 0) { auto stream = Nghttp2Stream::Init(ret, session_); - if (emptyPayload) stream->Shutdown(); + if (options & STREAM_OPTION_EMPTY_PAYLOAD) + stream->Shutdown(); if (assigned != nullptr) *assigned = stream; } return ret; @@ -756,16 +757,15 @@ inline int32_t Nghttp2Stream::SubmitPushPromise( // be sent. inline int Nghttp2Stream::SubmitResponse(nghttp2_nv* nva, size_t len, - bool emptyPayload, - bool getTrailers) { + int options) { CHECK_GT(len, 0); DEBUG_HTTP2("Nghttp2Stream %d: submitting response\n", id_); - getTrailers_ = getTrailers; + getTrailers_ = options & STREAM_OPTION_GET_TRAILERS; nghttp2_data_provider* provider = nullptr; nghttp2_data_provider prov; prov.source.ptr = this; prov.read_callback = Nghttp2Session::OnStreamRead; - if (!emptyPayload && IsWritable()) + if (IsWritable() && !(options & STREAM_OPTION_EMPTY_PAYLOAD)) provider = &prov; return nghttp2_submit_response(session_->session(), id_, @@ -777,11 +777,11 @@ inline int Nghttp2Stream::SubmitFile(int fd, nghttp2_nv* nva, size_t len, int64_t offset, int64_t length, - bool getTrailers) { + int options) { CHECK_GT(len, 0); CHECK_GT(fd, 0); DEBUG_HTTP2("Nghttp2Stream %d: submitting file\n", id_); - getTrailers_ = getTrailers; + getTrailers_ = options & STREAM_OPTION_GET_TRAILERS; nghttp2_data_provider prov; prov.source.ptr = this; prov.source.fd = fd; @@ -802,15 +802,14 @@ inline int32_t Nghttp2Session::SubmitRequest( nghttp2_nv* nva, size_t len, Nghttp2Stream** assigned, - bool emptyPayload, - bool getTrailers) { + int options) { CHECK_GT(len, 0); DEBUG_HTTP2("Nghttp2Session: submitting request\n"); nghttp2_data_provider* provider = nullptr; nghttp2_data_provider prov; prov.source.ptr = this; prov.read_callback = OnStreamRead; - if (!emptyPayload) + if (!(options & STREAM_OPTION_EMPTY_PAYLOAD)) provider = &prov; int32_t ret = nghttp2_submit_request(session_, prispec, nva, len, @@ -819,8 +818,9 @@ inline int32_t Nghttp2Session::SubmitRequest( if (ret > 0) { Nghttp2Stream* stream = Nghttp2Stream::Init(ret, this, NGHTTP2_HCAT_HEADERS, - getTrailers); - if (emptyPayload) stream->Shutdown(); + options); + if (options & STREAM_OPTION_EMPTY_PAYLOAD) + stream->Shutdown(); if (assigned != nullptr) *assigned = stream; } return ret; diff --git a/src/node_http2_core.h b/src/node_http2_core.h index 32f73648a3fc00..ff2321d0cb68a2 100644 --- a/src/node_http2_core.h +++ b/src/node_http2_core.h @@ -68,6 +68,10 @@ enum nghttp2_stream_flags { NGHTTP2_STREAM_FLAG_DESTROYED = 0x10 }; +enum nghttp2_stream_options { + STREAM_OPTION_EMPTY_PAYLOAD = 0x1, + STREAM_OPTION_GET_TRAILERS = 0x2, +}; // Callbacks typedef void (*nghttp2_stream_write_cb)( @@ -127,8 +131,7 @@ class Nghttp2Session { nghttp2_nv* nva, size_t len, Nghttp2Stream** assigned = nullptr, - bool emptyPayload = true, - bool getTrailers = false); + int options = 0); // Submits a notice to the connected peer that the session is in the // process of shutting down. @@ -299,7 +302,7 @@ class Nghttp2Stream { int32_t id, Nghttp2Session* session, nghttp2_headers_category category = NGHTTP2_HCAT_HEADERS, - bool getTrailers = false); + int options = 0); inline ~Nghttp2Stream() { CHECK_EQ(session_, nullptr); @@ -319,7 +322,7 @@ class Nghttp2Stream { int32_t id, Nghttp2Session* session, nghttp2_headers_category category = NGHTTP2_HCAT_HEADERS, - bool getTrailers = false); + int options = 0); // Destroy this stream instance and free all held memory. // Note that this will free queued outbound and inbound @@ -347,15 +350,14 @@ class Nghttp2Stream { // Initiate a response on this stream. inline int SubmitResponse(nghttp2_nv* nva, size_t len, - bool emptyPayload = false, - bool getTrailers = false); + int options); // Send data read from a file descriptor as the response on this stream. inline int SubmitFile(int fd, nghttp2_nv* nva, size_t len, int64_t offset, int64_t length, - bool getTrailers = false); + int options); // Submit informational headers for this stream inline int SubmitInfo(nghttp2_nv* nva, size_t len); @@ -372,7 +374,7 @@ class Nghttp2Stream { nghttp2_nv* nva, size_t len, Nghttp2Stream** assigned = nullptr, - bool writable = true); + int options = 0); // Marks the Writable side of the stream as being shutdown inline void Shutdown() {