From 8dfd5a515aae5ec12016ad3747cc40091292927d Mon Sep 17 00:00:00 2001 From: James M Snell Date: Fri, 3 Nov 2017 19:48:06 -0700 Subject: [PATCH] http2: multiple smaller code cleanups * Add Http2Priority utility class * Reduces some code duplication * Other small minor cleanups PR-URL: https://github.com/nodejs/node/pull/16764 Reviewed-By: Anatoli Papirovski --- src/node_http2.cc | 52 +++++++++++++------------------- src/node_http2.h | 14 +++++++++ src/node_http2_core-inl.h | 63 ++++++++++++++------------------------- 3 files changed, 57 insertions(+), 72 deletions(-) diff --git a/src/node_http2.cc b/src/node_http2.cc index f9025d1fd31724..83bd2781e48627 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -186,6 +186,18 @@ inline void Http2Settings::RefreshDefaults(Environment* env) { (1 << IDX_SETTINGS_MAX_HEADER_LIST_SIZE); } +Http2Priority::Http2Priority(Environment* env, + Local parent, + Local weight, + Local exclusive) { + Local context = env->context(); + int32_t parent_ = parent->Int32Value(context).ToChecked(); + int32_t weight_ = weight->Int32Value(context).ToChecked(); + bool exclusive_ = exclusive->BooleanValue(context).ToChecked(); + DEBUG_HTTP2("Http2Priority: parent: %d, weight: %d, exclusive: %d\n", + parent_, weight_, exclusive_); + nghttp2_priority_spec_init(&spec, parent_, weight_, exclusive_ ? 1 : 0); +} Http2Session::Http2Session(Environment* env, Local wrap, @@ -267,12 +279,8 @@ ssize_t Http2Session::OnCallbackPadding(size_t frameLen, buffer[PADDING_BUF_RETURN_VALUE] = frameLen; MakeCallback(env()->ongetpadding_string(), 0, nullptr); uint32_t retval = buffer[PADDING_BUF_RETURN_VALUE]; - retval = retval <= maxPayloadLen ? retval : maxPayloadLen; - retval = retval >= frameLen ? retval : frameLen; -#if defined(DEBUG) && DEBUG - CHECK_GE(retval, frameLen); - CHECK_LE(retval, maxPayloadLen); -#endif + retval = std::min(retval, static_cast(maxPayloadLen)); + retval = std::max(retval, static_cast(frameLen)); return retval; } @@ -454,30 +462,18 @@ void Http2Session::SubmitPriority(const FunctionCallbackInfo& args) { ASSIGN_OR_RETURN_UNWRAP(&session, args.Holder()); Local context = env->context(); - nghttp2_priority_spec spec; int32_t id = args[0]->Int32Value(context).ToChecked(); - int32_t parent = args[1]->Int32Value(context).ToChecked(); - int32_t weight = args[2]->Int32Value(context).ToChecked(); - bool exclusive = args[3]->BooleanValue(context).ToChecked(); + Http2Priority priority(env, args[1], args[2], args[3]); bool silent = args[4]->BooleanValue(context).ToChecked(); - DEBUG_HTTP2("Http2Session: submitting priority for stream %d: " - "parent: %d, weight: %d, exclusive: %d, silent: %d\n", - id, parent, weight, exclusive, silent); - -#if defined(DEBUG) && DEBUG - CHECK_GT(id, 0); - CHECK_GE(parent, 0); - CHECK_GE(weight, 0); -#endif + DEBUG_HTTP2("Http2Session: submitting priority for stream %d", id); Nghttp2Stream* stream; if (!(stream = session->FindStream(id))) { // invalid stream return args.GetReturnValue().Set(NGHTTP2_ERR_INVALID_STREAM_ID); } - nghttp2_priority_spec_init(&spec, parent, weight, exclusive ? 1 : 0); - args.GetReturnValue().Set(stream->SubmitPriority(&spec, silent)); + args.GetReturnValue().Set(stream->SubmitPriority(*priority, silent)); } void Http2Session::SubmitSettings(const FunctionCallbackInfo& args) { @@ -533,20 +529,14 @@ void Http2Session::SubmitRequest(const FunctionCallbackInfo& args) { Local headers = args[0].As(); 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(); - - DEBUG_HTTP2("Http2Session: submitting request: headers: %d, options: %d, " - "parent: %d, weight: %d, exclusive: %d\n", headers->Length(), - options, parent, weight, exclusive); + Http2Priority priority(env, args[2], args[3], args[4]); - nghttp2_priority_spec prispec; - nghttp2_priority_spec_init(&prispec, parent, weight, exclusive ? 1 : 0); + DEBUG_HTTP2("Http2Session: submitting request: headers: %d, options: %d\n", + headers->Length(), options); Headers list(isolate, context, headers); - int32_t ret = session->Nghttp2Session::SubmitRequest(&prispec, + int32_t ret = session->Nghttp2Session::SubmitRequest(*priority, *list, list.length(), nullptr, options); DEBUG_HTTP2("Http2Session: request submitted, response: %d\n", ret); diff --git a/src/node_http2.h b/src/node_http2.h index 1fee25a8d054b0..8e9f8c536b5ec5 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -368,6 +368,20 @@ class Http2Settings { MaybeStackBuffer entries_; }; +class Http2Priority { + public: + Http2Priority(Environment* env, + Local parent, + Local weight, + Local exclusive); + + nghttp2_priority_spec* operator*() { + return &spec; + } + private: + nghttp2_priority_spec spec; +}; + class Http2Session : public AsyncWrap, public StreamBase, public Nghttp2Session { diff --git a/src/node_http2_core-inl.h b/src/node_http2_core-inl.h index a4c6ef9b0eb579..de38a5df47b481 100644 --- a/src/node_http2_core-inl.h +++ b/src/node_http2_core-inl.h @@ -22,6 +22,13 @@ inline int Nghttp2Session::OnNghttpError(nghttp2_session* session, } #endif +inline int32_t GetFrameID(const nghttp2_frame* frame) { + // If this is a push promise, we want to grab the id of the promised stream + return (frame->hd.type == NGHTTP2_PUSH_PROMISE) ? + frame->push_promise.promised_stream_id : + frame->hd.stream_id; +} + // nghttp2 calls this at the beginning a new HEADERS or PUSH_PROMISE frame. // We use it to ensure that an Nghttp2Stream instance is allocated to store // the state. @@ -29,11 +36,7 @@ inline int Nghttp2Session::OnBeginHeadersCallback(nghttp2_session* session, const nghttp2_frame* frame, void* user_data) { Nghttp2Session* handle = static_cast(user_data); - // If this is a push promise frame, we want to grab the handle of - // the promised stream. - int32_t id = (frame->hd.type == NGHTTP2_PUSH_PROMISE) ? - frame->push_promise.promised_stream_id : - frame->hd.stream_id; + int32_t id = GetFrameID(frame); DEBUG_HTTP2("Nghttp2Session %s: beginning headers for stream %d\n", handle->TypeName(), id); @@ -79,11 +82,7 @@ inline int Nghttp2Session::OnHeaderCallback(nghttp2_session* session, uint8_t flags, void* user_data) { Nghttp2Session* handle = static_cast(user_data); - // If this is a push promise frame, we want to grab the handle of - // the promised stream. - int32_t id = (frame->hd.type == NGHTTP2_PUSH_PROMISE) ? - frame->push_promise.promised_stream_id : - frame->hd.stream_id; + int32_t id = GetFrameID(frame); Nghttp2Stream* stream = handle->FindStream(id); if (!stream->AddHeader(name, value, flags)) { // This will only happen if the connected peer sends us more @@ -432,7 +431,7 @@ inline void Nghttp2Stream::FlushDataChunks() { // see if the END_STREAM flag is set, and will flush the queued data chunks // to JS if the stream is flowing inline void Nghttp2Session::HandleDataFrame(const nghttp2_frame* frame) { - int32_t id = frame->hd.stream_id; + int32_t id = GetFrameID(frame); DEBUG_HTTP2("Nghttp2Session %s: handling data frame for stream %d\n", TypeName(), id); Nghttp2Stream* stream = this->FindStream(id); @@ -450,8 +449,7 @@ inline void Nghttp2Session::HandleDataFrame(const nghttp2_frame* frame) { // The headers are collected as the frame is being processed and sent out // to the JS side only when the frame is fully processed. inline void Nghttp2Session::HandleHeadersFrame(const nghttp2_frame* frame) { - int32_t id = (frame->hd.type == NGHTTP2_PUSH_PROMISE) ? - frame->push_promise.promised_stream_id : frame->hd.stream_id; + int32_t id = GetFrameID(frame); DEBUG_HTTP2("Nghttp2Session %s: handling headers frame for stream %d\n", TypeName(), id); Nghttp2Stream* stream = FindStream(id); @@ -469,7 +467,7 @@ inline void Nghttp2Session::HandleHeadersFrame(const nghttp2_frame* frame) { // Notifies the JS layer that a PRIORITY frame has been received inline void Nghttp2Session::HandlePriorityFrame(const nghttp2_frame* frame) { nghttp2_priority priority_frame = frame->priority; - int32_t id = frame->hd.stream_id; + int32_t id = GetFrameID(frame); DEBUG_HTTP2("Nghttp2Session %s: handling priority frame for stream %d\n", TypeName(), id); @@ -571,41 +569,24 @@ inline int Nghttp2Session::Init(const nghttp2_session_type type, session_type_ = type; DEBUG_HTTP2("Nghttp2Session %s: initializing session\n", TypeName()); destroying_ = false; - int ret = 0; max_header_pairs_ = maxHeaderPairs; nghttp2_session_callbacks* callbacks = callback_struct_saved[HasGetPaddingCallback() ? 1 : 0].callbacks; - nghttp2_option* opts; - if (options != nullptr) { - opts = options; - } else { - nghttp2_option_new(&opts); - } + CHECK_NE(options, nullptr); - switch (type) { - case NGHTTP2_SESSION_SERVER: - ret = nghttp2_session_server_new3(&session_, - callbacks, - this, - opts, - mem); - break; - case NGHTTP2_SESSION_CLIENT: - ret = nghttp2_session_client_new3(&session_, - callbacks, - this, - opts, - mem); - break; - } - if (opts != options) { - nghttp2_option_del(opts); - } + typedef int (*init_fn)(nghttp2_session** session, + const nghttp2_session_callbacks* callbacks, + void* user_data, + const nghttp2_option* options, + nghttp2_mem* mem); + init_fn fn = type == NGHTTP2_SESSION_SERVER ? + nghttp2_session_server_new3 : + nghttp2_session_client_new3; - return ret; + return fn(&session_, callbacks, this, options, mem); } inline void Nghttp2Session::MarkDestroying() {