From f38c9eaf8defc195ca98b39f19ee847a03fbc8a3 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Wed, 15 Apr 2020 15:21:32 -0700 Subject: [PATCH 01/18] http2: cleanup constants in http2 binding The error constants were just doing some weird things. Cleanup and improve maintainability. Signed-off-by: James M Snell --- src/node_http2.cc | 133 +++++++++++++--------------------------------- src/node_http2.h | 90 +++++++++++++++++++++++++++---- 2 files changed, 118 insertions(+), 105 deletions(-) diff --git a/src/node_http2.cc b/src/node_http2.cc index 7477bfbb6d8c10..eb02279d72a7b0 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -2970,6 +2970,9 @@ void Initialize(Local target, // Method to fetch the nghttp2 string description of an nghttp2 error code env->SetMethod(target, "nghttp2ErrorString", HttpErrorString); + env->SetMethod(target, "refreshDefaultSettings", RefreshDefaultSettings); + env->SetMethod(target, "packSettings", PackSettings); + env->SetMethod(target, "setCallbackFunctions", SetCallbackFunctions); Local http2SessionClassName = FIXED_ONE_BYTE_STRING(isolate, "Http2Session"); @@ -3038,113 +3041,53 @@ void Initialize(Local target, session->GetFunction(env->context()).ToLocalChecked()).Check(); Local constants = Object::New(isolate); - Local name_for_error_code = Array::New(isolate); - -#define NODE_NGHTTP2_ERROR_CODES(V) \ - V(NGHTTP2_SESSION_SERVER); \ - V(NGHTTP2_SESSION_CLIENT); \ - V(NGHTTP2_STREAM_STATE_IDLE); \ - V(NGHTTP2_STREAM_STATE_OPEN); \ - V(NGHTTP2_STREAM_STATE_RESERVED_LOCAL); \ - V(NGHTTP2_STREAM_STATE_RESERVED_REMOTE); \ - V(NGHTTP2_STREAM_STATE_HALF_CLOSED_LOCAL); \ - V(NGHTTP2_STREAM_STATE_HALF_CLOSED_REMOTE); \ - V(NGHTTP2_STREAM_STATE_CLOSED); \ - V(NGHTTP2_NO_ERROR); \ - V(NGHTTP2_PROTOCOL_ERROR); \ - V(NGHTTP2_INTERNAL_ERROR); \ - V(NGHTTP2_FLOW_CONTROL_ERROR); \ - V(NGHTTP2_SETTINGS_TIMEOUT); \ - V(NGHTTP2_STREAM_CLOSED); \ - V(NGHTTP2_FRAME_SIZE_ERROR); \ - V(NGHTTP2_REFUSED_STREAM); \ - V(NGHTTP2_CANCEL); \ - V(NGHTTP2_COMPRESSION_ERROR); \ - V(NGHTTP2_CONNECT_ERROR); \ - V(NGHTTP2_ENHANCE_YOUR_CALM); \ - V(NGHTTP2_INADEQUATE_SECURITY); \ - V(NGHTTP2_HTTP_1_1_REQUIRED); \ - -#define V(name) \ - NODE_DEFINE_CONSTANT(constants, name); \ - name_for_error_code->Set(env->context(), \ - static_cast(name), \ - FIXED_ONE_BYTE_STRING(isolate, \ - #name)).Check(); - NODE_NGHTTP2_ERROR_CODES(V) + + // This does alocate one more slot than needed but it's not used. +#define V(name) FIXED_ONE_BYTE_STRING(isolate, #name), + Local error_code_names[] = { + HTTP2_ERROR_CODES(V) + Local() // Unused. + }; #undef V - NODE_DEFINE_HIDDEN_CONSTANT(constants, NGHTTP2_HCAT_REQUEST); - NODE_DEFINE_HIDDEN_CONSTANT(constants, NGHTTP2_HCAT_RESPONSE); - NODE_DEFINE_HIDDEN_CONSTANT(constants, NGHTTP2_HCAT_PUSH_RESPONSE); - NODE_DEFINE_HIDDEN_CONSTANT(constants, NGHTTP2_HCAT_HEADERS); - NODE_DEFINE_HIDDEN_CONSTANT(constants, NGHTTP2_NV_FLAG_NONE); - NODE_DEFINE_HIDDEN_CONSTANT(constants, NGHTTP2_NV_FLAG_NO_INDEX); - NODE_DEFINE_HIDDEN_CONSTANT(constants, NGHTTP2_ERR_DEFERRED); - NODE_DEFINE_HIDDEN_CONSTANT(constants, NGHTTP2_ERR_STREAM_ID_NOT_AVAILABLE); - NODE_DEFINE_HIDDEN_CONSTANT(constants, NGHTTP2_ERR_INVALID_ARGUMENT); - 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); - NODE_DEFINE_CONSTANT(constants, NGHTTP2_FLAG_ACK); - NODE_DEFINE_CONSTANT(constants, NGHTTP2_FLAG_PADDED); - NODE_DEFINE_CONSTANT(constants, NGHTTP2_FLAG_PRIORITY); - - NODE_DEFINE_CONSTANT(constants, DEFAULT_SETTINGS_HEADER_TABLE_SIZE); - NODE_DEFINE_CONSTANT(constants, DEFAULT_SETTINGS_ENABLE_PUSH); - NODE_DEFINE_CONSTANT(constants, DEFAULT_SETTINGS_MAX_CONCURRENT_STREAMS); - NODE_DEFINE_CONSTANT(constants, DEFAULT_SETTINGS_INITIAL_WINDOW_SIZE); - NODE_DEFINE_CONSTANT(constants, DEFAULT_SETTINGS_MAX_FRAME_SIZE); - NODE_DEFINE_CONSTANT(constants, DEFAULT_SETTINGS_MAX_HEADER_LIST_SIZE); - NODE_DEFINE_CONSTANT(constants, DEFAULT_SETTINGS_ENABLE_CONNECT_PROTOCOL); - NODE_DEFINE_CONSTANT(constants, MAX_MAX_FRAME_SIZE); - NODE_DEFINE_CONSTANT(constants, MIN_MAX_FRAME_SIZE); - NODE_DEFINE_CONSTANT(constants, MAX_INITIAL_WINDOW_SIZE); - NODE_DEFINE_CONSTANT(constants, NGHTTP2_DEFAULT_WEIGHT); + Local name_for_error_code = + Array::New( + isolate, + error_code_names, + arraysize(error_code_names) - 1); + + target->Set(context, + FIXED_ONE_BYTE_STRING(isolate, "nameForErrorCode"), + name_for_error_code).Check(); + +#define V(constant) NODE_DEFINE_HIDDEN_CONSTANT(constants, constant); + HTTP2_HIDDEN_CONSTANTS(V) +#undef V - NODE_DEFINE_CONSTANT(constants, NGHTTP2_SETTINGS_HEADER_TABLE_SIZE); - NODE_DEFINE_CONSTANT(constants, NGHTTP2_SETTINGS_ENABLE_PUSH); - NODE_DEFINE_CONSTANT(constants, NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS); - NODE_DEFINE_CONSTANT(constants, NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE); - NODE_DEFINE_CONSTANT(constants, NGHTTP2_SETTINGS_MAX_FRAME_SIZE); - NODE_DEFINE_CONSTANT(constants, NGHTTP2_SETTINGS_MAX_HEADER_LIST_SIZE); - NODE_DEFINE_CONSTANT(constants, NGHTTP2_SETTINGS_ENABLE_CONNECT_PROTOCOL); +#define V(constant) NODE_DEFINE_CONSTANT(constants, constant); + HTTP2_CONSTANTS(V) +#undef V - NODE_DEFINE_CONSTANT(constants, PADDING_STRATEGY_NONE); - NODE_DEFINE_CONSTANT(constants, PADDING_STRATEGY_ALIGNED); - NODE_DEFINE_CONSTANT(constants, PADDING_STRATEGY_MAX); - NODE_DEFINE_CONSTANT(constants, PADDING_STRATEGY_CALLBACK); + // NGHTTP2_DEFAULT_WEIGHT is a macro and not a regular define + // it won't be set properly on the constants object if included + // in the HTTP2_CONSTANTS macro. + NODE_DEFINE_CONSTANT(constants, NGHTTP2_DEFAULT_WEIGHT); -#define STRING_CONSTANT(NAME, VALUE) \ +#define V(NAME, VALUE) \ NODE_DEFINE_STRING_CONSTANT(constants, "HTTP2_HEADER_" # NAME, VALUE); -HTTP_KNOWN_HEADERS(STRING_CONSTANT) -#undef STRING_CONSTANT + HTTP_KNOWN_HEADERS(V) +#undef V -#define STRING_CONSTANT(NAME, VALUE) \ +#define V(NAME, VALUE) \ NODE_DEFINE_STRING_CONSTANT(constants, "HTTP2_METHOD_" # NAME, VALUE); -HTTP_KNOWN_METHODS(STRING_CONSTANT) -#undef STRING_CONSTANT + HTTP_KNOWN_METHODS(V) +#undef V #define V(name, _) NODE_DEFINE_CONSTANT(constants, HTTP_STATUS_##name); -HTTP_STATUS_CODES(V) + HTTP_STATUS_CODES(V) #undef V - env->SetMethod(target, "refreshDefaultSettings", RefreshDefaultSettings); - env->SetMethod(target, "packSettings", PackSettings); - env->SetMethod(target, "setCallbackFunctions", SetCallbackFunctions); - - target->Set(context, - env->constants_string(), - constants).Check(); - target->Set(context, - FIXED_ONE_BYTE_STRING(isolate, "nameForErrorCode"), - name_for_error_code).Check(); + target->Set(context, env->constants_string(), constants).Check(); } } // namespace http2 } // namespace node diff --git a/src/node_http2.h b/src/node_http2.h index 1e5f99acfacec9..0d804dc8cae0a6 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -34,16 +34,16 @@ constexpr size_t kDefaultMaxSettings = 10; constexpr uint64_t kDefaultMaxSessionMemory = 10000000; // These are the standard HTTP/2 defaults as specified by the RFC -#define DEFAULT_SETTINGS_HEADER_TABLE_SIZE 4096 -#define DEFAULT_SETTINGS_ENABLE_PUSH 1 -#define DEFAULT_SETTINGS_MAX_CONCURRENT_STREAMS 0xffffffffu -#define DEFAULT_SETTINGS_INITIAL_WINDOW_SIZE 65535 -#define DEFAULT_SETTINGS_MAX_FRAME_SIZE 16384 -#define DEFAULT_SETTINGS_MAX_HEADER_LIST_SIZE 65535 -#define DEFAULT_SETTINGS_ENABLE_CONNECT_PROTOCOL 0 -#define MAX_MAX_FRAME_SIZE 16777215 -#define MIN_MAX_FRAME_SIZE DEFAULT_SETTINGS_MAX_FRAME_SIZE -#define MAX_INITIAL_WINDOW_SIZE 2147483647 +constexpr uint32_t DEFAULT_SETTINGS_HEADER_TABLE_SIZE = 4096; +constexpr uint32_t DEFAULT_SETTINGS_ENABLE_PUSH = 1; +constexpr uint32_t DEFAULT_SETTINGS_MAX_CONCURRENT_STREAMS = 0xffffffffu; +constexpr uint32_t DEFAULT_SETTINGS_INITIAL_WINDOW_SIZE = 65535; +constexpr uint32_t DEFAULT_SETTINGS_MAX_FRAME_SIZE = 16384; +constexpr uint32_t DEFAULT_SETTINGS_MAX_HEADER_LIST_SIZE = 65535; +constexpr uint32_t DEFAULT_SETTINGS_ENABLE_CONNECT_PROTOCOL = 0; +constexpr uint32_t MAX_MAX_FRAME_SIZE = 16777215; +constexpr uint32_t MIN_MAX_FRAME_SIZE = DEFAULT_SETTINGS_MAX_FRAME_SIZE; +constexpr uint32_t MAX_INITIAL_WINDOW_SIZE = 2147483647; template struct Nghttp2Deleter { @@ -1054,6 +1054,76 @@ class Origins { MaybeStackBuffer buf_; }; +#define HTTP2_HIDDEN_CONSTANTS(V) \ + V(NGHTTP2_HCAT_REQUEST) \ + V(NGHTTP2_HCAT_RESPONSE) \ + V(NGHTTP2_HCAT_PUSH_RESPONSE) \ + V(NGHTTP2_HCAT_HEADERS) \ + V(NGHTTP2_NV_FLAG_NONE) \ + V(NGHTTP2_NV_FLAG_NO_INDEX) \ + V(NGHTTP2_ERR_DEFERRED) \ + V(NGHTTP2_ERR_STREAM_ID_NOT_AVAILABLE) \ + V(NGHTTP2_ERR_INVALID_ARGUMENT) \ + V(NGHTTP2_ERR_STREAM_CLOSED) \ + V(STREAM_OPTION_EMPTY_PAYLOAD) \ + V(STREAM_OPTION_GET_TRAILERS) + +#define HTTP2_ERROR_CODES(V) \ + V(NGHTTP2_NO_ERROR) \ + V(NGHTTP2_PROTOCOL_ERROR) \ + V(NGHTTP2_INTERNAL_ERROR) \ + V(NGHTTP2_FLOW_CONTROL_ERROR) \ + V(NGHTTP2_SETTINGS_TIMEOUT) \ + V(NGHTTP2_STREAM_CLOSED) \ + V(NGHTTP2_FRAME_SIZE_ERROR) \ + V(NGHTTP2_REFUSED_STREAM) \ + V(NGHTTP2_CANCEL) \ + V(NGHTTP2_COMPRESSION_ERROR) \ + V(NGHTTP2_CONNECT_ERROR) \ + V(NGHTTP2_ENHANCE_YOUR_CALM) \ + V(NGHTTP2_INADEQUATE_SECURITY) \ + V(NGHTTP2_HTTP_1_1_REQUIRED) \ + +#define HTTP2_CONSTANTS(V) \ + V(NGHTTP2_ERR_FRAME_SIZE_ERROR) \ + V(NGHTTP2_SESSION_SERVER) \ + V(NGHTTP2_SESSION_CLIENT) \ + V(NGHTTP2_STREAM_STATE_IDLE) \ + V(NGHTTP2_STREAM_STATE_OPEN) \ + V(NGHTTP2_STREAM_STATE_RESERVED_LOCAL) \ + V(NGHTTP2_STREAM_STATE_RESERVED_REMOTE) \ + V(NGHTTP2_STREAM_STATE_HALF_CLOSED_LOCAL) \ + V(NGHTTP2_STREAM_STATE_HALF_CLOSED_REMOTE) \ + V(NGHTTP2_STREAM_STATE_CLOSED) \ + V(NGHTTP2_FLAG_NONE) \ + V(NGHTTP2_FLAG_END_STREAM) \ + V(NGHTTP2_FLAG_END_HEADERS) \ + V(NGHTTP2_FLAG_ACK) \ + V(NGHTTP2_FLAG_PADDED) \ + V(NGHTTP2_FLAG_PRIORITY) \ + V(DEFAULT_SETTINGS_HEADER_TABLE_SIZE) \ + V(DEFAULT_SETTINGS_ENABLE_PUSH) \ + V(DEFAULT_SETTINGS_MAX_CONCURRENT_STREAMS) \ + V(DEFAULT_SETTINGS_INITIAL_WINDOW_SIZE) \ + V(DEFAULT_SETTINGS_MAX_FRAME_SIZE) \ + V(DEFAULT_SETTINGS_MAX_HEADER_LIST_SIZE) \ + V(DEFAULT_SETTINGS_ENABLE_CONNECT_PROTOCOL) \ + V(MAX_MAX_FRAME_SIZE) \ + V(MIN_MAX_FRAME_SIZE) \ + V(MAX_INITIAL_WINDOW_SIZE) \ + V(NGHTTP2_SETTINGS_HEADER_TABLE_SIZE) \ + V(NGHTTP2_SETTINGS_ENABLE_PUSH) \ + V(NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS) \ + V(NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE) \ + V(NGHTTP2_SETTINGS_MAX_FRAME_SIZE) \ + V(NGHTTP2_SETTINGS_MAX_HEADER_LIST_SIZE) \ + V(NGHTTP2_SETTINGS_ENABLE_CONNECT_PROTOCOL) \ + V(PADDING_STRATEGY_NONE) \ + V(PADDING_STRATEGY_ALIGNED) \ + V(PADDING_STRATEGY_MAX) \ + V(PADDING_STRATEGY_CALLBACK) \ + HTTP2_ERROR_CODES(V) + } // namespace http2 } // namespace node From 219e339afc598150a97f7b3f09395fad329a2d2a Mon Sep 17 00:00:00 2001 From: James M Snell Date: Wed, 15 Apr 2020 15:48:43 -0700 Subject: [PATCH 02/18] http2: refactor GetPackedSettings to avoid unnecessary AsyncWrap Signed-off-by: James M Snell --- src/node_http2.cc | 86 +++++++++++++++++++++++++---------------------- src/node_http2.h | 17 +++++++--- 2 files changed, 58 insertions(+), 45 deletions(-) diff --git a/src/node_http2.cc b/src/node_http2.cc index eb02279d72a7b0..889c73222e74df 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -21,6 +21,7 @@ using v8::ArrayBuffer; using v8::ArrayBufferView; using v8::Boolean; using v8::Context; +using v8::EscapableHandleScope; using v8::Float64Array; using v8::Function; using v8::FunctionCallbackInfo; @@ -212,33 +213,33 @@ Http2Options::Http2Options(Http2State* http2_state, nghttp2_session_type type) { SetMaxSessionMemory(buffer[IDX_OPTIONS_MAX_SESSION_MEMORY] * 1000000); } -void Http2Session::Http2Settings::Init(Http2State* http2_state) { +#define GRABSETTING(entries, count, name) \ + do { \ + if (flags & (1 << IDX_SETTINGS_ ## name)) { \ + uint32_t val = buffer[IDX_SETTINGS_ ## name]; \ + entries[count++] = \ + nghttp2_settings_entry {NGHTTP2_SETTINGS_ ## name, val}; \ + } } while(0) + +size_t Http2Session::Http2Settings::Init( + Http2State* http2_state, + nghttp2_settings_entry* entries) { AliasedUint32Array& buffer = http2_state->settings_buffer; uint32_t flags = buffer[IDX_SETTINGS_COUNT]; - size_t n = 0; + size_t count = 0; -#define GRABSETTING(N, trace) \ - if (flags & (1 << IDX_SETTINGS_##N)) { \ - uint32_t val = buffer[IDX_SETTINGS_##N]; \ - if (session_ != nullptr) \ - Debug(session_, "setting " trace ": %d\n", val); \ - entries_[n++] = \ - nghttp2_settings_entry {NGHTTP2_SETTINGS_##N, val}; \ - } + GRABSETTING(entries, count, HEADER_TABLE_SIZE); + GRABSETTING(entries, count, MAX_CONCURRENT_STREAMS); + GRABSETTING(entries, count, MAX_FRAME_SIZE); + GRABSETTING(entries, count, INITIAL_WINDOW_SIZE); + GRABSETTING(entries, count, MAX_HEADER_LIST_SIZE); + GRABSETTING(entries, count, ENABLE_PUSH); + GRABSETTING(entries, count, ENABLE_CONNECT_PROTOCOL); - GRABSETTING(HEADER_TABLE_SIZE, "header table size"); - GRABSETTING(MAX_CONCURRENT_STREAMS, "max concurrent streams"); - GRABSETTING(MAX_FRAME_SIZE, "max frame size"); - GRABSETTING(INITIAL_WINDOW_SIZE, "initial window size"); - GRABSETTING(MAX_HEADER_LIST_SIZE, "max header list size"); - GRABSETTING(ENABLE_PUSH, "enable push"); - GRABSETTING(ENABLE_CONNECT_PROTOCOL, "enable connect protocol"); - -#undef GRABSETTING - - count_ = n; + return count; } +#undef GRABSETTING // The Http2Settings class is used to configure a SETTINGS frame that is // to be sent to the connected peer. The settings are set using a TypedArray @@ -250,23 +251,37 @@ Http2Session::Http2Settings::Http2Settings(Http2State* http2_state, : AsyncWrap(http2_state->env(), obj, PROVIDER_HTTP2SETTINGS), session_(session), startTime_(start_time) { - Init(http2_state); + count_ = Init(http2_state, entries_); } // Generates a Buffer that contains the serialized payload of a SETTINGS // frame. This can be used, for instance, to create the Base64-encoded // content of an Http2-Settings header field. Local Http2Session::Http2Settings::Pack() { - const size_t len = count_ * 6; - Local buf = Buffer::New(env(), len).ToLocalChecked(); + return Pack(session_->env(), count_, entries_); +} + +Local Http2Session::Http2Settings::Pack(Http2State* state) { + nghttp2_settings_entry entries[IDX_SETTINGS_COUNT]; + size_t count = Init(state, entries); + return Pack(state->env(), count, entries); +} + +Local Http2Session::Http2Settings::Pack( + Environment* env, + size_t count, + const nghttp2_settings_entry* entries) { + EscapableHandleScope scope(env->isolate()); + const size_t size = count * 6; + AllocatedBuffer buffer = env->AllocateManaged(size); ssize_t ret = nghttp2_pack_settings_payload( - reinterpret_cast(Buffer::Data(buf)), len, - &entries_[0], count_); - if (ret >= 0) - return buf; - else - return Undefined(env()->isolate()); + reinterpret_cast(buffer.data()), + size, + entries, + count); + Local undef = Undefined(env->isolate()); + return scope.Escape(ret >= 0 ? buffer.ToBuffer().FromMaybe(undef) : undef); } // Updates the shared TypedArray with the current remote or local settings for @@ -2330,16 +2345,7 @@ void HttpErrorString(const FunctionCallbackInfo& args) { // output for an HTTP2-Settings header field. void PackSettings(const FunctionCallbackInfo& args) { Http2State* state = Unwrap(args.Data()); - Environment* env = state->env(); - // TODO(addaleax): We should not be creating a full AsyncWrap for this. - Local obj; - if (!env->http2settings_constructor_template() - ->NewInstance(env->context()) - .ToLocal(&obj)) { - return; - } - Http2Session::Http2Settings settings(state, nullptr, obj); - args.GetReturnValue().Set(settings.Pack()); + args.GetReturnValue().Set(Http2Session::Http2Settings::Pack(state)); } // A TypedArray instance is shared between C++ and JS land to contain the diff --git a/src/node_http2.h b/src/node_http2.h index 0d804dc8cae0a6..66a1d7bad6502c 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -1005,10 +1005,7 @@ class Http2Session::Http2Settings : public AsyncWrap { v8::Local obj, uint64_t start_time = uv_hrtime()); - void MemoryInfo(MemoryTracker* tracker) const override { - tracker->TrackField("session", session_); - } - + SET_NO_MEMORY_INFO(); SET_MEMORY_INFO_NAME(Http2Settings) SET_SELF_SIZE(Http2Settings) @@ -1018,6 +1015,8 @@ class Http2Session::Http2Settings : public AsyncWrap { // Returns a Buffer instance with the serialized SETTINGS payload v8::Local Pack(); + static v8::Local Pack(Http2State* state); + // Resets the default values in the settings buffer static void RefreshDefaults(Http2State* http2_state); @@ -1026,7 +1025,15 @@ class Http2Session::Http2Settings : public AsyncWrap { get_setting fn); private: - void Init(Http2State* http2_state); + static size_t Init( + Http2State* http2_state, + nghttp2_settings_entry* entries); + + static v8::Local Pack( + Environment* env, + size_t count, + const nghttp2_settings_entry* entries); + Http2Session* session_; uint64_t startTime_; size_t count_ = 0; From 5499c7c621d9deef9bd6b1f20df93d08bdbdea77 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Wed, 15 Apr 2020 16:16:17 -0700 Subject: [PATCH 03/18] http2: simplify settings to reduce duplicate code Signed-off-by: James M Snell --- src/node_http2.cc | 62 +++++++++++++++-------------------------------- src/node_http2.h | 9 +++++++ 2 files changed, 28 insertions(+), 43 deletions(-) diff --git a/src/node_http2.cc b/src/node_http2.cc index 889c73222e74df..e6db9873d4a5dd 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -229,13 +229,9 @@ size_t Http2Session::Http2Settings::Init( size_t count = 0; - GRABSETTING(entries, count, HEADER_TABLE_SIZE); - GRABSETTING(entries, count, MAX_CONCURRENT_STREAMS); - GRABSETTING(entries, count, MAX_FRAME_SIZE); - GRABSETTING(entries, count, INITIAL_WINDOW_SIZE); - GRABSETTING(entries, count, MAX_HEADER_LIST_SIZE); - GRABSETTING(entries, count, ENABLE_PUSH); - GRABSETTING(entries, count, ENABLE_CONNECT_PROTOCOL); +#define V(name) GRABSETTING(entries, count, name); + HTTP2_SETTINGS(V) +#undef V return count; } @@ -289,48 +285,28 @@ Local Http2Session::Http2Settings::Pack( void Http2Session::Http2Settings::Update(Http2Session* session, get_setting fn) { AliasedUint32Array& buffer = session->http2_state()->settings_buffer; - buffer[IDX_SETTINGS_HEADER_TABLE_SIZE] = - fn(**session, NGHTTP2_SETTINGS_HEADER_TABLE_SIZE); - buffer[IDX_SETTINGS_MAX_CONCURRENT_STREAMS] = - fn(**session, NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS); - buffer[IDX_SETTINGS_INITIAL_WINDOW_SIZE] = - fn(**session, NGHTTP2_SETTINGS_INITIAL_WINDOW_SIZE); - buffer[IDX_SETTINGS_MAX_FRAME_SIZE] = - fn(**session, NGHTTP2_SETTINGS_MAX_FRAME_SIZE); - buffer[IDX_SETTINGS_MAX_HEADER_LIST_SIZE] = - fn(**session, NGHTTP2_SETTINGS_MAX_HEADER_LIST_SIZE); - buffer[IDX_SETTINGS_ENABLE_PUSH] = - fn(**session, NGHTTP2_SETTINGS_ENABLE_PUSH); - buffer[IDX_SETTINGS_ENABLE_CONNECT_PROTOCOL] = - fn(**session, NGHTTP2_SETTINGS_ENABLE_CONNECT_PROTOCOL); + +#define V(name) \ + buffer[IDX_SETTINGS_ ## name] = fn(**session, NGHTTP2_SETTINGS_ ## name); + HTTP2_SETTINGS(V) +#undef V + } // Initializes the shared TypedArray with the default settings values. void Http2Session::Http2Settings::RefreshDefaults(Http2State* http2_state) { AliasedUint32Array& buffer = http2_state->settings_buffer; + uint32_t flags = 0; + +#define V(name) \ + do { \ + buffer[IDX_SETTINGS_ ## name] = DEFAULT_SETTINGS_ ## name; \ + flags |= 1 << IDX_SETTINGS_ ## name; \ + } while (0); + HTTP2_SETTINGS(V) +#undef V - buffer[IDX_SETTINGS_HEADER_TABLE_SIZE] = - DEFAULT_SETTINGS_HEADER_TABLE_SIZE; - buffer[IDX_SETTINGS_ENABLE_PUSH] = - DEFAULT_SETTINGS_ENABLE_PUSH; - buffer[IDX_SETTINGS_MAX_CONCURRENT_STREAMS] = - DEFAULT_SETTINGS_MAX_CONCURRENT_STREAMS; - buffer[IDX_SETTINGS_INITIAL_WINDOW_SIZE] = - DEFAULT_SETTINGS_INITIAL_WINDOW_SIZE; - buffer[IDX_SETTINGS_MAX_FRAME_SIZE] = - DEFAULT_SETTINGS_MAX_FRAME_SIZE; - buffer[IDX_SETTINGS_MAX_HEADER_LIST_SIZE] = - DEFAULT_SETTINGS_MAX_HEADER_LIST_SIZE; - buffer[IDX_SETTINGS_ENABLE_CONNECT_PROTOCOL] = - DEFAULT_SETTINGS_ENABLE_CONNECT_PROTOCOL; - buffer[IDX_SETTINGS_COUNT] = - (1 << IDX_SETTINGS_HEADER_TABLE_SIZE) | - (1 << IDX_SETTINGS_ENABLE_PUSH) | - (1 << IDX_SETTINGS_MAX_CONCURRENT_STREAMS) | - (1 << IDX_SETTINGS_INITIAL_WINDOW_SIZE) | - (1 << IDX_SETTINGS_MAX_FRAME_SIZE) | - (1 << IDX_SETTINGS_MAX_HEADER_LIST_SIZE) | - (1 << IDX_SETTINGS_ENABLE_CONNECT_PROTOCOL); + buffer[IDX_SETTINGS_COUNT] = flags; } diff --git a/src/node_http2.h b/src/node_http2.h index 66a1d7bad6502c..cde10c56d03feb 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -1131,6 +1131,15 @@ class Origins { V(PADDING_STRATEGY_CALLBACK) \ HTTP2_ERROR_CODES(V) +#define HTTP2_SETTINGS(V) \ + V(HEADER_TABLE_SIZE) \ + V(ENABLE_PUSH) \ + V(MAX_CONCURRENT_STREAMS) \ + V(INITIAL_WINDOW_SIZE) \ + V(MAX_FRAME_SIZE) \ + V(MAX_HEADER_LIST_SIZE) \ + V(ENABLE_CONNECT_PROTOCOL) \ + } // namespace http2 } // namespace node From dd06df1d622a98598b7112f5a94f2efa065be538 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Wed, 15 Apr 2020 17:23:47 -0700 Subject: [PATCH 04/18] http2: improve style consistency and correctness Use snake_case for getters and setters at c++ level, avoid unnecessary use of enums, use consistent style for exported vs. internal constants, avoid unnecessary memory info reporting, use setters/getters for flags_ for improved code readability Signed-off-by: James M Snell --- src/node_http2.cc | 210 +++++++++++++++++----------------- src/node_http2.h | 282 ++++++++++++++++++++++++++-------------------- 2 files changed, 266 insertions(+), 226 deletions(-) diff --git a/src/node_http2.cc b/src/node_http2.cc index e6db9873d4a5dd..615462a79191b6 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -49,7 +49,7 @@ namespace { const char zero_bytes_256[256] = {}; -inline Http2Stream* GetStream(Http2Session* session, +Http2Stream* GetStream(Http2Session* session, int32_t id, nghttp2_data_source* source) { Http2Stream* stream = static_cast(source->ptr); @@ -86,13 +86,12 @@ Http2Scope::Http2Scope(Http2Session* session) { if (session == nullptr) return; - if (session->flags_ & (SESSION_STATE_HAS_SCOPE | - SESSION_STATE_WRITE_SCHEDULED)) { + if (session->is_in_scope() | session->is_write_scheduled()) { // There is another scope further below on the stack, or it is already // known that a write is scheduled. In either case, there is nothing to do. return; } - session->flags_ |= SESSION_STATE_HAS_SCOPE; + session->set_in_scope(); session_ = session; // Always keep the session object alive for at least as long as @@ -105,7 +104,7 @@ Http2Scope::~Http2Scope() { if (session_ == nullptr) return; - session_->flags_ &= ~SESSION_STATE_HAS_SCOPE; + session_->set_in_scope(false); session_->MaybeScheduleWrite(); } @@ -113,7 +112,7 @@ Http2Scope::~Http2Scope() { // instances to configure an appropriate nghttp2_options struct. The class // uses a single TypedArray instance that is shared with the JavaScript side // to more efficiently pass values back and forth. -Http2Options::Http2Options(Http2State* http2_state, nghttp2_session_type type) { +Http2Options::Http2Options(Http2State* http2_state, SessionType type) { nghttp2_option* option; CHECK_EQ(nghttp2_option_new(&option), 0); CHECK_NOT_NULL(option); @@ -172,10 +171,10 @@ Http2Options::Http2Options(Http2State* http2_state, nghttp2_session_type type) { // this is set on a per-session basis, but eventually we may switch to // a per-stream setting, giving users greater control if (flags & (1 << IDX_OPTIONS_PADDING_STRATEGY)) { - padding_strategy_type strategy = - static_cast( + PaddingStrategy strategy = + static_cast( buffer.GetValue(IDX_OPTIONS_PADDING_STRATEGY)); - SetPaddingStrategy(strategy); + set_padding_strategy(strategy); } // The max header list pairs option controls the maximum number of @@ -183,7 +182,7 @@ Http2Options::Http2Options(Http2State* http2_state, nghttp2_session_type type) { // if the remote peer sends more than this amount, the stream will be // automatically closed with an RST_STREAM. if (flags & (1 << IDX_OPTIONS_MAX_HEADER_LIST_PAIRS)) - SetMaxHeaderPairs(buffer[IDX_OPTIONS_MAX_HEADER_LIST_PAIRS]); + set_max_header_pairs(buffer[IDX_OPTIONS_MAX_HEADER_LIST_PAIRS]); // The HTTP2 specification places no limits on the number of HTTP2 // PING frames that can be sent. In order to prevent PINGS from being @@ -191,7 +190,7 @@ Http2Options::Http2Options(Http2State* http2_state, nghttp2_session_type type) { // on the number of unacknowledged PINGS that can be sent at any given // time. if (flags & (1 << IDX_OPTIONS_MAX_OUTSTANDING_PINGS)) - SetMaxOutstandingPings(buffer[IDX_OPTIONS_MAX_OUTSTANDING_PINGS]); + set_max_outstanding_pings(buffer[IDX_OPTIONS_MAX_OUTSTANDING_PINGS]); // The HTTP2 specification places no limits on the number of HTTP2 // SETTINGS frames that can be sent. In order to prevent PINGS from being @@ -199,7 +198,7 @@ Http2Options::Http2Options(Http2State* http2_state, nghttp2_session_type type) { // on the number of unacknowledged SETTINGS that can be sent at any given // time. if (flags & (1 << IDX_OPTIONS_MAX_OUTSTANDING_SETTINGS)) - SetMaxOutstandingSettings(buffer[IDX_OPTIONS_MAX_OUTSTANDING_SETTINGS]); + set_max_outstanding_settings(buffer[IDX_OPTIONS_MAX_OUTSTANDING_SETTINGS]); // The HTTP2 specification places no limits on the amount of memory // that a session can consume. In order to prevent abuse, we place a @@ -210,7 +209,7 @@ Http2Options::Http2Options(Http2State* http2_state, nghttp2_session_type type) { // Important: The maxSessionMemory option in javascript is expressed in // terms of MB increments (i.e. the value 1 == 1 MB) if (flags & (1 << IDX_OPTIONS_MAX_SESSION_MEMORY)) - SetMaxSessionMemory(buffer[IDX_OPTIONS_MAX_SESSION_MEMORY] * 1000000); + set_max_session_memory(buffer[IDX_OPTIONS_MAX_SESSION_MEMORY] * 1000000); } #define GRABSETTING(entries, count, name) \ @@ -460,7 +459,7 @@ void Http2Session::DecreaseAllocatedSize(size_t size) { Http2Session::Http2Session(Http2State* http2_state, Local wrap, - nghttp2_session_type type) + SessionType type) : AsyncWrap(http2_state->env(), wrap, AsyncWrap::PROVIDER_HTTP2SESSION), js_fields_(http2_state->env()->isolate()), session_type_(type), @@ -471,18 +470,18 @@ Http2Session::Http2Session(Http2State* http2_state, // Capture the configuration options for this session Http2Options opts(http2_state, type); - max_session_memory_ = opts.GetMaxSessionMemory(); + max_session_memory_ = opts.max_session_memory(); - uint32_t maxHeaderPairs = opts.GetMaxHeaderPairs(); + uint32_t maxHeaderPairs = opts.max_header_pairs(); max_header_pairs_ = type == NGHTTP2_SESSION_SERVER ? GetServerMaxHeaderPairs(maxHeaderPairs) : GetClientMaxHeaderPairs(maxHeaderPairs); - max_outstanding_pings_ = opts.GetMaxOutstandingPings(); - max_outstanding_settings_ = opts.GetMaxOutstandingSettings(); + max_outstanding_pings_ = opts.max_outstanding_pings(); + max_outstanding_settings_ = opts.max_outstanding_settings(); - padding_strategy_ = opts.GetPaddingStrategy(); + padding_strategy_ = opts.padding_strategy(); bool hasGetPaddingCallback = padding_strategy_ != PADDING_STRATEGY_NONE; @@ -516,7 +515,7 @@ Http2Session::Http2Session(Http2State* http2_state, } Http2Session::~Http2Session() { - CHECK_EQ(flags_ & SESSION_STATE_HAS_SCOPE, 0); + CHECK(!is_in_scope()); Debug(this, "freeing nghttp2 session"); // Explicitly reset session_ so the subsequent // current_nghttp2_memory_ check passes. @@ -529,7 +528,7 @@ std::string Http2Session::diagnostic_name() const { std::to_string(static_cast(get_async_id())) + ")"; } -inline bool HasHttp2Observer(Environment* env) { +bool HasHttp2Observer(Environment* env) { AliasedUint32Array& observers = env->performance_state()->observers; return observers[performance::NODE_PERFORMANCE_ENTRY_TYPE_HTTP2] != 0; } @@ -606,13 +605,13 @@ void Http2Session::EmitStatistics() { void Http2Session::Close(uint32_t code, bool socket_closed) { Debug(this, "closing session"); - if (flags_ & SESSION_STATE_CLOSING) + if (is_closing()) return; - flags_ |= SESSION_STATE_CLOSING; + set_closing(); // Stop reading on the i/o stream if (stream_ != nullptr) { - flags_ |= SESSION_STATE_READING_STOPPED; + set_reading_stopped(); stream_->ReadStop(); } @@ -628,10 +627,10 @@ void Http2Session::Close(uint32_t code, bool socket_closed) { stream_->RemoveStreamListener(this); } - flags_ |= SESSION_STATE_CLOSED; + set_destroyed(); // If we are writing we will get to make the callback in OnStreamAfterWrite. - if ((flags_ & SESSION_STATE_WRITE_IN_PROGRESS) == 0) { + if (!is_write_in_progress()) { Debug(this, "make done session callback"); HandleScope scope(env()->isolate()); MakeCallback(env()->ondone_string(), 0, nullptr); @@ -654,12 +653,12 @@ void Http2Session::Close(uint32_t code, bool socket_closed) { // Locates an existing known stream by ID. nghttp2 has a similar method // but this is faster and does not fail if the stream is not found. -inline Http2Stream* Http2Session::FindStream(int32_t id) { +Http2Stream* Http2Session::FindStream(int32_t id) { auto s = streams_.find(id); return s != streams_.end() ? s->second : nullptr; } -inline bool Http2Session::CanAddStream() { +bool Http2Session::CanAddStream() { uint32_t maxConcurrentStreams = nghttp2_session_get_local_settings( session_.get(), NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS); @@ -668,10 +667,10 @@ inline bool Http2Session::CanAddStream() { // We can add a new stream so long as we are less than the current // maximum on concurrent streams and there's enough available memory return streams_.size() < maxSize && - IsAvailableSessionMemory(sizeof(Http2Stream)); + has_available_session_memory(sizeof(Http2Stream)); } -inline void Http2Session::AddStream(Http2Stream* stream) { +void Http2Session::AddStream(Http2Stream* stream) { CHECK_GE(++statistics_.stream_count, 0); streams_[stream->id()] = stream; size_t size = streams_.size(); @@ -681,7 +680,7 @@ inline void Http2Session::AddStream(Http2Stream* stream) { } -inline void Http2Session::RemoveStream(Http2Stream* stream) { +void Http2Session::RemoveStream(Http2Stream* stream) { if (streams_.empty() || stream == nullptr) return; // Nothing to remove, item was never added? streams_.erase(stream->id()); @@ -729,7 +728,7 @@ ssize_t Http2Session::ConsumeHTTP2Data() { Debug(this, "receiving %d bytes [wants data? %d]", read_len, nghttp2_session_want_read(session_.get())); - flags_ &= ~SESSION_STATE_NGHTTP2_RECV_PAUSED; + set_receive_paused(false); ssize_t ret = nghttp2_session_mem_recv(session_.get(), reinterpret_cast(stream_buf_.base) + @@ -737,8 +736,8 @@ ssize_t Http2Session::ConsumeHTTP2Data() { read_len); CHECK_NE(ret, NGHTTP2_ERR_NOMEM); - if (flags_ & SESSION_STATE_NGHTTP2_RECV_PAUSED) { - CHECK_NE(flags_ & SESSION_STATE_READING_STOPPED, 0); + if (is_receive_paused()) { + CHECK(is_reading_stopped()); CHECK_GT(ret, 0); CHECK_LE(static_cast(ret), read_len); @@ -761,14 +760,14 @@ ssize_t Http2Session::ConsumeHTTP2Data() { return ret; // Send any data that was queued up while processing the received data. - if (!IsDestroyed()) { + if (!is_destroyed()) { SendPendingData(); } return ret; } -inline int32_t GetFrameID(const nghttp2_frame* frame) { +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 : @@ -804,7 +803,7 @@ int Http2Session::OnBeginHeadersCallback(nghttp2_session* handle, } session->rejected_stream_count_ = 0; - } else if (!stream->IsDestroyed()) { + } else if (!stream->is_destroyed()) { stream->StartHeaders(frame->headers.cat); } return 0; @@ -829,7 +828,7 @@ int Http2Session::OnHeaderCallback(nghttp2_session* handle, return NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE; // If the stream has already been destroyed, ignore. - if (!stream->IsDestroyed() && !stream->AddHeader(name, value, flags)) { + if (!stream->is_destroyed() && !stream->AddHeader(name, value, flags)) { // This will only happen if the connected peer sends us more // than the allowed number of header items at any given time stream->SubmitRstStream(NGHTTP2_ENHANCE_YOUR_CALM); @@ -970,7 +969,7 @@ int Http2Session::OnStreamClose(nghttp2_session* handle, Http2Stream* stream = session->FindStream(id); // Intentionally ignore the callback if the stream does not exist or has // already been destroyed - if (stream == nullptr || stream->IsDestroyed()) + if (stream == nullptr || stream->is_destroyed()) return 0; stream->Close(code); @@ -1030,7 +1029,7 @@ int Http2Session::OnDataChunkReceived(nghttp2_session* handle, CHECK_EQ(nghttp2_session_consume_connection(handle, len), 0); Http2Stream* stream = session->FindStream(id); // If the stream has been destroyed, ignore this chunk - if (stream->IsDestroyed()) + if (stream->is_destroyed()) return 0; stream->statistics_.received_bytes += len; @@ -1062,7 +1061,7 @@ int Http2Session::OnDataChunkReceived(nghttp2_session* handle, // If the stream owner (e.g. the JS Http2Stream) wants more data, just // tell nghttp2 that all data has been consumed. Otherwise, defer until // more data is being requested. - if (stream->IsReading()) + if (stream->is_reading()) nghttp2_session_consume_stream(handle, id, avail); else stream->inbound_consumed_data_while_paused_ += avail; @@ -1076,9 +1075,9 @@ int Http2Session::OnDataChunkReceived(nghttp2_session* handle, // If we are currently waiting for a write operation to finish, we should // tell nghttp2 that we want to wait before we process more input data. - if (session->flags_ & SESSION_STATE_WRITE_IN_PROGRESS) { - CHECK_NE(session->flags_ & SESSION_STATE_READING_STOPPED, 0); - session->flags_ |= SESSION_STATE_NGHTTP2_RECV_PAUSED; + if (session->is_write_in_progress()) { + CHECK(!session->is_reading_stopped()); + session->set_receive_paused(); return NGHTTP2_ERR_PAUSE; } @@ -1188,7 +1187,7 @@ void Http2Session::HandleHeadersFrame(const nghttp2_frame* frame) { Http2Stream* stream = FindStream(id); // If the stream has already been destroyed, ignore. - if (stream->IsDestroyed()) + if (stream->is_destroyed()) return; // The headers are stored as a vector of Http2Header instances. @@ -1256,7 +1255,7 @@ int Http2Session::HandleDataFrame(const nghttp2_frame* frame) { Debug(this, "handling data frame for stream %d", id); Http2Stream* stream = FindStream(id); - if (!stream->IsDestroyed() && frame->hd.flags & NGHTTP2_FLAG_END_STREAM) { + if (!stream->is_destroyed() && frame->hd.flags & NGHTTP2_FLAG_END_STREAM) { stream->EmitRead(UV_EOF); } else if (frame->hd.length == 0) { return 1; // Consider 0-length frame without END_STREAM an error. @@ -1421,20 +1420,20 @@ void Http2Session::HandleSettingsFrame(const nghttp2_frame* frame) { void Http2Session::OnStreamAfterWrite(WriteWrap* w, int status) { Debug(this, "write finished with status %d", status); - CHECK_NE(flags_ & SESSION_STATE_WRITE_IN_PROGRESS, 0); - flags_ &= ~SESSION_STATE_WRITE_IN_PROGRESS; + CHECK(is_write_in_progress()); + set_write_in_progress(false); // Inform all pending writes about their completion. ClearOutgoing(status); - if ((flags_ & SESSION_STATE_READING_STOPPED) && - !(flags_ & SESSION_STATE_WRITE_IN_PROGRESS) && + if (is_reading_stopped() && + !is_write_in_progress() && nghttp2_session_want_read(session_.get())) { - flags_ &= ~SESSION_STATE_READING_STOPPED; + set_reading_stopped(); stream_->ReadStart(); } - if ((flags_ & SESSION_STATE_CLOSED) != 0) { + if (is_destroyed()) { HandleScope scope(env()->isolate()); MakeCallback(env()->ondone_string(), 0, nullptr); return; @@ -1445,7 +1444,7 @@ void Http2Session::OnStreamAfterWrite(WriteWrap* w, int status) { ConsumeHTTP2Data(); } - if (!(flags_ & SESSION_STATE_WRITE_SCHEDULED)) { + if (!is_write_scheduled()) { // Schedule a new write if nghttp2 wants to send data. MaybeScheduleWrite(); } @@ -1456,17 +1455,17 @@ void Http2Session::OnStreamAfterWrite(WriteWrap* w, int status) { // on the next iteration of the Node.js event loop (using the SetImmediate // queue), but only if a write has not already been scheduled. void Http2Session::MaybeScheduleWrite() { - CHECK_EQ(flags_ & SESSION_STATE_WRITE_SCHEDULED, 0); + CHECK(!is_write_scheduled()); if (UNLIKELY(!session_)) return; if (nghttp2_session_want_write(session_.get())) { HandleScope handle_scope(env()->isolate()); Debug(this, "scheduling write"); - flags_ |= SESSION_STATE_WRITE_SCHEDULED; + set_write_scheduled(); BaseObjectPtr strong_ref{this}; env()->SetImmediate([this, strong_ref](Environment* env) { - if (!session_ || !(flags_ & SESSION_STATE_WRITE_SCHEDULED)) { + if (!session_ || !is_write_scheduled()) { // This can happen e.g. when a stream was reset before this turn // of the event loop, in which case SendPendingData() is called early, // or the session was destroyed in the meantime. @@ -1483,11 +1482,11 @@ void Http2Session::MaybeScheduleWrite() { } void Http2Session::MaybeStopReading() { - if (flags_ & SESSION_STATE_READING_STOPPED) return; + if (is_reading_stopped()) return; int want_read = nghttp2_session_want_read(session_.get()); Debug(this, "wants read? %d", want_read); - if (want_read == 0 || (flags_ & SESSION_STATE_WRITE_IN_PROGRESS)) { - flags_ |= SESSION_STATE_READING_STOPPED; + if (want_read == 0 || is_write_in_progress()) { + set_reading_stopped(); stream_->ReadStop(); } } @@ -1495,9 +1494,9 @@ void Http2Session::MaybeStopReading() { // Unset the sending state, finish up all current writes, and reset // storage for data and metadata that was associated with these writes. void Http2Session::ClearOutgoing(int status) { - CHECK_NE(flags_ & SESSION_STATE_SENDING, 0); + CHECK(is_sending()); - flags_ &= ~SESSION_STATE_SENDING; + set_sending(false); if (outgoing_buffers_.size() > 0) { outgoing_storage_.clear(); @@ -1564,15 +1563,15 @@ uint8_t Http2Session::SendPendingData() { // Do not attempt to send data on the socket if the destroying flag has // been set. That means everything is shutting down and the socket // will not be usable. - if (IsDestroyed()) + if (is_destroyed()) return 0; - flags_ &= ~SESSION_STATE_WRITE_SCHEDULED; + set_write_scheduled(false); // SendPendingData should not be called recursively. - if (flags_ & SESSION_STATE_SENDING) + if (is_sending()) return 1; // This is cleared by ClearOutgoing(). - flags_ |= SESSION_STATE_SENDING; + set_sending(); ssize_t src_length; const uint8_t* src; @@ -1626,11 +1625,11 @@ uint8_t Http2Session::SendPendingData() { chunks_sent_since_last_write_++; - CHECK_EQ(flags_ & SESSION_STATE_WRITE_IN_PROGRESS, 0); - flags_ |= SESSION_STATE_WRITE_IN_PROGRESS; + CHECK(!is_write_in_progress()); + set_write_in_progress(); StreamWriteResult res = underlying_stream()->Write(*bufs, count); if (!res.async) { - flags_ &= ~SESSION_STATE_WRITE_IN_PROGRESS; + set_write_in_progress(false); ClearOutgoing(res.err); } @@ -1837,7 +1836,7 @@ Http2Stream::Http2Stream(Http2Session* session, statistics_.start_time = uv_hrtime(); // Limit the number of header pairs - max_header_pairs_ = session->GetMaxHeaderPairs(); + max_header_pairs_ = session->max_header_pairs(); if (max_header_pairs_ == 0) { max_header_pairs_ = DEFAULT_MAX_HEADER_LIST_PAIRS; } @@ -1852,7 +1851,7 @@ Http2Stream::Http2Stream(Http2Session* session, MAX_MAX_HEADER_LIST_SIZE); if (options & STREAM_OPTION_GET_TRAILERS) - flags_ |= NGHTTP2_STREAM_FLAG_TRAILERS; + set_has_trailers(); PushStreamListener(&stream_listener_); @@ -1878,7 +1877,7 @@ std::string Http2Stream::diagnostic_name() const { // Notify the Http2Stream that a new block of HEADERS is being processed. void Http2Stream::StartHeaders(nghttp2_headers_category category) { Debug(this, "starting headers, category: %d", category); - CHECK(!this->IsDestroyed()); + CHECK(!this->is_destroyed()); session_->DecrementCurrentSessionMemory(current_headers_length_); current_headers_length_ = 0; current_headers_.clear(); @@ -1891,8 +1890,8 @@ nghttp2_stream* Http2Stream::operator*() { } void Http2Stream::Close(int32_t code) { - CHECK(!this->IsDestroyed()); - flags_ |= NGHTTP2_STREAM_FLAG_CLOSED; + CHECK(!this->is_destroyed()); + set_closed(); code_ = code; Debug(this, "closed with code %d", code); } @@ -1904,12 +1903,12 @@ ShutdownWrap* Http2Stream::CreateShutdownWrap(v8::Local object) { } int Http2Stream::DoShutdown(ShutdownWrap* req_wrap) { - if (IsDestroyed()) + if (is_destroyed()) return UV_EPIPE; { Http2Scope h2scope(this); - flags_ |= NGHTTP2_STREAM_FLAG_SHUT; + set_not_writable(); CHECK_NE(nghttp2_session_resume_data(**session_, id_), NGHTTP2_ERR_NOMEM); Debug(this, "writable side shutdown"); @@ -1922,11 +1921,11 @@ int Http2Stream::DoShutdown(ShutdownWrap* req_wrap) { // using the SetImmediate queue. void Http2Stream::Destroy() { // Do nothing if this stream instance is already destroyed - if (IsDestroyed()) + if (is_destroyed()) return; if (session_->HasPendingRstStream(id_)) FlushRstStream(); - flags_ |= NGHTTP2_STREAM_FLAG_DESTROYED; + set_destroyed(); Debug(this, "destroying stream"); @@ -1964,13 +1963,13 @@ void Http2Stream::Destroy() { // Initiates a response on the Http2Stream using data provided via the // StreamBase Streams API. int Http2Stream::SubmitResponse(const Http2Headers& headers, int options) { - CHECK(!this->IsDestroyed()); + CHECK(!this->is_destroyed()); Http2Scope h2scope(this); Debug(this, "submitting response"); if (options & STREAM_OPTION_GET_TRAILERS) - flags_ |= NGHTTP2_STREAM_FLAG_TRAILERS; + set_has_trailers(); - if (!IsWritable()) + if (!is_writable()) options |= STREAM_OPTION_EMPTY_PAYLOAD; Http2Stream::Provider::Stream prov(this, options); @@ -1987,7 +1986,7 @@ int Http2Stream::SubmitResponse(const Http2Headers& headers, int options) { // Submit informational headers for a stream. int Http2Stream::SubmitInfo(const Http2Headers& headers) { - CHECK(!this->IsDestroyed()); + CHECK(!this->is_destroyed()); Http2Scope h2scope(this); Debug(this, "sending %d informational headers", headers.length()); int ret = nghttp2_submit_headers( @@ -2004,18 +2003,18 @@ int Http2Stream::SubmitInfo(const Http2Headers& headers) { void Http2Stream::OnTrailers() { Debug(this, "let javascript know we are ready for trailers"); - CHECK(!this->IsDestroyed()); + CHECK(!this->is_destroyed()); Isolate* isolate = env()->isolate(); HandleScope scope(isolate); Local context = env()->context(); Context::Scope context_scope(context); - flags_ &= ~NGHTTP2_STREAM_FLAG_TRAILERS; + set_has_trailers(false); MakeCallback(env()->http2session_on_stream_trailers_function(), 0, nullptr); } // Submit informational headers for a stream. int Http2Stream::SubmitTrailers(const Http2Headers& headers) { - CHECK(!this->IsDestroyed()); + CHECK(!this->is_destroyed()); Http2Scope h2scope(this); Debug(this, "sending %d trailers", headers.length()); int ret; @@ -2039,7 +2038,7 @@ int Http2Stream::SubmitTrailers(const Http2Headers& headers) { // Submit a PRIORITY frame to the connected peer. int Http2Stream::SubmitPriority(nghttp2_priority_spec* prispec, bool silent) { - CHECK(!this->IsDestroyed()); + CHECK(!this->is_destroyed()); Http2Scope h2scope(this); Debug(this, "sending priority spec"); int ret = silent ? @@ -2055,7 +2054,7 @@ int Http2Stream::SubmitPriority(nghttp2_priority_spec* prispec, // Closes the Http2Stream by submitting an RST_STREAM frame to the connected // peer. void Http2Stream::SubmitRstStream(const uint32_t code) { - CHECK(!this->IsDestroyed()); + CHECK(!this->is_destroyed()); code_ = code; // If possible, force a purge of any currently pending data here to make sure // it is sent before closing the stream. If it returns non-zero then we need @@ -2070,7 +2069,7 @@ void Http2Stream::SubmitRstStream(const uint32_t code) { } void Http2Stream::FlushRstStream() { - if (IsDestroyed()) + if (is_destroyed()) return; Http2Scope h2scope(this); CHECK_EQ(nghttp2_submit_rst_stream(**session_, NGHTTP2_FLAG_NONE, @@ -2082,7 +2081,7 @@ void Http2Stream::FlushRstStream() { Http2Stream* Http2Stream::SubmitPushPromise(const Http2Headers& headers, int32_t* ret, int options) { - CHECK(!this->IsDestroyed()); + CHECK(!this->is_destroyed()); Http2Scope h2scope(this); Debug(this, "sending push promise"); *ret = nghttp2_submit_push_promise( @@ -2106,9 +2105,8 @@ Http2Stream* Http2Stream::SubmitPushPromise(const Http2Headers& headers, // out to JS land. int Http2Stream::ReadStart() { Http2Scope h2scope(this); - CHECK(!this->IsDestroyed()); - flags_ |= NGHTTP2_STREAM_FLAG_READ_START; - flags_ &= ~NGHTTP2_STREAM_FLAG_READ_PAUSED; + CHECK(!this->is_destroyed()); + set_reading(); Debug(this, "reading starting"); @@ -2124,10 +2122,10 @@ int Http2Stream::ReadStart() { // Switch the StreamBase into paused mode. int Http2Stream::ReadStop() { - CHECK(!this->IsDestroyed()); - if (!IsReading()) + CHECK(!this->is_destroyed()); + if (!is_reading()) return 0; - flags_ |= NGHTTP2_STREAM_FLAG_READ_PAUSED; + set_paused(); Debug(this, "reading stopped"); return 0; } @@ -2148,7 +2146,7 @@ int Http2Stream::DoWrite(WriteWrap* req_wrap, uv_stream_t* send_handle) { CHECK_NULL(send_handle); Http2Scope h2scope(this); - if (!IsWritable() || IsDestroyed()) { + if (!is_writable() || is_destroyed()) { req_wrap->Done(UV_EOF); return 0; } @@ -2174,7 +2172,7 @@ int Http2Stream::DoWrite(WriteWrap* req_wrap, bool Http2Stream::AddHeader(nghttp2_rcbuf* name, nghttp2_rcbuf* value, uint8_t flags) { - CHECK(!this->IsDestroyed()); + CHECK(!this->is_destroyed()); if (Http2RcBufferPointer::IsZeroLength(name)) return true; // Ignore empty headers. @@ -2183,7 +2181,7 @@ bool Http2Stream::AddHeader(nghttp2_rcbuf* name, size_t length = header.length() + 32; // A header can only be added if we have not exceeded the maximum number // of headers and the session has memory available for it. - if (!session_->IsAvailableSessionMemory(length) || + if (!session_->has_available_session_memory(length) || current_headers_.size() == max_header_pairs_ || current_headers_length_ + length > max_header_length_) { return false; @@ -2201,7 +2199,7 @@ bool Http2Stream::AddHeader(nghttp2_rcbuf* name, // A Provider is the thing that provides outbound DATA frame data. Http2Stream::Provider::Provider(Http2Stream* stream, int options) { - CHECK(!stream->IsDestroyed()); + CHECK(!stream->is_destroyed()); provider_.source.ptr = stream; empty_ = options & STREAM_OPTION_EMPTY_PAYLOAD; } @@ -2266,21 +2264,21 @@ ssize_t Http2Stream::Provider::Stream::OnRead(nghttp2_session* handle, } } - if (amount == 0 && stream->IsWritable()) { + if (amount == 0 && stream->is_writable()) { CHECK(stream->queue_.empty()); Debug(session, "deferring stream %d", id); stream->EmitWantsWrite(length); - if (stream->available_outbound_length_ > 0 || !stream->IsWritable()) { + if (stream->available_outbound_length_ > 0 || !stream->is_writable()) { // EmitWantsWrite() did something interesting synchronously, restart: return OnRead(handle, id, buf, length, flags, source, user_data); } return NGHTTP2_ERR_DEFERRED; } - if (stream->queue_.empty() && !stream->IsWritable()) { + if (stream->queue_.empty() && !stream->is_writable()) { Debug(session, "no more data for stream %d", id); *flags |= NGHTTP2_DATA_FLAG_EOF; - if (stream->HasTrailers()) { + if (stream->has_trailers()) { *flags |= NGHTTP2_DATA_FLAG_NO_END_STREAM; stream->OnTrailers(); } @@ -2290,12 +2288,12 @@ ssize_t Http2Stream::Provider::Stream::OnRead(nghttp2_session* handle, return amount; } -inline void Http2Stream::IncrementAvailableOutboundLength(size_t amount) { +void Http2Stream::IncrementAvailableOutboundLength(size_t amount) { available_outbound_length_ += amount; session_->IncrementCurrentSessionMemory(amount); } -inline void Http2Stream::DecrementAvailableOutboundLength(size_t amount) { +void Http2Stream::DecrementAvailableOutboundLength(size_t amount) { available_outbound_length_ -= amount; session_->DecrementCurrentSessionMemory(amount); } @@ -2396,7 +2394,7 @@ void Http2Session::New(const FunctionCallbackInfo& args) { Environment* env = state->env(); CHECK(args.IsConstructCall()); int32_t val = args[0]->Int32Value(env->context()).ToChecked(); - nghttp2_session_type type = static_cast(val); + SessionType type = static_cast(val); Http2Session* session = new Http2Session(state, args.This(), type); session->get_async_id(); // avoid compiler warning Debug(session, "session created"); @@ -2460,7 +2458,7 @@ void Http2Session::Goaway(uint32_t code, int32_t lastStreamID, const uint8_t* data, size_t len) { - if (IsDestroyed()) + if (is_destroyed()) return; Http2Scope h2scope(this); diff --git a/src/node_http2.h b/src/node_http2.h index cde10c56d03feb..ea7d07777f7d08 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -21,6 +21,10 @@ namespace node { namespace http2 { +// Constants in all caps are exported as user-facing constants +// in JavaScript. Constants using the kName pattern are internal +// only. + // We strictly limit the number of outstanding unacknowledged PINGS a user // may send in order to prevent abuse. The current default cap is 10. The // user may set a different limit using a per Http2Session configuration @@ -45,6 +49,52 @@ constexpr uint32_t MAX_MAX_FRAME_SIZE = 16777215; constexpr uint32_t MIN_MAX_FRAME_SIZE = DEFAULT_SETTINGS_MAX_FRAME_SIZE; constexpr uint32_t MAX_INITIAL_WINDOW_SIZE = 2147483647; +// Stream is not going to have any DATA frames +constexpr int STREAM_OPTION_EMPTY_PAYLOAD = 0x1; + +// Stream might have trailing headers +constexpr int STREAM_OPTION_GET_TRAILERS = 0x2; + +// Http2Stream internal states +constexpr int kStreamStateNone = 0x0; +constexpr int kStreamStateShut = 0x1; +constexpr int kStreamStateReadStart = 0x2; +constexpr int kStreamStateReadPaused = 0x4; +constexpr int kStreamStateClosed = 0x8; +constexpr int kStreamStateDestroyed = 0x10; +constexpr int kStreamStateTrailers = 0x20; + +// Http2Session internal states +constexpr int kSessionStateNone = 0x0; +constexpr int kSessionStateHasScope = 0x1; +constexpr int kSessionStateWriteScheduled = 0x2; +constexpr int kSessionStateClosed = 0x4; +constexpr int kSessionStateClosing = 0x8; +constexpr int kSessionStateSending = 0x10; +constexpr int kSessionStateWriteInProgress = 0x20; +constexpr int kSessionStateReadingStopped = 0x40; +constexpr int kSessionStateReceivePaused = 0x80; + +// The Padding Strategy determines the method by which extra padding is +// selected for HEADERS and DATA frames. These are configurable via the +// options passed in to a Http2Session object. +enum PaddingStrategy { + // No padding strategy. This is the default. + PADDING_STRATEGY_NONE, + // Attempts to ensure that the frame is 8-byte aligned + PADDING_STRATEGY_ALIGNED, + // Padding will ensure all data frames are maxFrameSize + PADDING_STRATEGY_MAX, + // Removed and turned into an alias because it is unreasonably expensive for + // very little benefit. + PADDING_STRATEGY_CALLBACK = PADDING_STRATEGY_ALIGNED +}; + +enum SessionType { + NGHTTP2_SESSION_SERVER, + NGHTTP2_SESSION_CLIENT +}; + template struct Nghttp2Deleter { void operator()(T* ptr) const noexcept { fn(ptr); } @@ -93,37 +143,6 @@ struct Http2RcBufferPointerTraits { using Http2Headers = NgHeaders; using Http2RcBufferPointer = NgRcBufPointer; - -enum nghttp2_session_type { - NGHTTP2_SESSION_SERVER, - NGHTTP2_SESSION_CLIENT -}; - -enum nghttp2_stream_flags { - NGHTTP2_STREAM_FLAG_NONE = 0x0, - // Writable side has ended - NGHTTP2_STREAM_FLAG_SHUT = 0x1, - // Reading has started - NGHTTP2_STREAM_FLAG_READ_START = 0x2, - // Reading is paused - NGHTTP2_STREAM_FLAG_READ_PAUSED = 0x4, - // Stream is closed - NGHTTP2_STREAM_FLAG_CLOSED = 0x8, - // Stream is destroyed - NGHTTP2_STREAM_FLAG_DESTROYED = 0x10, - // Stream has trailers - NGHTTP2_STREAM_FLAG_TRAILERS = 0x20, - // Stream has received all the data it can - NGHTTP2_STREAM_FLAG_EOS = 0x40 -}; - -enum nghttp2_stream_options { - // Stream is not going to have any DATA frames - STREAM_OPTION_EMPTY_PAYLOAD = 0x1, - // Stream might have trailing headers - STREAM_OPTION_GET_TRAILERS = 0x2, -}; - struct NgHttp2StreamWrite : public MemoryRetainer { WriteWrap* req_wrap = nullptr; uv_buf_t buf; @@ -137,33 +156,6 @@ struct NgHttp2StreamWrite : public MemoryRetainer { SET_SELF_SIZE(NgHttp2StreamWrite) }; -// The Padding Strategy determines the method by which extra padding is -// selected for HEADERS and DATA frames. These are configurable via the -// options passed in to a Http2Session object. -enum padding_strategy_type { - // No padding strategy. This is the default. - PADDING_STRATEGY_NONE, - // Attempts to ensure that the frame is 8-byte aligned - PADDING_STRATEGY_ALIGNED, - // Padding will ensure all data frames are maxFrameSize - PADDING_STRATEGY_MAX, - // Removed and turned into an alias because it is unreasonably expensive for - // very little benefit. - PADDING_STRATEGY_CALLBACK = PADDING_STRATEGY_ALIGNED -}; - -enum session_state_flags { - SESSION_STATE_NONE = 0x0, - SESSION_STATE_HAS_SCOPE = 0x1, - SESSION_STATE_WRITE_SCHEDULED = 0x2, - SESSION_STATE_CLOSED = 0x4, - SESSION_STATE_CLOSING = 0x8, - SESSION_STATE_SENDING = 0x10, - SESSION_STATE_WRITE_IN_PROGRESS = 0x20, - SESSION_STATE_READING_STOPPED = 0x40, - SESSION_STATE_NGHTTP2_RECV_PAUSED = 0x80 -}; - typedef uint32_t(*get_setting)(nghttp2_session* session, nghttp2_settings_id id); @@ -191,7 +183,7 @@ class Http2Scope { class Http2Options { public: Http2Options(Http2State* http2_state, - nghttp2_session_type type); + SessionType type); ~Http2Options() = default; @@ -199,43 +191,43 @@ class Http2Options { return options_.get(); } - void SetMaxHeaderPairs(uint32_t max) { + void set_max_header_pairs(uint32_t max) { max_header_pairs_ = max; } - uint32_t GetMaxHeaderPairs() const { + uint32_t max_header_pairs() const { return max_header_pairs_; } - void SetPaddingStrategy(padding_strategy_type val) { + void set_padding_strategy(PaddingStrategy val) { padding_strategy_ = val; } - padding_strategy_type GetPaddingStrategy() const { + PaddingStrategy padding_strategy() const { return padding_strategy_; } - void SetMaxOutstandingPings(size_t max) { + void set_max_outstanding_pings(size_t max) { max_outstanding_pings_ = max; } - size_t GetMaxOutstandingPings() const { + size_t max_outstanding_pings() const { return max_outstanding_pings_; } - void SetMaxOutstandingSettings(size_t max) { + void set_max_outstanding_settings(size_t max) { max_outstanding_settings_ = max; } - size_t GetMaxOutstandingSettings() const { + size_t max_outstanding_settings() const { return max_outstanding_settings_; } - void SetMaxSessionMemory(uint64_t max) { + void set_max_session_memory(uint64_t max) { max_session_memory_ = max; } - uint64_t GetMaxSessionMemory() const { + uint64_t max_session_memory() const { return max_session_memory_; } @@ -243,7 +235,7 @@ class Http2Options { Nghttp2OptionPointer options_; uint64_t max_session_memory_ = kDefaultMaxSessionMemory; uint32_t max_header_pairs_ = DEFAULT_MAX_HEADER_LIST_PAIRS; - padding_strategy_type padding_strategy_ = PADDING_STRATEGY_NONE; + PaddingStrategy padding_strategy_ = PADDING_STRATEGY_NONE; size_t max_outstanding_pings_ = kDefaultMaxPings; size_t max_outstanding_settings_ = kDefaultMaxSettings; }; @@ -331,42 +323,74 @@ class Http2Stream : public AsyncWrap, // Destroy this stream instance and free all held memory. void Destroy(); - inline bool IsDestroyed() const { - return flags_ & NGHTTP2_STREAM_FLAG_DESTROYED; + bool is_destroyed() const { + return flags_ & kStreamStateDestroyed; + } + + bool is_writable() const { + return !(flags_ & kStreamStateShut); + } + + bool is_paused() const { + return flags_ & kStreamStateReadPaused; + } + + bool is_closed() const { + return flags_ & kStreamStateClosed; } - inline bool IsWritable() const { - return !(flags_ & NGHTTP2_STREAM_FLAG_SHUT); + bool has_trailers() const { + return flags_ & kStreamStateTrailers; } - inline bool IsPaused() const { - return flags_ & NGHTTP2_STREAM_FLAG_READ_PAUSED; + void set_has_trailers(bool on = true) { + if (on) + flags_ |= kStreamStateTrailers; + else + flags_ &= ~kStreamStateTrailers; } - inline bool IsClosed() const { - return flags_ & NGHTTP2_STREAM_FLAG_CLOSED; + void set_closed() { + flags_ |= kStreamStateClosed; } - inline bool HasTrailers() const { - return flags_ & NGHTTP2_STREAM_FLAG_TRAILERS; + void set_destroyed() { + flags_ |= kStreamStateDestroyed; + } + + void set_not_writable() { + flags_ |= kStreamStateShut; + } + + void set_reading(bool on = true) { + if (on) { + flags_ |= kStreamStateReadStart; + set_paused(false); + } else {} + } + + void set_paused(bool on = true) { + if (on) + flags_ |= kStreamStateReadPaused; + else + flags_ &= ~kStreamStateReadPaused; } // Returns true if this stream is in the reading state, which occurs when - // the NGHTTP2_STREAM_FLAG_READ_START flag has been set and the - // NGHTTP2_STREAM_FLAG_READ_PAUSED flag is *not* set. - inline bool IsReading() const { - return flags_ & NGHTTP2_STREAM_FLAG_READ_START && - !(flags_ & NGHTTP2_STREAM_FLAG_READ_PAUSED); + // the kStreamStateReadStart flag has been set and the + // kStreamStateReadPaused flag is *not* set. + bool is_reading() const { + return flags_ & kStreamStateReadStart && !is_paused(); } // Returns the RST_STREAM code used to close this stream - inline int32_t code() const { return code_; } + int32_t code() const { return code_; } // Returns the stream identifier for this stream - inline int32_t id() const { return id_; } + int32_t id() const { return id_; } - inline void IncrementAvailableOutboundLength(size_t amount); - inline void DecrementAvailableOutboundLength(size_t amount); + void IncrementAvailableOutboundLength(size_t amount); + void DecrementAvailableOutboundLength(size_t amount); bool AddHeader(nghttp2_rcbuf* name, nghttp2_rcbuf* value, uint8_t flags); @@ -382,7 +406,7 @@ class Http2Stream : public AsyncWrap, return current_headers_.size(); } - inline nghttp2_headers_category headers_category() const { + nghttp2_headers_category headers_category() const { return current_headers_category_; } @@ -448,7 +472,7 @@ class Http2Stream : public AsyncWrap, BaseObjectWeakPtr session_; // The Parent HTTP/2 Session int32_t id_ = 0; // The Stream Identifier int32_t code_ = NGHTTP2_NO_ERROR; // The RST_STREAM code (if any) - int flags_ = NGHTTP2_STREAM_FLAG_NONE; // Internal state flags + int flags_ = kStreamStateNone; // Internal state flags uint32_t max_header_pairs_ = DEFAULT_MAX_HEADER_LIST_PAIRS; uint32_t max_header_length_ = DEFAULT_SETTINGS_MAX_HEADER_LIST_SIZE; @@ -545,7 +569,7 @@ class Http2Session : public AsyncWrap, public: Http2Session(Http2State* http2_state, v8::Local wrap, - nghttp2_session_type type = NGHTTP2_SESSION_SERVER); + SessionType type = NGHTTP2_SESSION_SERVER); ~Http2Session() override; class Http2Ping; @@ -553,7 +577,7 @@ class Http2Session : public AsyncWrap, void EmitStatistics(); - inline StreamBase* underlying_stream() { + StreamBase* underlying_stream() { return static_cast(stream_); } @@ -580,20 +604,41 @@ class Http2Session : public AsyncWrap, int32_t* ret, int options = 0); - inline nghttp2_session_type type() const { return session_type_; } + SessionType type() const { return session_type_; } + + nghttp2_session* session() const { return session_.get(); } - inline nghttp2_session* session() const { return session_.get(); } + nghttp2_session* operator*() { return session_.get(); } - inline nghttp2_session* operator*() { return session_.get(); } + uint32_t max_header_pairs() const { return max_header_pairs_; } - inline uint32_t GetMaxHeaderPairs() const { return max_header_pairs_; } + const char* TypeName() const; - inline const char* TypeName() const; + bool is_destroyed() { + return (flags_ & kSessionStateClosed) || session_ == nullptr; + } + + void set_destroyed() { + flags_ |= kSessionStateClosed; + } - inline bool IsDestroyed() { - return (flags_ & SESSION_STATE_CLOSED) || session_ == nullptr; +#define IS_FLAG(name, flag) \ + bool is_##name() const { return flags_ & flag; } \ + void set_##name(bool on = true) { \ + if (on) flags_ |= flag; \ + else flags_ &= ~flag; \ } + IS_FLAG(in_scope, kSessionStateHasScope) + IS_FLAG(write_scheduled, kSessionStateWriteScheduled) + IS_FLAG(closing, kSessionStateClosing) + IS_FLAG(sending, kSessionStateSending) + IS_FLAG(write_in_progress, kSessionStateWriteInProgress) + IS_FLAG(reading_stopped, kSessionStateReadingStopped) + IS_FLAG(receive_paused, kSessionStateReceivePaused) + +#undef IS_FLAG + // Schedule a write if nghttp2 indicates it wants to write to the socket. void MaybeScheduleWrite(); @@ -601,15 +646,15 @@ class Http2Session : public AsyncWrap, void MaybeStopReading(); // Returns pointer to the stream, or nullptr if stream does not exist - inline Http2Stream* FindStream(int32_t id); + Http2Stream* FindStream(int32_t id); - inline bool CanAddStream(); + bool CanAddStream(); // Adds a stream instance to this session - inline void AddStream(Http2Stream* stream); + void AddStream(Http2Stream* stream); // Removes a stream instance from this session - inline void RemoveStream(Http2Stream* stream); + void RemoveStream(Http2Stream* stream); // Indicates whether there currently exist outgoing buffers for this stream. bool HasWritesOnSocketForStream(Http2Stream* stream); @@ -635,11 +680,11 @@ class Http2Session : public AsyncWrap, std::string diagnostic_name() const override; // Schedule an RstStream for after the current write finishes. - inline void AddPendingRstStream(int32_t stream_id) { + void AddPendingRstStream(int32_t stream_id) { pending_rst_streams_.emplace_back(stream_id); } - inline bool HasPendingRstStream(int32_t stream_id) { + bool HasPendingRstStream(int32_t stream_id) { return pending_rst_streams_.end() != std::find(pending_rst_streams_.begin(), pending_rst_streams_.end(), stream_id); @@ -700,7 +745,7 @@ class Http2Session : public AsyncWrap, // Returns the current session memory including memory allocated by nghttp2, // the current outbound storage queue, and pending writes. - uint64_t GetCurrentSessionMemory() { + uint64_t current_session_memory() const { uint64_t total = current_session_memory_ + sizeof(Http2Session); total += current_nghttp2_memory_; total += outgoing_storage_.size(); @@ -708,8 +753,8 @@ class Http2Session : public AsyncWrap, } // Return true if current_session_memory + amount is less than the max - bool IsAvailableSessionMemory(uint64_t amount) { - return GetCurrentSessionMemory() + amount <= max_session_memory_; + bool has_available_session_memory(uint64_t amount) const { + return current_session_memory() + amount <= max_session_memory_; } struct Statistics { @@ -812,7 +857,7 @@ class Http2Session : public AsyncWrap, void* user_data); struct Callbacks { - inline explicit Callbacks(bool kHasGetPaddingCallback); + explicit Callbacks(bool kHasGetPaddingCallback); Nghttp2SessionCallbacksPointer callbacks; }; @@ -827,7 +872,7 @@ class Http2Session : public AsyncWrap, AliasedStruct js_fields_; // The session type: client or server - nghttp2_session_type session_type_; + SessionType session_type_; // The maximum number of header pairs permitted for streams on this session uint32_t max_header_pairs_ = DEFAULT_MAX_HEADER_LIST_PAIRS; @@ -841,10 +886,10 @@ class Http2Session : public AsyncWrap, // The collection of active Http2Streams associated with this session std::unordered_map streams_; - int flags_ = SESSION_STATE_NONE; + int flags_ = kSessionStateNone; // The StreamBase instance being used for i/o - padding_strategy_type padding_strategy_ = PADDING_STRATEGY_NONE; + PaddingStrategy padding_strategy_ = PADDING_STRATEGY_NONE; // use this to allow timeout tracking during long-lasting writes uint32_t chunks_sent_since_last_write_ = 0; @@ -890,7 +935,7 @@ class Http2SessionPerformanceEntry : public performance::PerformanceEntry { Http2SessionPerformanceEntry( Http2State* http2_state, const Http2Session::Statistics& stats, - nghttp2_session_type type) : + SessionType type) : performance::PerformanceEntry( http2_state->env(), "Http2Session", "http2", stats.start_time, @@ -914,7 +959,7 @@ class Http2SessionPerformanceEntry : public performance::PerformanceEntry { int32_t stream_count() const { return stream_count_; } size_t max_concurrent_streams() const { return max_concurrent_streams_; } double stream_average_duration() const { return stream_average_duration_; } - nghttp2_session_type type() const { return session_type_; } + SessionType type() const { return session_type_; } Http2State* http2_state() const { return http2_state_.get(); } void Notify(v8::Local obj) { @@ -930,7 +975,7 @@ class Http2SessionPerformanceEntry : public performance::PerformanceEntry { int32_t stream_count_; size_t max_concurrent_streams_; double stream_average_duration_; - nghttp2_session_type session_type_; + SessionType session_type_; BaseObjectPtr http2_state_; }; @@ -979,10 +1024,7 @@ class Http2Session::Http2Ping : public AsyncWrap { public: explicit Http2Ping(Http2Session* session, v8::Local obj); - void MemoryInfo(MemoryTracker* tracker) const override { - tracker->TrackField("session", session_); - } - + SET_NO_MEMORY_INFO() SET_MEMORY_INFO_NAME(Http2Ping) SET_SELF_SIZE(Http2Ping) From a2efaa711f056657b4ac8f39b4ed72ebf9d11ae5 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Thu, 16 Apr 2020 06:50:53 -0700 Subject: [PATCH 05/18] http2: make EmitStatistics function private Signed-off-by: James M Snell --- src/node_http2.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/node_http2.h b/src/node_http2.h index ea7d07777f7d08..125b8a0d4a6a0f 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -279,8 +279,6 @@ class Http2Stream : public AsyncWrap, Http2Session* session() { return session_.get(); } const Http2Session* session() const { return session_.get(); } - void EmitStatistics(); - // Required for StreamBase int ReadStart() override; @@ -469,6 +467,8 @@ class Http2Stream : public AsyncWrap, nghttp2_headers_category category, int options); + void EmitStatistics(); + BaseObjectWeakPtr session_; // The Parent HTTP/2 Session int32_t id_ = 0; // The Stream Identifier int32_t code_ = NGHTTP2_NO_ERROR; // The RST_STREAM code (if any) @@ -575,8 +575,6 @@ class Http2Session : public AsyncWrap, class Http2Ping; class Http2Settings; - void EmitStatistics(); - StreamBase* underlying_stream() { return static_cast(stream_); } @@ -773,6 +771,8 @@ class Http2Session : public AsyncWrap, Statistics statistics_ = {}; private: + void EmitStatistics(); + // Frame Padding Strategies ssize_t OnDWordAlignedPadding(size_t frameLength, size_t maxPayloadLen); From b23cecc60618f27372ee0a85e5db60e9b9860e51 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Thu, 16 Apr 2020 06:57:32 -0700 Subject: [PATCH 06/18] http2: un-nest Http2Settings and Http2Ping Signed-off-by: James M Snell --- src/node_http2.cc | 44 ++++++++++++++++++++++---------------------- src/node_http2.h | 9 ++++----- 2 files changed, 26 insertions(+), 27 deletions(-) diff --git a/src/node_http2.cc b/src/node_http2.cc index 615462a79191b6..9b68ae2eaed9b5 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -220,7 +220,7 @@ Http2Options::Http2Options(Http2State* http2_state, SessionType type) { nghttp2_settings_entry {NGHTTP2_SETTINGS_ ## name, val}; \ } } while(0) -size_t Http2Session::Http2Settings::Init( +size_t Http2Settings::Init( Http2State* http2_state, nghttp2_settings_entry* entries) { AliasedUint32Array& buffer = http2_state->settings_buffer; @@ -239,7 +239,7 @@ size_t Http2Session::Http2Settings::Init( // The Http2Settings class is used to configure a SETTINGS frame that is // to be sent to the connected peer. The settings are set using a TypedArray // that is shared with the JavaScript side. -Http2Session::Http2Settings::Http2Settings(Http2State* http2_state, +Http2Settings::Http2Settings(Http2State* http2_state, Http2Session* session, Local obj, uint64_t start_time) @@ -252,17 +252,17 @@ Http2Session::Http2Settings::Http2Settings(Http2State* http2_state, // Generates a Buffer that contains the serialized payload of a SETTINGS // frame. This can be used, for instance, to create the Base64-encoded // content of an Http2-Settings header field. -Local Http2Session::Http2Settings::Pack() { +Local Http2Settings::Pack() { return Pack(session_->env(), count_, entries_); } -Local Http2Session::Http2Settings::Pack(Http2State* state) { +Local Http2Settings::Pack(Http2State* state) { nghttp2_settings_entry entries[IDX_SETTINGS_COUNT]; size_t count = Init(state, entries); return Pack(state->env(), count, entries); } -Local Http2Session::Http2Settings::Pack( +Local Http2Settings::Pack( Environment* env, size_t count, const nghttp2_settings_entry* entries) { @@ -281,7 +281,7 @@ Local Http2Session::Http2Settings::Pack( // Updates the shared TypedArray with the current remote or local settings for // the session. -void Http2Session::Http2Settings::Update(Http2Session* session, +void Http2Settings::Update(Http2Session* session, get_setting fn) { AliasedUint32Array& buffer = session->http2_state()->settings_buffer; @@ -293,7 +293,7 @@ void Http2Session::Http2Settings::Update(Http2Session* session, } // Initializes the shared TypedArray with the default settings values. -void Http2Session::Http2Settings::RefreshDefaults(Http2State* http2_state) { +void Http2Settings::RefreshDefaults(Http2State* http2_state) { AliasedUint32Array& buffer = http2_state->settings_buffer; uint32_t flags = 0; @@ -309,13 +309,13 @@ void Http2Session::Http2Settings::RefreshDefaults(Http2State* http2_state) { } -void Http2Session::Http2Settings::Send() { +void Http2Settings::Send() { Http2Scope h2scope(session_); CHECK_EQ(nghttp2_submit_settings(**session_, NGHTTP2_FLAG_NONE, &entries_[0], count_), 0); } -void Http2Session::Http2Settings::Done(bool ack) { +void Http2Settings::Done(bool ack) { uint64_t end = uv_hrtime(); double duration = (end - startTime_) / 1e6; @@ -2319,7 +2319,7 @@ void HttpErrorString(const FunctionCallbackInfo& args) { // output for an HTTP2-Settings header field. void PackSettings(const FunctionCallbackInfo& args) { Http2State* state = Unwrap(args.Data()); - args.GetReturnValue().Set(Http2Session::Http2Settings::Pack(state)); + args.GetReturnValue().Set(Http2Settings::Pack(state)); } // A TypedArray instance is shared between C++ and JS land to contain the @@ -2327,7 +2327,7 @@ void PackSettings(const FunctionCallbackInfo& args) { // default values. void RefreshDefaultSettings(const FunctionCallbackInfo& args) { Http2State* state = Unwrap(args.Data()); - Http2Session::Http2Settings::RefreshDefaults(state); + Http2Settings::RefreshDefaults(state); } // Sets the next stream ID the Http2Session. If successful, returns true. @@ -2781,7 +2781,7 @@ void Http2Session::Settings(const FunctionCallbackInfo& args) { args.GetReturnValue().Set(true); } -BaseObjectPtr Http2Session::PopPing() { +BaseObjectPtr Http2Session::PopPing() { BaseObjectPtr ping; if (!outstanding_pings_.empty()) { ping = std::move(outstanding_pings_.front()); @@ -2791,8 +2791,8 @@ BaseObjectPtr Http2Session::PopPing() { return ping; } -Http2Session::Http2Ping* Http2Session::AddPing( - BaseObjectPtr ping) { +Http2Ping* Http2Session::AddPing( + BaseObjectPtr ping) { if (outstanding_pings_.size() == max_outstanding_pings_) { ping->Done(false); return nullptr; @@ -2803,7 +2803,7 @@ Http2Session::Http2Ping* Http2Session::AddPing( return ptr; } -BaseObjectPtr Http2Session::PopSettings() { +BaseObjectPtr Http2Session::PopSettings() { BaseObjectPtr settings; if (!outstanding_settings_.empty()) { settings = std::move(outstanding_settings_.front()); @@ -2813,8 +2813,8 @@ BaseObjectPtr Http2Session::PopSettings() { return settings; } -Http2Session::Http2Settings* Http2Session::AddSettings( - BaseObjectPtr settings) { +Http2Settings* Http2Session::AddSettings( + BaseObjectPtr settings) { if (outstanding_settings_.size() == max_outstanding_settings_) { settings->Done(false); return nullptr; @@ -2825,13 +2825,13 @@ Http2Session::Http2Settings* Http2Session::AddSettings( return ptr; } -Http2Session::Http2Ping::Http2Ping(Http2Session* session, Local obj) +Http2Ping::Http2Ping(Http2Session* session, Local obj) : AsyncWrap(session->env(), obj, AsyncWrap::PROVIDER_HTTP2PING), session_(session), startTime_(uv_hrtime()) { } -void Http2Session::Http2Ping::Send(const uint8_t* payload) { +void Http2Ping::Send(const uint8_t* payload) { CHECK_NOT_NULL(session_); uint8_t data[8]; if (payload == nullptr) { @@ -2842,7 +2842,7 @@ void Http2Session::Http2Ping::Send(const uint8_t* payload) { CHECK_EQ(nghttp2_submit_ping(**session_, NGHTTP2_FLAG_NONE, payload), 0); } -void Http2Session::Http2Ping::Done(bool ack, const uint8_t* payload) { +void Http2Ping::Done(bool ack, const uint8_t* payload) { uint64_t duration_ns = uv_hrtime() - startTime_; double duration_ms = duration_ns / 1e6; if (session_ != nullptr) session_->statistics_.ping_rtt = duration_ns; @@ -2865,7 +2865,7 @@ void Http2Session::Http2Ping::Done(bool ack, const uint8_t* payload) { MakeCallback(env()->ondone_string(), arraysize(argv), argv); } -void Http2Session::Http2Ping::DetachFromSession() { +void Http2Ping::DetachFromSession() { session_ = nullptr; } @@ -2961,7 +2961,7 @@ void Initialize(Local target, ping->SetClassName(FIXED_ONE_BYTE_STRING(env->isolate(), "Http2Ping")); ping->Inherit(AsyncWrap::GetConstructorTemplate(env)); Local pingt = ping->InstanceTemplate(); - pingt->SetInternalFieldCount(Http2Session::Http2Ping::kInternalFieldCount); + pingt->SetInternalFieldCount(Http2Ping::kInternalFieldCount); env->set_http2ping_constructor_template(pingt); Local setting = FunctionTemplate::New(env->isolate()); diff --git a/src/node_http2.h b/src/node_http2.h index 125b8a0d4a6a0f..034dc3bcc406b9 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -159,7 +159,9 @@ struct NgHttp2StreamWrite : public MemoryRetainer { typedef uint32_t(*get_setting)(nghttp2_session* session, nghttp2_settings_id id); +class Http2Ping; class Http2Session; +class Http2Settings; class Http2Stream; // This scope should be present when any call into nghttp2 that may schedule @@ -572,9 +574,6 @@ class Http2Session : public AsyncWrap, SessionType type = NGHTTP2_SESSION_SERVER); ~Http2Session() override; - class Http2Ping; - class Http2Settings; - StreamBase* underlying_stream() { return static_cast(stream_); } @@ -1020,7 +1019,7 @@ class Http2StreamPerformanceEntry BaseObjectPtr http2_state_; }; -class Http2Session::Http2Ping : public AsyncWrap { +class Http2Ping : public AsyncWrap { public: explicit Http2Ping(Http2Session* session, v8::Local obj); @@ -1040,7 +1039,7 @@ class Http2Session::Http2Ping : public AsyncWrap { // The Http2Settings class is used to parse the settings passed in for // an Http2Session, converting those into an array of nghttp2_settings_entry // structs. -class Http2Session::Http2Settings : public AsyncWrap { +class Http2Settings : public AsyncWrap { public: Http2Settings(Http2State* http2_state, Http2Session* session, From 681bfe46ea00ba7fb491a37229b1758826ba9cb6 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Thu, 16 Apr 2020 07:54:51 -0700 Subject: [PATCH 07/18] http2: refactoring and cleanup of Http2Settings and Http2Ping Signed-off-by: James M Snell --- src/node_http2.cc | 153 +++++++++++++++++++++++----------------------- src/node_http2.h | 28 ++++++--- 2 files changed, 96 insertions(+), 85 deletions(-) diff --git a/src/node_http2.cc b/src/node_http2.cc index 9b68ae2eaed9b5..12cbe6704928eb 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -239,14 +239,19 @@ size_t Http2Settings::Init( // The Http2Settings class is used to configure a SETTINGS frame that is // to be sent to the connected peer. The settings are set using a TypedArray // that is shared with the JavaScript side. -Http2Settings::Http2Settings(Http2State* http2_state, - Http2Session* session, - Local obj, - uint64_t start_time) - : AsyncWrap(http2_state->env(), obj, PROVIDER_HTTP2SETTINGS), +Http2Settings::Http2Settings(Http2Session* session, + Local obj, + Local callback, + uint64_t start_time) + : AsyncWrap(session->env(), obj, PROVIDER_HTTP2SETTINGS), session_(session), startTime_(start_time) { - count_ = Init(http2_state, entries_); + callback_.Reset(env()->isolate(), callback); + count_ = Init(session->http2_state(), entries_); +} + +Local Http2Settings::callback() const { + return Local::New(env()->isolate(), callback_); } // Generates a Buffer that contains the serialized payload of a SETTINGS @@ -281,8 +286,7 @@ Local Http2Settings::Pack( // Updates the shared TypedArray with the current remote or local settings for // the session. -void Http2Settings::Update(Http2Session* session, - get_setting fn) { +void Http2Settings::Update(Http2Session* session, get_setting fn) { AliasedUint32Array& buffer = session->http2_state()->settings_buffer; #define V(name) \ @@ -310,7 +314,7 @@ void Http2Settings::RefreshDefaults(Http2State* http2_state) { void Http2Settings::Send() { - Http2Scope h2scope(session_); + Http2Scope h2scope(session_.get()); CHECK_EQ(nghttp2_submit_settings(**session_, NGHTTP2_FLAG_NONE, &entries_[0], count_), 0); } @@ -320,10 +324,10 @@ void Http2Settings::Done(bool ack) { double duration = (end - startTime_) / 1e6; Local argv[] = { - Boolean::New(env()->isolate(), ack), + ack ? v8::True(env()->isolate()) : v8::False(env()->isolate()), Number::New(env()->isolate(), duration) }; - MakeCallback(env()->ondone_string(), arraysize(argv), argv); + MakeCallback(callback(), arraysize(argv), argv); } // The Http2Priority class initializes an appropriate nghttp2_priority_spec @@ -2733,28 +2737,9 @@ void Http2Session::Ping(const FunctionCallbackInfo& args) { CHECK_EQ(payload.length(), 8); } - Local obj; - if (!env->http2ping_constructor_template() - ->NewInstance(env->context()) - .ToLocal(&obj)) { - return; - } - if (obj->Set(env->context(), env->ondone_string(), args[1]).IsNothing()) - return; - - Http2Ping* ping = session->AddPing( - MakeDetachedBaseObject(session, obj)); - // To prevent abuse, we strictly limit the number of unacknowledged PING - // frames that may be sent at any given time. This is configurable in the - // Options when creating a Http2Session. - if (ping == nullptr) return args.GetReturnValue().Set(false); - - // The Ping itself is an Async resource. When the acknowledgement is received, - // the callback will be invoked and a notification sent out to JS land. The - // notification will include the duration of the ping, allowing the round - // trip to be measured. - ping->Send(payload.data()); - args.GetReturnValue().Set(true); + CHECK(args[1]->IsFunction()); + args.GetReturnValue().Set( + session->AddPing(payload.data(), args[1].As())); } // Submits a SETTINGS frame for the Http2Session @@ -2762,23 +2747,8 @@ void Http2Session::Settings(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); Http2Session* session; ASSIGN_OR_RETURN_UNWRAP(&session, args.Holder()); - - Local obj; - if (!env->http2settings_constructor_template() - ->NewInstance(env->context()) - .ToLocal(&obj)) { - return; - } - if (obj->Set(env->context(), env->ondone_string(), args[0]).IsNothing()) - return; - - Http2Settings* settings = session->AddSettings( - MakeDetachedBaseObject( - session->http2_state(), session, obj, 0)); - if (settings == nullptr) return args.GetReturnValue().Set(false); - - settings->Send(); - args.GetReturnValue().Set(true); + CHECK(args[0]->IsFunction()); + args.GetReturnValue().Set(session->AddSettings(args[0].As())); } BaseObjectPtr Http2Session::PopPing() { @@ -2791,16 +2761,33 @@ BaseObjectPtr Http2Session::PopPing() { return ping; } -Http2Ping* Http2Session::AddPing( - BaseObjectPtr ping) { +bool Http2Session::AddPing(const uint8_t* payload, Local callback) { + Local obj; + if (!env()->http2ping_constructor_template() + ->NewInstance(env()->context()) + .ToLocal(&obj)) { + return false; + } + + BaseObjectPtr ping = + MakeDetachedBaseObject(this, obj, callback); + if (!ping) + return false; + if (outstanding_pings_.size() == max_outstanding_pings_) { ping->Done(false); - return nullptr; + return false; } - Http2Ping* ptr = ping.get(); - outstanding_pings_.emplace(std::move(ping)); + IncrementCurrentSessionMemory(sizeof(*ping)); - return ptr; + // The Ping itself is an Async resource. When the acknowledgement is received, + // the callback will be invoked and a notification sent out to JS land. The + // notification will include the duration of the ping, allowing the round + // trip to be measured. + ping->Send(payload); + + outstanding_pings_.push(std::move(ping)); + return true; } BaseObjectPtr Http2Session::PopSettings() { @@ -2813,60 +2800,76 @@ BaseObjectPtr Http2Session::PopSettings() { return settings; } -Http2Settings* Http2Session::AddSettings( - BaseObjectPtr settings) { - if (outstanding_settings_.size() == max_outstanding_settings_) { - settings->Done(false); - return nullptr; +bool Http2Session::AddSettings(Local callback) { + Local obj; + if (!env()->http2settings_constructor_template() + ->NewInstance(env()->context()) + .ToLocal(&obj)) { + return false; } - Http2Settings* ptr = settings.get(); - outstanding_settings_.emplace(std::move(settings)); + + BaseObjectPtr settings = + MakeDetachedBaseObject(this, obj, callback, 0); + if (!settings) + return false; + IncrementCurrentSessionMemory(sizeof(*settings)); - return ptr; + settings->Send(); + outstanding_settings_.push(std::move(settings)); + return true; } -Http2Ping::Http2Ping(Http2Session* session, Local obj) +Http2Ping::Http2Ping( + Http2Session* session, + Local obj, + Local callback) : AsyncWrap(session->env(), obj, AsyncWrap::PROVIDER_HTTP2PING), session_(session), startTime_(uv_hrtime()) { + callback_.Reset(env()->isolate(), callback); +} + +Local Http2Ping::callback() const { + return Local::New(env()->isolate(), callback_); } void Http2Ping::Send(const uint8_t* payload) { - CHECK_NOT_NULL(session_); + CHECK(session_); uint8_t data[8]; if (payload == nullptr) { memcpy(&data, &startTime_, arraysize(data)); payload = data; } - Http2Scope h2scope(session_); + Http2Scope h2scope(session_.get()); CHECK_EQ(nghttp2_submit_ping(**session_, NGHTTP2_FLAG_NONE, payload), 0); } void Http2Ping::Done(bool ack, const uint8_t* payload) { uint64_t duration_ns = uv_hrtime() - startTime_; double duration_ms = duration_ns / 1e6; - if (session_ != nullptr) session_->statistics_.ping_rtt = duration_ns; + if (session_) session_->statistics_.ping_rtt = duration_ns; - HandleScope handle_scope(env()->isolate()); + Isolate* isolate = env()->isolate(); + HandleScope handle_scope(isolate); Context::Scope context_scope(env()->context()); - Local buf = Undefined(env()->isolate()); + Local buf = Undefined(isolate); if (payload != nullptr) { - buf = Buffer::Copy(env()->isolate(), + buf = Buffer::Copy(isolate, reinterpret_cast(payload), 8).ToLocalChecked(); } Local argv[] = { - Boolean::New(env()->isolate(), ack), - Number::New(env()->isolate(), duration_ms), + ack ? v8::True(isolate) : v8::False(isolate), + Number::New(isolate, duration_ms), buf }; - MakeCallback(env()->ondone_string(), arraysize(argv), argv); + MakeCallback(callback(), arraysize(argv), argv); } void Http2Ping::DetachFromSession() { - session_ = nullptr; + session_.reset(); } void NgHttp2StreamWrite::MemoryInfo(MemoryTracker* tracker) const { diff --git a/src/node_http2.h b/src/node_http2.h index 034dc3bcc406b9..41ab7e82962400 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -718,14 +718,13 @@ class Http2Session : public AsyncWrap, return env()->event_loop(); } - Http2State* http2_state() { - return http2_state_.get(); - } + Http2State* http2_state() const { return http2_state_.get(); } + BaseObjectPtr PopPing(); - Http2Ping* AddPing(BaseObjectPtr ping); + bool AddPing(const uint8_t* data, v8::Local callback); BaseObjectPtr PopSettings(); - Http2Settings* AddSettings(BaseObjectPtr settings); + bool AddSettings(v8::Local callback); void IncrementCurrentSessionMemory(uint64_t amount) { current_session_memory_ += amount; @@ -1021,7 +1020,10 @@ class Http2StreamPerformanceEntry class Http2Ping : public AsyncWrap { public: - explicit Http2Ping(Http2Session* session, v8::Local obj); + explicit Http2Ping( + Http2Session* session, + v8::Local obj, + v8::Local callback); SET_NO_MEMORY_INFO() SET_MEMORY_INFO_NAME(Http2Ping) @@ -1031,8 +1033,11 @@ class Http2Ping : public AsyncWrap { void Done(bool ack, const uint8_t* payload = nullptr); void DetachFromSession(); + v8::Local callback() const; + private: - Http2Session* session_; + BaseObjectWeakPtr session_; + v8::Persistent callback_; uint64_t startTime_; }; @@ -1041,9 +1046,9 @@ class Http2Ping : public AsyncWrap { // structs. class Http2Settings : public AsyncWrap { public: - Http2Settings(Http2State* http2_state, - Http2Session* session, + Http2Settings(Http2Session* session, v8::Local obj, + v8::Local callback, uint64_t start_time = uv_hrtime()); SET_NO_MEMORY_INFO(); @@ -1053,6 +1058,8 @@ class Http2Settings : public AsyncWrap { void Send(); void Done(bool ack); + v8::Local callback() const; + // Returns a Buffer instance with the serialized SETTINGS payload v8::Local Pack(); @@ -1075,7 +1082,8 @@ class Http2Settings : public AsyncWrap { size_t count, const nghttp2_settings_entry* entries); - Http2Session* session_; + BaseObjectWeakPtr session_; + v8::Persistent callback_; uint64_t startTime_; size_t count_ = 0; nghttp2_settings_entry entries_[IDX_SETTINGS_COUNT]; From 0e0a3a28c0a57c8301d7e16be64d47d66ae3df1a Mon Sep 17 00:00:00 2001 From: James M Snell Date: Thu, 16 Apr 2020 08:08:12 -0700 Subject: [PATCH 08/18] http2: avoid ** syntax for readability The **session and **stream syntax for getting the underlying nghttp2 pointers is not ideal for readability Signed-off-by: James M Snell --- src/node_http2.cc | 88 ++++++++++++++++++++++++++++++----------------- src/node_http2.h | 4 ++- 2 files changed, 60 insertions(+), 32 deletions(-) diff --git a/src/node_http2.cc b/src/node_http2.cc index 12cbe6704928eb..f2f618381afaaf 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -76,7 +76,7 @@ const Http2Session::Callbacks Http2Session::callback_struct_saved[2] = { // For example: // // Http2Scope h2scope(session); -// nghttp2_submit_ping(**session, ... ); +// nghttp2_submit_ping(session->session(), ... ); // // When the Http2Scope passes out of scope and is deconstructed, it will // call Http2Session::MaybeScheduleWrite(). @@ -290,7 +290,8 @@ void Http2Settings::Update(Http2Session* session, get_setting fn) { AliasedUint32Array& buffer = session->http2_state()->settings_buffer; #define V(name) \ - buffer[IDX_SETTINGS_ ## name] = fn(**session, NGHTTP2_SETTINGS_ ## name); + buffer[IDX_SETTINGS_ ## name] = \ + fn(session->session(), NGHTTP2_SETTINGS_ ## name); HTTP2_SETTINGS(V) #undef V @@ -315,8 +316,11 @@ void Http2Settings::RefreshDefaults(Http2State* http2_state) { void Http2Settings::Send() { Http2Scope h2scope(session_.get()); - CHECK_EQ(nghttp2_submit_settings(**session_, NGHTTP2_FLAG_NONE, - &entries_[0], count_), 0); + CHECK_EQ(nghttp2_submit_settings( + session_->session(), + NGHTTP2_FLAG_NONE, + &entries_[0], + count_), 0); } void Http2Settings::Done(bool ack) { @@ -801,8 +805,11 @@ int Http2Session::OnBeginHeadersCallback(nghttp2_session* handle, session->js_fields_->max_rejected_streams) return NGHTTP2_ERR_CALLBACK_FAILURE; // Too many concurrent streams being opened - nghttp2_submit_rst_stream(**session, NGHTTP2_FLAG_NONE, id, - NGHTTP2_ENHANCE_YOUR_CALM); + nghttp2_submit_rst_stream( + session->session(), + NGHTTP2_FLAG_NONE, + id, + NGHTTP2_ENHANCE_YOUR_CALM); return NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE; } @@ -1889,8 +1896,10 @@ void Http2Stream::StartHeaders(nghttp2_headers_category category) { } -nghttp2_stream* Http2Stream::operator*() { - return nghttp2_session_find_stream(**session_, id_); +nghttp2_stream* Http2Stream::operator*() const { return stream(); } + +nghttp2_stream* Http2Stream::stream() const { + return nghttp2_session_find_stream(session_->session(), id_); } void Http2Stream::Close(int32_t code) { @@ -1913,8 +1922,9 @@ int Http2Stream::DoShutdown(ShutdownWrap* req_wrap) { { Http2Scope h2scope(this); set_not_writable(); - CHECK_NE(nghttp2_session_resume_data(**session_, id_), - NGHTTP2_ERR_NOMEM); + CHECK_NE(nghttp2_session_resume_data( + session_->session(), id_), + NGHTTP2_ERR_NOMEM); Debug(this, "writable side shutdown"); } return 1; @@ -1978,7 +1988,7 @@ int Http2Stream::SubmitResponse(const Http2Headers& headers, int options) { Http2Stream::Provider::Stream prov(this, options); int ret = nghttp2_submit_response( - **session_, + session_->session(), id_, headers.data(), headers.length(), @@ -1994,7 +2004,7 @@ int Http2Stream::SubmitInfo(const Http2Headers& headers) { Http2Scope h2scope(this); Debug(this, "sending %d informational headers", headers.length()); int ret = nghttp2_submit_headers( - **session_, + session_->session(), NGHTTP2_FLAG_NONE, id_, nullptr, @@ -2027,10 +2037,14 @@ int Http2Stream::SubmitTrailers(const Http2Headers& headers) { // to indicate that the stream is ready to be closed. if (headers.length() == 0) { Http2Stream::Provider::Stream prov(this, 0); - ret = nghttp2_submit_data(**session_, NGHTTP2_FLAG_END_STREAM, id_, *prov); + ret = nghttp2_submit_data( + session_->session(), + NGHTTP2_FLAG_END_STREAM, + id_, + *prov); } else { ret = nghttp2_submit_trailer( - **session_, + session_->session(), id_, headers.data(), headers.length()); @@ -2046,11 +2060,14 @@ int Http2Stream::SubmitPriority(nghttp2_priority_spec* prispec, Http2Scope h2scope(this); Debug(this, "sending priority spec"); int ret = silent ? - nghttp2_session_change_stream_priority(**session_, - id_, prispec) : - nghttp2_submit_priority(**session_, - NGHTTP2_FLAG_NONE, - id_, prispec); + nghttp2_session_change_stream_priority( + session_->session(), + id_, + prispec) : + nghttp2_submit_priority( + session_->session(), + NGHTTP2_FLAG_NONE, + id_, prispec); CHECK_NE(ret, NGHTTP2_ERR_NOMEM); return ret; } @@ -2076,8 +2093,11 @@ void Http2Stream::FlushRstStream() { if (is_destroyed()) return; Http2Scope h2scope(this); - CHECK_EQ(nghttp2_submit_rst_stream(**session_, NGHTTP2_FLAG_NONE, - id_, code_), 0); + CHECK_EQ(nghttp2_submit_rst_stream( + session_->session(), + NGHTTP2_FLAG_NONE, + id_, + code_), 0); } @@ -2089,7 +2109,7 @@ Http2Stream* Http2Stream::SubmitPushPromise(const Http2Headers& headers, Http2Scope h2scope(this); Debug(this, "sending push promise"); *ret = nghttp2_submit_push_promise( - **session_, + session_->session(), NGHTTP2_FLAG_NONE, id_, headers.data(), @@ -2116,9 +2136,10 @@ int Http2Stream::ReadStart() { // Tell nghttp2 about our consumption of the data that was handed // off to JS land. - nghttp2_session_consume_stream(**session_, - id_, - inbound_consumed_data_while_paused_); + nghttp2_session_consume_stream( + session_->session(), + id_, + inbound_consumed_data_while_paused_); inbound_consumed_data_while_paused_ = 0; return 0; @@ -2164,7 +2185,9 @@ int Http2Stream::DoWrite(WriteWrap* req_wrap, }); IncrementAvailableOutboundLength(bufs[i].len); } - CHECK_NE(nghttp2_session_resume_data(**session_, id_), NGHTTP2_ERR_NOMEM); + CHECK_NE(nghttp2_session_resume_data( + session_->session(), + id_), NGHTTP2_ERR_NOMEM); return 0; } @@ -2340,7 +2363,7 @@ void Http2Session::SetNextStreamID(const FunctionCallbackInfo& args) { Http2Session* session; ASSIGN_OR_RETURN_UNWRAP(&session, args.Holder()); int32_t id = args[0]->Int32Value(env->context()).ToChecked(); - if (nghttp2_session_set_next_stream_id(**session, id) < 0) { + if (nghttp2_session_set_next_stream_id(session->session(), id) < 0) { Debug(session, "failed to set next stream id to %d", id); return args.GetReturnValue().Set(false); } @@ -2369,7 +2392,7 @@ void Http2Session::RefreshState(const FunctionCallbackInfo& args) { AliasedFloat64Array& buffer = session->http2_state()->session_state_buffer; - nghttp2_session* s = **session; + nghttp2_session* s = session->session(); buffer[IDX_SESSION_STATE_EFFECTIVE_LOCAL_WINDOW_SIZE] = nghttp2_session_get_effective_local_window_size(s); @@ -2633,8 +2656,8 @@ void Http2Stream::RefreshState(const FunctionCallbackInfo& args) { AliasedFloat64Array& buffer = stream->session()->http2_state()->stream_state_buffer; - nghttp2_stream* str = **stream; - nghttp2_session* s = **(stream->session()); + nghttp2_stream* str = stream->stream(); + nghttp2_session* s = stream->session()->session(); if (str == nullptr) { buffer[IDX_STREAM_STATE] = NGHTTP2_STREAM_STATE_IDLE; @@ -2841,7 +2864,10 @@ void Http2Ping::Send(const uint8_t* payload) { payload = data; } Http2Scope h2scope(session_.get()); - CHECK_EQ(nghttp2_submit_ping(**session_, NGHTTP2_FLAG_NONE, payload), 0); + CHECK_EQ(nghttp2_submit_ping( + session_->session(), + NGHTTP2_FLAG_NONE, + payload), 0); } void Http2Ping::Done(bool ack, const uint8_t* payload) { diff --git a/src/node_http2.h b/src/node_http2.h index 41ab7e82962400..17a549688839e8 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -276,7 +276,9 @@ class Http2Stream : public AsyncWrap, int options = 0); ~Http2Stream() override; - nghttp2_stream* operator*(); + nghttp2_stream* operator*() const; + + nghttp2_stream* stream() const; Http2Session* session() { return session_.get(); } const Http2Session* session() const { return session_.get(); } From e39420f9a70ec5bec0271bc2c66a535e7b836c8b Mon Sep 17 00:00:00 2001 From: James M Snell Date: Thu, 16 Apr 2020 08:25:00 -0700 Subject: [PATCH 09/18] http2: avoid most uses to ToChecked/ToLocalChecked Signed-off-by: James M Snell --- src/node_http2.cc | 85 +++++++++++++++++++++++++---------------------- 1 file changed, 45 insertions(+), 40 deletions(-) diff --git a/src/node_http2.cc b/src/node_http2.cc index f2f618381afaaf..ee8289ed244b60 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -342,8 +342,8 @@ Http2Priority::Http2Priority(Environment* env, Local weight, Local exclusive) { Local context = env->context(); - int32_t parent_ = parent->Int32Value(context).ToChecked(); - int32_t weight_ = weight->Int32Value(context).ToChecked(); + int32_t parent_ = parent->Int32Value(context).FromMaybe(0); + int32_t weight_ = weight->Int32Value(context).FromMaybe(0); bool exclusive_ = exclusive->IsTrue(); Debug(env, DebugCategory::HTTP2STREAM, "Http2Priority: parent: %d, weight: %d, exclusive: %s\n", @@ -992,7 +992,8 @@ int Http2Session::OnStreamClose(nghttp2_session* handle, MaybeLocal answer = stream->MakeCallback(env->http2session_on_stream_close_function(), 1, &arg); - if (answer.IsEmpty() || answer.ToLocalChecked()->IsFalse()) { + Local def = v8::False(env->isolate()); + if (answer.IsEmpty() || answer.FromMaybe(def)->IsFalse()) { // Skip to destroy stream->Destroy(); } @@ -1285,17 +1286,21 @@ void Http2Session::HandleGoawayFrame(const nghttp2_frame* frame) { nghttp2_goaway goaway_frame = frame->goaway; Debug(this, "handling goaway frame"); + Local undef = Undefined(isolate); Local argv[3] = { Integer::NewFromUnsigned(isolate, goaway_frame.error_code), Integer::New(isolate, goaway_frame.last_stream_id), - Undefined(isolate) + undef }; size_t length = goaway_frame.opaque_data_len; if (length > 0) { + // If the copy fails for any reason here, we just ignore it. + // The additional goaway data is completely optional and we + // shouldn't fail if we're not able to process it. argv[2] = Buffer::Copy(isolate, reinterpret_cast(goaway_frame.opaque_data), - length).ToLocalChecked(); + length).FromMaybe(undef); } MakeCallback(env()->http2session_on_goaway_data_function(), @@ -1318,14 +1323,8 @@ void Http2Session::HandleAltSvcFrame(const nghttp2_frame* frame) { Local argv[3] = { Integer::New(isolate, id), - String::NewFromOneByte(isolate, - altsvc->origin, - NewStringType::kNormal, - altsvc->origin_len).ToLocalChecked(), - String::NewFromOneByte(isolate, - altsvc->field_value, - NewStringType::kNormal, - altsvc->field_value_len).ToLocalChecked(), + OneByteString(isolate, altsvc->origin, altsvc->origin_len), + OneByteString(isolate, altsvc->field_value, altsvc->field_value_len) }; MakeCallback(env()->http2session_on_altsvc_function(), @@ -1348,10 +1347,7 @@ void Http2Session::HandleOriginFrame(const nghttp2_frame* frame) { for (size_t i = 0; i < nov; ++i) { const nghttp2_origin_entry& entry = origin->ov[i]; - origin_v[i] = - String::NewFromOneByte( - isolate, entry.origin, NewStringType::kNormal, entry.origin_len) - .ToLocalChecked(); + origin_v[i] = OneByteString(isolate, entry.origin, entry.origin_len); } Local holder = Array::New(isolate, origin_v.data(), origin_v.size()); MakeCallback(env()->http2session_on_origin_function(), 1, &holder); @@ -1385,9 +1381,13 @@ void Http2Session::HandlePingFrame(const nghttp2_frame* frame) { if (!(js_fields_->bitfield & (1 << kSessionHasPingListeners))) return; // Notify the session that a ping occurred - arg = Buffer::Copy(env(), - reinterpret_cast(frame->ping.opaque_data), - 8).ToLocalChecked(); + Local undef = Undefined(env()->isolate()); + // There are few reasons why the copy should fail, but if it does, + // we'll just return Undefined rather than a buffer. + arg = Buffer::Copy( + env(), + reinterpret_cast(frame->ping.opaque_data), + 8).FromMaybe(undef); MakeCallback(env()->http2session_on_ping_function(), 1, &arg); } @@ -2332,12 +2332,11 @@ void Http2Stream::DecrementAvailableOutboundLength(size_t amount) { // back to JS land void HttpErrorString(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); - uint32_t val = args[0]->Uint32Value(env->context()).ToChecked(); + uint32_t val = args[0]->Uint32Value(env->context()).FromMaybe(0); args.GetReturnValue().Set( - String::NewFromOneByte( + OneByteString( env->isolate(), - reinterpret_cast(nghttp2_strerror(val)), - NewStringType::kInternalized).ToLocalChecked()); + reinterpret_cast(nghttp2_strerror(val)))); } @@ -2362,7 +2361,7 @@ void Http2Session::SetNextStreamID(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); Http2Session* session; ASSIGN_OR_RETURN_UNWRAP(&session, args.Holder()); - int32_t id = args[0]->Int32Value(env->context()).ToChecked(); + int32_t id = args[0]->Int32Value(env->context()).FromMaybe(0); if (nghttp2_session_set_next_stream_id(session->session(), id) < 0) { Debug(session, "failed to set next stream id to %d", id); return args.GetReturnValue().Set(false); @@ -2420,8 +2419,10 @@ void Http2Session::New(const FunctionCallbackInfo& args) { Http2State* state = Unwrap(args.Data()); Environment* env = state->env(); CHECK(args.IsConstructCall()); - int32_t val = args[0]->Int32Value(env->context()).ToChecked(); - SessionType type = static_cast(val); + SessionType type = + static_cast( + args[0]->Int32Value(env->context()) + .FromMaybe(NGHTTP2_SESSION_SERVER)); Http2Session* session = new Http2Session(state, args.This(), type); session->get_async_id(); // avoid compiler warning Debug(session, "session created"); @@ -2444,7 +2445,7 @@ void Http2Session::Destroy(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); Local context = env->context(); - uint32_t code = args[0]->Uint32Value(context).ToChecked(); + uint32_t code = args[0]->Uint32Value(context).FromMaybe(0); session->Close(code, args[1]->IsTrue()); } @@ -2456,7 +2457,7 @@ void Http2Session::Request(const FunctionCallbackInfo& args) { Environment* env = session->env(); Local headers = args[0].As(); - int32_t options = args[1]->Int32Value(env->context()).ToChecked(); + int32_t options = args[1]->Int32Value(env->context()).FromMaybe(0); Http2Priority priority(env, args[2], args[3], args[4]); Debug(session, "request submitted"); @@ -2506,8 +2507,8 @@ void Http2Session::Goaway(const FunctionCallbackInfo& args) { Http2Session* session; ASSIGN_OR_RETURN_UNWRAP(&session, args.Holder()); - uint32_t code = args[0]->Uint32Value(context).ToChecked(); - int32_t lastStreamID = args[1]->Int32Value(context).ToChecked(); + uint32_t code = args[0]->Uint32Value(context).FromMaybe(0); + int32_t lastStreamID = args[1]->Int32Value(context).FromMaybe(-1); ArrayBufferViewContents opaque_data; if (args[2]->IsArrayBufferView()) { @@ -2543,7 +2544,7 @@ void Http2Stream::RstStream(const FunctionCallbackInfo& args) { Local context = env->context(); Http2Stream* stream; ASSIGN_OR_RETURN_UNWRAP(&stream, args.Holder()); - uint32_t code = args[0]->Uint32Value(context).ToChecked(); + uint32_t code = args[0]->Uint32Value(context).FromMaybe(0); Debug(stream, "sending rst_stream with code %d", code); stream->SubmitRstStream(code); } @@ -2556,7 +2557,7 @@ void Http2Stream::Respond(const FunctionCallbackInfo& args) { ASSIGN_OR_RETURN_UNWRAP(&stream, args.Holder()); Local headers = args[0].As(); - int32_t options = args[1]->Int32Value(env->context()).ToChecked(); + int32_t options = args[1]->Int32Value(env->context()).FromMaybe(0); args.GetReturnValue().Set( stream->SubmitResponse( @@ -2611,7 +2612,7 @@ void Http2Stream::PushPromise(const FunctionCallbackInfo& args) { ASSIGN_OR_RETURN_UNWRAP(&parent, args.Holder()); Local headers = args[0].As(); - int32_t options = args[1]->Int32Value(env->context()).ToChecked(); + int32_t options = args[1]->Int32Value(env->context()).FromMaybe(0); Debug(parent, "creating push promise"); @@ -2706,11 +2707,16 @@ void Http2Session::AltSvc(const FunctionCallbackInfo& args) { Http2Session* session; ASSIGN_OR_RETURN_UNWRAP(&session, args.Holder()); - int32_t id = args[0]->Int32Value(env->context()).ToChecked(); + int32_t id = args[0]->Int32Value(env->context()).FromMaybe(0); // origin and value are both required to be ASCII, handle them as such. - Local origin_str = args[1]->ToString(env->context()).ToLocalChecked(); - Local value_str = args[2]->ToString(env->context()).ToLocalChecked(); + Local origin_str = + args[1]->ToString(env->context()).FromMaybe(Local()); + Local value_str = + args[2]->ToString(env->context()).FromMaybe(Local()); + + if (origin_str.IsEmpty() || value_str.IsEmpty()) + return; size_t origin_len = origin_str->Length(); size_t value_len = value_str->Length(); @@ -2735,8 +2741,7 @@ void Http2Session::Origin(const FunctionCallbackInfo& args) { ASSIGN_OR_RETURN_UNWRAP(&session, args.Holder()); Local origin_string = args[0].As(); - int32_t count = args[1]->Int32Value(context).ToChecked(); - + int32_t count = args[1]->Int32Value(context).FromMaybe(0); Origins origins(env->isolate(), env->context(), @@ -2883,7 +2888,7 @@ void Http2Ping::Done(bool ack, const uint8_t* payload) { if (payload != nullptr) { buf = Buffer::Copy(isolate, reinterpret_cast(payload), - 8).ToLocalChecked(); + 8).FromMaybe(buf); } Local argv[] = { From e8a2f5e2669e2d9f5deb924314a0eb0933dbe33d Mon Sep 17 00:00:00 2001 From: James M Snell Date: Thu, 16 Apr 2020 08:41:46 -0700 Subject: [PATCH 10/18] http2: use const references for Http2Priority Signed-off-by: James M Snell --- src/node_http2.cc | 20 +++++++++----------- src/node_http2.h | 4 ++-- 2 files changed, 11 insertions(+), 13 deletions(-) diff --git a/src/node_http2.cc b/src/node_http2.cc index ee8289ed244b60..f6a5294ca06160 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -1708,7 +1708,7 @@ int Http2Session::OnSendData( // Creates a new Http2Stream and submits a new http2 request. Http2Stream* Http2Session::SubmitRequest( - nghttp2_priority_spec* prispec, + const Http2Priority& priority, const Http2Headers& headers, int32_t* ret, int options) { @@ -1718,7 +1718,7 @@ Http2Stream* Http2Session::SubmitRequest( Http2Stream::Provider::Stream prov(options); *ret = nghttp2_submit_request( session_.get(), - prispec, + &priority, headers.data(), headers.length(), *prov, @@ -2054,7 +2054,7 @@ int Http2Stream::SubmitTrailers(const Http2Headers& headers) { } // Submit a PRIORITY frame to the connected peer. -int Http2Stream::SubmitPriority(nghttp2_priority_spec* prispec, +int Http2Stream::SubmitPriority(const Http2Priority& priority, bool silent) { CHECK(!this->is_destroyed()); Http2Scope h2scope(this); @@ -2063,11 +2063,11 @@ int Http2Stream::SubmitPriority(nghttp2_priority_spec* prispec, nghttp2_session_change_stream_priority( session_->session(), id_, - prispec) : + &priority) : nghttp2_submit_priority( session_->session(), NGHTTP2_FLAG_NONE, - id_, prispec); + id_, &priority); CHECK_NE(ret, NGHTTP2_ERR_NOMEM); return ret; } @@ -2458,14 +2458,13 @@ void Http2Session::Request(const FunctionCallbackInfo& args) { Local headers = args[0].As(); int32_t options = args[1]->Int32Value(env->context()).FromMaybe(0); - Http2Priority priority(env, args[2], args[3], args[4]); Debug(session, "request submitted"); int32_t ret = 0; Http2Stream* stream = session->Http2Session::SubmitRequest( - &priority, + Http2Priority(env, args[2], args[3], args[4]), Http2Headers(env, headers), &ret, static_cast(options)); @@ -2637,10 +2636,9 @@ void Http2Stream::Priority(const FunctionCallbackInfo& args) { Http2Stream* stream; ASSIGN_OR_RETURN_UNWRAP(&stream, args.Holder()); - Http2Priority priority(env, args[0], args[1], args[2]); - bool silent = args[3]->IsTrue(); - - CHECK_EQ(stream->SubmitPriority(&priority, silent), 0); + CHECK_EQ(stream->SubmitPriority( + Http2Priority(env, args[0], args[1], args[2]), + args[3]->IsTrue()), 0); Debug(stream, "priority submitted"); } diff --git a/src/node_http2.h b/src/node_http2.h index 17a549688839e8..1ab87932276055 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -306,7 +306,7 @@ class Http2Stream : public AsyncWrap, void OnTrailers(); // Submit a PRIORITY frame for this stream - int SubmitPriority(nghttp2_priority_spec* prispec, bool silent = false); + int SubmitPriority(const Http2Priority& priority, bool silent = false); // Submits an RST_STREAM frame using the given code void SubmitRstStream(const uint32_t code); @@ -598,7 +598,7 @@ class Http2Session : public AsyncWrap, // will be a pointer to the Http2Stream instance assigned. // This only works if the session is a client session. Http2Stream* SubmitRequest( - nghttp2_priority_spec* prispec, + const Http2Priority& priority, const Http2Headers& headers, int32_t* ret, int options = 0); From 52fcf89319168b41e4f8f58290cb28cdff62075d Mon Sep 17 00:00:00 2001 From: James M Snell Date: Thu, 16 Apr 2020 08:46:05 -0700 Subject: [PATCH 11/18] http2: remove unnecessary GetStream function Signed-off-by: James M Snell --- src/node_http2.cc | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-) diff --git a/src/node_http2.cc b/src/node_http2.cc index f6a5294ca06160..108c0d85da5c69 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -49,17 +49,6 @@ namespace { const char zero_bytes_256[256] = {}; -Http2Stream* GetStream(Http2Session* session, - int32_t id, - nghttp2_data_source* source) { - Http2Stream* stream = static_cast(source->ptr); - if (stream == nullptr) - stream = session->FindStream(id); - CHECK_NOT_NULL(stream); - CHECK_EQ(id, stream->id()); - return stream; -} - } // anonymous namespace // These configure the callbacks required by nghttp2 itself. There are @@ -1662,7 +1651,7 @@ int Http2Session::OnSendData( nghttp2_data_source* source, void* user_data) { Http2Session* session = static_cast(user_data); - Http2Stream* stream = GetStream(session, frame->hd.stream_id, source); + Http2Stream* stream = session->FindStream(frame->hd.stream_id); // Send the frame header + a byte that indicates padding length. session->CopyDataIntoOutgoing(framehd, 9); @@ -2261,7 +2250,7 @@ ssize_t Http2Stream::Provider::Stream::OnRead(nghttp2_session* handle, void* user_data) { Http2Session* session = static_cast(user_data); Debug(session, "reading outbound data for stream %d", id); - Http2Stream* stream = GetStream(session, id, source); + Http2Stream* stream = session->FindStream(id); if (stream->statistics_.first_byte_sent == 0) stream->statistics_.first_byte_sent = uv_hrtime(); CHECK_EQ(id, stream->id()); From 7aefc8f43142c344472dd65e806c51f9ee2a9685 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Thu, 16 Apr 2020 08:53:04 -0700 Subject: [PATCH 12/18] http2: refactor Http2Scope to use BaseObjectPtr Signed-off-by: James M Snell --- src/node_http2.cc | 24 ++++++++---------------- src/node_http2.h | 3 +-- 2 files changed, 9 insertions(+), 18 deletions(-) diff --git a/src/node_http2.cc b/src/node_http2.cc index 108c0d85da5c69..0fdb7755c1c078 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -71,28 +71,20 @@ const Http2Session::Callbacks Http2Session::callback_struct_saved[2] = { // call Http2Session::MaybeScheduleWrite(). Http2Scope::Http2Scope(Http2Stream* stream) : Http2Scope(stream->session()) {} -Http2Scope::Http2Scope(Http2Session* session) { - if (session == nullptr) - return; +Http2Scope::Http2Scope(Http2Session* session) : session_(session) { + if (!session_) return; - if (session->is_in_scope() | session->is_write_scheduled()) { - // There is another scope further below on the stack, or it is already - // known that a write is scheduled. In either case, there is nothing to do. + // If there is another scope further below on the stack, or + // a write is already scheduled, there's nothing to do. + if (session_->is_in_scope() || session_->is_write_scheduled()) { + session_.reset(); return; } - session->set_in_scope(); - session_ = session; - - // Always keep the session object alive for at least as long as - // this scope is active. - session_handle_ = session->object(); - CHECK(!session_handle_.IsEmpty()); + session_->set_in_scope(); } Http2Scope::~Http2Scope() { - if (session_ == nullptr) - return; - + if (!session_) return; session_->set_in_scope(false); session_->MaybeScheduleWrite(); } diff --git a/src/node_http2.h b/src/node_http2.h index 1ab87932276055..e22210bb726e92 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -174,8 +174,7 @@ class Http2Scope { ~Http2Scope(); private: - Http2Session* session_ = nullptr; - v8::Local session_handle_; + BaseObjectPtr session_; }; // The Http2Options class is used to parse the options object passed in to From d9c5a403b39c948119b85aab7dd61e53ca9e7749 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Thu, 16 Apr 2020 10:13:59 -0700 Subject: [PATCH 13/18] http2: move utility function to anonymous namespace Signed-off-by: James M Snell --- src/node_http2.cc | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/node_http2.cc b/src/node_http2.cc index 0fdb7755c1c078..bcf2f85c20a695 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -49,6 +49,11 @@ namespace { const char zero_bytes_256[256] = {}; +bool HasHttp2Observer(Environment* env) { + AliasedUint32Array& observers = env->performance_state()->observers; + return observers[performance::NODE_PERFORMANCE_ENTRY_TYPE_HTTP2] != 0; +} + } // anonymous namespace // These configure the callbacks required by nghttp2 itself. There are @@ -517,11 +522,6 @@ std::string Http2Session::diagnostic_name() const { std::to_string(static_cast(get_async_id())) + ")"; } -bool HasHttp2Observer(Environment* env) { - AliasedUint32Array& observers = env->performance_state()->observers; - return observers[performance::NODE_PERFORMANCE_ENTRY_TYPE_HTTP2] != 0; -} - void Http2Stream::EmitStatistics() { CHECK_NOT_NULL(session()); if (!HasHttp2Observer(env())) From 6b86554266d1cd4ada6ca99999654c0e4fa4d7bf Mon Sep 17 00:00:00 2001 From: James M Snell Date: Thu, 16 Apr 2020 10:30:16 -0700 Subject: [PATCH 14/18] http2: refactor and simplify Origins * Use an AllocatedBuffer instead of MaybeStackBuffer * Use a const reference instead of pointer Signed-off-by: James M Snell --- src/node_http2.cc | 38 ++++++++++++++++---------------------- src/node_http2.h | 16 ++++++++++------ 2 files changed, 26 insertions(+), 28 deletions(-) diff --git a/src/node_http2.cc b/src/node_http2.cc index bcf2f85c20a695..518ae1a3e0b8aa 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -348,34 +348,32 @@ const char* Http2Session::TypeName() const { } } -Origins::Origins(Isolate* isolate, - Local context, - Local origin_string, - size_t origin_count) : count_(origin_count) { +Origins::Origins( + Environment* env, + Local origin_string, + size_t origin_count) + : count_(origin_count) { int origin_string_len = origin_string->Length(); if (count_ == 0) { CHECK_EQ(origin_string_len, 0); return; } - // Allocate a single buffer with count_ nghttp2_nv structs, followed - // by the raw header data as passed from JS. This looks like: - // | possible padding | nghttp2_nv | nghttp2_nv | ... | header contents | - buf_.AllocateSufficientStorage((alignof(nghttp2_origin_entry) - 1) + - count_ * sizeof(nghttp2_origin_entry) + - origin_string_len); + buf_ = env->AllocateManaged((alignof(nghttp2_origin_entry) - 1) + + count_ * sizeof(nghttp2_origin_entry) + + origin_string_len); // Make sure the start address is aligned appropriately for an nghttp2_nv*. char* start = reinterpret_cast( - RoundUp(reinterpret_cast(*buf_), + RoundUp(reinterpret_cast(buf_.data()), alignof(nghttp2_origin_entry))); char* origin_contents = start + (count_ * sizeof(nghttp2_origin_entry)); nghttp2_origin_entry* const nva = reinterpret_cast(start); - CHECK_LE(origin_contents + origin_string_len, *buf_ + buf_.length()); + CHECK_LE(origin_contents + origin_string_len, buf_.data() + buf_.size()); CHECK_EQ(origin_string->WriteOneByte( - isolate, + env->isolate(), reinterpret_cast(origin_contents), 0, origin_string_len, @@ -2672,12 +2670,13 @@ void Http2Session::AltSvc(int32_t id, origin, origin_len, value, value_len), 0); } -void Http2Session::Origin(nghttp2_origin_entry* ov, size_t count) { +void Http2Session::Origin(const Origins& origins) { Http2Scope h2scope(this); CHECK_EQ(nghttp2_submit_origin( session_.get(), NGHTTP2_FLAG_NONE, - ov, count), 0); + *origins, + origins.length()), 0); } // Submits an AltSvc frame to be sent to the connected peer. @@ -2720,14 +2719,9 @@ void Http2Session::Origin(const FunctionCallbackInfo& args) { ASSIGN_OR_RETURN_UNWRAP(&session, args.Holder()); Local origin_string = args[0].As(); - int32_t count = args[1]->Int32Value(context).FromMaybe(0); - - Origins origins(env->isolate(), - env->context(), - origin_string, - static_cast(count)); + size_t count = args[1]->Int32Value(context).FromMaybe(0); - session->Origin(*origins, origins.length()); + session->Origin(Origins(env, origin_string, count)); } // Submits a PING frame to be sent to the connected peer. diff --git a/src/node_http2.h b/src/node_http2.h index e22210bb726e92..a5c700c32194df 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -163,6 +163,7 @@ class Http2Ping; class Http2Session; class Http2Settings; class Http2Stream; +class Origins; // This scope should be present when any call into nghttp2 that may schedule // data to be written to the underlying transport is made, and schedules @@ -581,15 +582,19 @@ class Http2Session : public AsyncWrap, void Close(uint32_t code = NGHTTP2_NO_ERROR, bool socket_closed = false); + void Consume(v8::Local stream); + void Goaway(uint32_t code, int32_t lastStreamID, const uint8_t* data, size_t len); + void AltSvc(int32_t id, uint8_t* origin, size_t origin_len, uint8_t* value, size_t value_len); - void Origin(nghttp2_origin_entry* ov, size_t count); + + void Origin(const Origins& origins); uint8_t SendPendingData(); @@ -1092,14 +1097,13 @@ class Http2Settings : public AsyncWrap { class Origins { public: - Origins(v8::Isolate* isolate, - v8::Local context, + Origins(Environment* env, v8::Local origin_string, size_t origin_count); ~Origins() = default; - nghttp2_origin_entry* operator*() { - return reinterpret_cast(*buf_); + const nghttp2_origin_entry* operator*() const { + return reinterpret_cast(buf_.data()); } size_t length() const { @@ -1108,7 +1112,7 @@ class Origins { private: size_t count_; - MaybeStackBuffer buf_; + AllocatedBuffer buf_; }; #define HTTP2_HIDDEN_CONSTANTS(V) \ From 4ccd69090dcd9a7adc88ccc926a6a18885001a25 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Thu, 16 Apr 2020 10:37:49 -0700 Subject: [PATCH 15/18] http2: use BaseObjectPtr for Http2Streams map Signed-off-by: James M Snell --- src/node_http2.cc | 4 ++-- src/node_http2.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/node_http2.cc b/src/node_http2.cc index 518ae1a3e0b8aa..ce62279c5827d7 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -642,7 +642,7 @@ void Http2Session::Close(uint32_t code, bool socket_closed) { // but this is faster and does not fail if the stream is not found. Http2Stream* Http2Session::FindStream(int32_t id) { auto s = streams_.find(id); - return s != streams_.end() ? s->second : nullptr; + return s != streams_.end() ? s->second.get() : nullptr; } bool Http2Session::CanAddStream() { @@ -659,7 +659,7 @@ bool Http2Session::CanAddStream() { void Http2Session::AddStream(Http2Stream* stream) { CHECK_GE(++statistics_.stream_count, 0); - streams_[stream->id()] = stream; + streams_[stream->id()] = BaseObjectPtr(stream); size_t size = streams_.size(); if (size > statistics_.max_concurrent_streams) statistics_.max_concurrent_streams = size; diff --git a/src/node_http2.h b/src/node_http2.h index a5c700c32194df..e5345c74d6d8c1 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -888,7 +888,7 @@ class Http2Session : public AsyncWrap, uint64_t current_nghttp2_memory_ = 0; // The collection of active Http2Streams associated with this session - std::unordered_map streams_; + std::unordered_map> streams_; int flags_ = kSessionStateNone; From fe77e2300ec3c515e76a13b7d07993b08ee9ec11 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Thu, 16 Apr 2020 10:41:44 -0700 Subject: [PATCH 16/18] http2: move MemoryInfo impl to cc Signed-off-by: James M Snell --- src/node_http2.cc | 17 +++++++++++++++++ src/node_http2.h | 19 ++----------------- 2 files changed, 19 insertions(+), 17 deletions(-) diff --git a/src/node_http2.cc b/src/node_http2.cc index ce62279c5827d7..178a3242ba378e 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -515,6 +515,18 @@ Http2Session::~Http2Session() { CHECK_EQ(current_nghttp2_memory_, 0); } +void Http2Session::MemoryInfo(MemoryTracker* tracker) const { + tracker->TrackField("streams", streams_); + tracker->TrackField("outstanding_pings", outstanding_pings_); + tracker->TrackField("outstanding_settings", outstanding_settings_); + tracker->TrackField("outgoing_buffers", outgoing_buffers_); + tracker->TrackFieldWithSize("stream_buf", stream_buf_.len); + tracker->TrackFieldWithSize("outgoing_storage", outgoing_storage_.size()); + tracker->TrackFieldWithSize("pending_rst_streams", + pending_rst_streams_.size() * sizeof(int32_t)); + tracker->TrackFieldWithSize("nghttp2_memory", current_nghttp2_memory_); +} + std::string Http2Session::diagnostic_name() const { return std::string("Http2Session ") + TypeName() + " (" + std::to_string(static_cast(get_async_id())) + ")"; @@ -1858,6 +1870,11 @@ Http2Stream::~Http2Stream() { session_->RemoveStream(this); } +void Http2Stream::MemoryInfo(MemoryTracker* tracker) const { + tracker->TrackField("current_headers", current_headers_); + tracker->TrackField("queue", queue_); +} + std::string Http2Stream::diagnostic_name() const { return "HttpStream " + std::to_string(id()) + " (" + std::to_string(static_cast(get_async_id())) + ") [" + diff --git a/src/node_http2.h b/src/node_http2.h index e5345c74d6d8c1..054d4255c30bcc 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -429,11 +429,7 @@ class Http2Stream : public AsyncWrap, int DoWrite(WriteWrap* w, uv_buf_t* bufs, size_t count, uv_stream_t* send_handle) override; - void MemoryInfo(MemoryTracker* tracker) const override { - tracker->TrackField("current_headers", current_headers_); - tracker->TrackField("queue", queue_); - } - + void MemoryInfo(MemoryTracker* tracker) const override; SET_MEMORY_INFO_NAME(Http2Stream) SET_SELF_SIZE(Http2Stream) @@ -665,18 +661,7 @@ class Http2Session : public AsyncWrap, // Write data from stream_buf_ to the session ssize_t ConsumeHTTP2Data(); - void MemoryInfo(MemoryTracker* tracker) const override { - tracker->TrackField("streams", streams_); - tracker->TrackField("outstanding_pings", outstanding_pings_); - tracker->TrackField("outstanding_settings", outstanding_settings_); - tracker->TrackField("outgoing_buffers", outgoing_buffers_); - tracker->TrackFieldWithSize("stream_buf", stream_buf_.len); - tracker->TrackFieldWithSize("outgoing_storage", outgoing_storage_.size()); - tracker->TrackFieldWithSize("pending_rst_streams", - pending_rst_streams_.size() * sizeof(int32_t)); - tracker->TrackFieldWithSize("nghttp2_memory", current_nghttp2_memory_); - } - + void MemoryInfo(MemoryTracker* tracker) const override; SET_MEMORY_INFO_NAME(Http2Session) SET_SELF_SIZE(Http2Session) From 764c71d599f3766baf8d21507c402527e2aa19bb Mon Sep 17 00:00:00 2001 From: James M Snell Date: Thu, 16 Apr 2020 10:47:40 -0700 Subject: [PATCH 17/18] http2: fixup lint and style issues Signed-off-by: James M Snell --- src/node_http2.cc | 8 +++----- src/node_http2.h | 19 +++++++++++-------- 2 files changed, 14 insertions(+), 13 deletions(-) diff --git a/src/node_http2.cc b/src/node_http2.cc index 178a3242ba378e..19419ac0ae410b 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -31,7 +31,6 @@ using v8::Integer; using v8::Isolate; using v8::Local; using v8::MaybeLocal; -using v8::NewStringType; using v8::Number; using v8::Object; using v8::ObjectTemplate; @@ -204,7 +203,7 @@ Http2Options::Http2Options(Http2State* http2_state, SessionType type) { uint32_t val = buffer[IDX_SETTINGS_ ## name]; \ entries[count++] = \ nghttp2_settings_entry {NGHTTP2_SETTINGS_ ## name, val}; \ - } } while(0) + } } while (0) size_t Http2Settings::Init( Http2State* http2_state, @@ -280,7 +279,6 @@ void Http2Settings::Update(Http2Session* session, get_setting fn) { fn(session->session(), NGHTTP2_SETTINGS_ ## name); HTTP2_SETTINGS(V) #undef V - } // Initializes the shared TypedArray with the default settings values. @@ -1933,7 +1931,7 @@ void Http2Stream::Destroy() { // Do nothing if this stream instance is already destroyed if (is_destroyed()) return; - if (session_->HasPendingRstStream(id_)) + if (session_->has_pending_rststream(id_)) FlushRstStream(); set_destroyed(); @@ -3050,7 +3048,7 @@ void Initialize(Local target, #define V(name) FIXED_ONE_BYTE_STRING(isolate, #name), Local error_code_names[] = { HTTP2_ERROR_CODES(V) - Local() // Unused. + Local() // Unused. }; #undef V diff --git a/src/node_http2.h b/src/node_http2.h index 054d4255c30bcc..dabaec15e84f81 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -624,8 +624,10 @@ class Http2Session : public AsyncWrap, #define IS_FLAG(name, flag) \ bool is_##name() const { return flags_ & flag; } \ void set_##name(bool on = true) { \ - if (on) flags_ |= flag; \ - else flags_ &= ~flag; \ + if (on) \ + flags_ |= flag; \ + else \ + flags_ &= ~flag; \ } IS_FLAG(in_scope, kSessionStateHasScope) @@ -672,10 +674,11 @@ class Http2Session : public AsyncWrap, pending_rst_streams_.emplace_back(stream_id); } - bool HasPendingRstStream(int32_t stream_id) { - return pending_rst_streams_.end() != std::find(pending_rst_streams_.begin(), - pending_rst_streams_.end(), - stream_id); + bool has_pending_rststream(int32_t stream_id) { + return pending_rst_streams_.end() != + std::find(pending_rst_streams_.begin(), + pending_rst_streams_.end(), + stream_id); } // Handle reads/writes from the underlying network transport. @@ -1028,7 +1031,7 @@ class Http2Ping : public AsyncWrap { private: BaseObjectWeakPtr session_; - v8::Persistent callback_; + v8::Global callback_; uint64_t startTime_; }; @@ -1074,7 +1077,7 @@ class Http2Settings : public AsyncWrap { const nghttp2_settings_entry* entries); BaseObjectWeakPtr session_; - v8::Persistent callback_; + v8::Global callback_; uint64_t startTime_; size_t count_ = 0; nghttp2_settings_entry entries_[IDX_SETTINGS_COUNT]; From 1342b243aa1ebfda57e0267a5d09379f9ce6e5d7 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Thu, 16 Apr 2020 12:33:29 -0700 Subject: [PATCH 18/18] [Squash] fixup --- src/node_http2.cc | 187 ++++++++++-------- src/node_http2.h | 8 +- test/parallel/test-http2-getpackedsettings.js | 11 +- 3 files changed, 113 insertions(+), 93 deletions(-) diff --git a/src/node_http2.cc b/src/node_http2.cc index 19419ac0ae410b..385a2352040c4e 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -236,7 +236,11 @@ Http2Settings::Http2Settings(Http2Session* session, } Local Http2Settings::callback() const { - return Local::New(env()->isolate(), callback_); + return callback_.Get(env()->isolate()); +} + +void Http2Settings::MemoryInfo(MemoryTracker* tracker) const { + tracker->TrackField("callback", callback_); } // Generates a Buffer that contains the serialized payload of a SETTINGS @@ -265,8 +269,9 @@ Local Http2Settings::Pack( size, entries, count); - Local undef = Undefined(env->isolate()); - return scope.Escape(ret >= 0 ? buffer.ToBuffer().FromMaybe(undef) : undef); + Local buf = Undefined(env->isolate()); + if (ret >= 0) buf = buffer.ToBuffer().ToLocalChecked(); + return scope.Escape(buf); } // Updates the shared TypedArray with the current remote or local settings for @@ -326,8 +331,8 @@ Http2Priority::Http2Priority(Environment* env, Local weight, Local exclusive) { Local context = env->context(); - int32_t parent_ = parent->Int32Value(context).FromMaybe(0); - int32_t weight_ = weight->Int32Value(context).FromMaybe(0); + int32_t parent_ = parent->Int32Value(context).ToChecked(); + int32_t weight_ = weight->Int32Value(context).ToChecked(); bool exclusive_ = exclusive->IsTrue(); Debug(env, DebugCategory::HTTP2STREAM, "Http2Priority: parent: %d, weight: %d, exclusive: %s\n", @@ -650,9 +655,9 @@ void Http2Session::Close(uint32_t code, bool socket_closed) { // Locates an existing known stream by ID. nghttp2 has a similar method // but this is faster and does not fail if the stream is not found. -Http2Stream* Http2Session::FindStream(int32_t id) { +BaseObjectPtr Http2Session::FindStream(int32_t id) { auto s = streams_.find(id); - return s != streams_.end() ? s->second.get() : nullptr; + return s != streams_.end() ? s->second : BaseObjectPtr(); } bool Http2Session::CanAddStream() { @@ -677,11 +682,16 @@ void Http2Session::AddStream(Http2Stream* stream) { } -void Http2Session::RemoveStream(Http2Stream* stream) { - if (streams_.empty() || stream == nullptr) - return; // Nothing to remove, item was never added? - streams_.erase(stream->id()); - DecrementCurrentSessionMemory(sizeof(*stream)); +BaseObjectPtr Http2Session::RemoveStream(int32_t id) { + BaseObjectPtr stream; + if (streams_.empty()) + return stream; + stream = FindStream(id); + if (stream) { + streams_.erase(id); + DecrementCurrentSessionMemory(sizeof(*stream)); + } + return stream; } // Used as one of the Padding Strategy functions. Will attempt to ensure @@ -783,10 +793,10 @@ int Http2Session::OnBeginHeadersCallback(nghttp2_session* handle, int32_t id = GetFrameID(frame); Debug(session, "beginning headers for stream %d", id); - Http2Stream* stream = session->FindStream(id); + BaseObjectPtr stream = session->FindStream(id); // The common case is that we're creating a new stream. The less likely // case is that we're receiving a set of trailers - if (LIKELY(stream == nullptr)) { + if (LIKELY(!stream)) { if (UNLIKELY(!session->CanAddStream() || Http2Stream::New(session, id, frame->headers.cat) == nullptr)) { @@ -820,11 +830,11 @@ int Http2Session::OnHeaderCallback(nghttp2_session* handle, void* user_data) { Http2Session* session = static_cast(user_data); int32_t id = GetFrameID(frame); - Http2Stream* stream = session->FindStream(id); + BaseObjectPtr stream = session->FindStream(id); // If stream is null at this point, either something odd has happened // or the stream was closed locally while header processing was occurring. // either way, do not proceed and close the stream. - if (UNLIKELY(stream == nullptr)) + if (UNLIKELY(!stream)) return NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE; // If the stream has already been destroyed, ignore. @@ -966,10 +976,10 @@ int Http2Session::OnStreamClose(nghttp2_session* handle, Local context = env->context(); Context::Scope context_scope(context); Debug(session, "stream %d closed with code: %d", id, code); - Http2Stream* stream = session->FindStream(id); + BaseObjectPtr stream = session->FindStream(id); // Intentionally ignore the callback if the stream does not exist or has // already been destroyed - if (stream == nullptr || stream->is_destroyed()) + if (!stream || stream->is_destroyed()) return 0; stream->Close(code); @@ -982,7 +992,7 @@ int Http2Session::OnStreamClose(nghttp2_session* handle, stream->MakeCallback(env->http2session_on_stream_close_function(), 1, &arg); Local def = v8::False(env->isolate()); - if (answer.IsEmpty() || answer.FromMaybe(def)->IsFalse()) { + if (answer.IsEmpty() || answer.ToLocalChecked()->IsFalse()) { // Skip to destroy stream->Destroy(); } @@ -1028,9 +1038,10 @@ int Http2Session::OnDataChunkReceived(nghttp2_session* handle, // so that it can send a WINDOW_UPDATE frame. This is a critical part of // the flow control process in http2 CHECK_EQ(nghttp2_session_consume_connection(handle, len), 0); - Http2Stream* stream = session->FindStream(id); + BaseObjectPtr stream = session->FindStream(id); + // If the stream has been destroyed, ignore this chunk - if (stream->is_destroyed()) + if (!stream || stream->is_destroyed()) return 0; stream->statistics_.received_bytes += len; @@ -1077,7 +1088,7 @@ int Http2Session::OnDataChunkReceived(nghttp2_session* handle, // If we are currently waiting for a write operation to finish, we should // tell nghttp2 that we want to wait before we process more input data. if (session->is_write_in_progress()) { - CHECK(!session->is_reading_stopped()); + CHECK(session->is_reading_stopped()); session->set_receive_paused(); return NGHTTP2_ERR_PAUSE; } @@ -1185,10 +1196,10 @@ void Http2Session::HandleHeadersFrame(const nghttp2_frame* frame) { int32_t id = GetFrameID(frame); Debug(this, "handle headers frame for stream %d", id); - Http2Stream* stream = FindStream(id); + BaseObjectPtr stream = FindStream(id); // If the stream has already been destroyed, ignore. - if (stream->is_destroyed()) + if (!stream || stream->is_destroyed()) return; // The headers are stored as a vector of Http2Header instances. @@ -1254,9 +1265,11 @@ void Http2Session::HandlePriorityFrame(const nghttp2_frame* frame) { int Http2Session::HandleDataFrame(const nghttp2_frame* frame) { int32_t id = GetFrameID(frame); Debug(this, "handling data frame for stream %d", id); - Http2Stream* stream = FindStream(id); + BaseObjectPtr stream = FindStream(id); - if (!stream->is_destroyed() && frame->hd.flags & NGHTTP2_FLAG_END_STREAM) { + if (stream && + !stream->is_destroyed() && + frame->hd.flags & NGHTTP2_FLAG_END_STREAM) { stream->EmitRead(UV_EOF); } else if (frame->hd.length == 0) { return 1; // Consider 0-length frame without END_STREAM an error. @@ -1275,11 +1288,10 @@ void Http2Session::HandleGoawayFrame(const nghttp2_frame* frame) { nghttp2_goaway goaway_frame = frame->goaway; Debug(this, "handling goaway frame"); - Local undef = Undefined(isolate); Local argv[3] = { Integer::NewFromUnsigned(isolate, goaway_frame.error_code), Integer::New(isolate, goaway_frame.last_stream_id), - undef + Undefined(isolate) }; size_t length = goaway_frame.opaque_data_len; @@ -1289,7 +1301,7 @@ void Http2Session::HandleGoawayFrame(const nghttp2_frame* frame) { // shouldn't fail if we're not able to process it. argv[2] = Buffer::Copy(isolate, reinterpret_cast(goaway_frame.opaque_data), - length).FromMaybe(undef); + length).ToLocalChecked(); } MakeCallback(env()->http2session_on_goaway_data_function(), @@ -1370,13 +1382,10 @@ void Http2Session::HandlePingFrame(const nghttp2_frame* frame) { if (!(js_fields_->bitfield & (1 << kSessionHasPingListeners))) return; // Notify the session that a ping occurred - Local undef = Undefined(env()->isolate()); - // There are few reasons why the copy should fail, but if it does, - // we'll just return Undefined rather than a buffer. arg = Buffer::Copy( env(), reinterpret_cast(frame->ping.opaque_data), - 8).FromMaybe(undef); + 8).ToLocalChecked(); MakeCallback(env()->http2session_on_ping_function(), 1, &arg); } @@ -1429,7 +1438,7 @@ void Http2Session::OnStreamAfterWrite(WriteWrap* w, int status) { if (is_reading_stopped() && !is_write_in_progress() && nghttp2_session_want_read(session_.get())) { - set_reading_stopped(); + set_reading_stopped(false); stream_->ReadStart(); } @@ -1524,8 +1533,8 @@ void Http2Session::ClearOutgoing(int status) { SendPendingData(); for (int32_t stream_id : current_pending_rst_streams) { - Http2Stream* stream = FindStream(stream_id); - if (LIKELY(stream != nullptr)) + BaseObjectPtr stream = FindStream(stream_id); + if (LIKELY(stream)) stream->FlushRstStream(); } } @@ -1651,7 +1660,8 @@ int Http2Session::OnSendData( nghttp2_data_source* source, void* user_data) { Http2Session* session = static_cast(user_data); - Http2Stream* stream = session->FindStream(frame->hd.stream_id); + BaseObjectPtr stream = session->FindStream(frame->hd.stream_id); + if (!stream) return 0; // Send the frame header + a byte that indicates padding length. session->CopyDataIntoOutgoing(framehd, 9); @@ -1758,6 +1768,9 @@ void Http2Session::OnStreamRead(ssize_t nread, const uv_buf_t& buf_) { // The data in stream_buf_ is already accounted for, add nread received // bytes to session memory but remove the already processed // stream_buf_offset_ bytes. + // TODO(@jasnell): There are some cases where nread is < stream_buf_offset_ + // here but things still work. Those need to be investigated. + // CHECK_GE(nread, stream_buf_offset_); IncrementCurrentSessionMemory(nread - stream_buf_offset_); buf = std::move(new_buf); @@ -1861,11 +1874,7 @@ Http2Stream::Http2Stream(Http2Session* session, } Http2Stream::~Http2Stream() { - if (!session_) - return; Debug(this, "tearing down stream"); - session_->DecrementCurrentSessionMemory(current_headers_length_); - session_->RemoveStream(this); } void Http2Stream::MemoryInfo(MemoryTracker* tracker) const { @@ -1939,26 +1948,30 @@ void Http2Stream::Destroy() { // Wait until the start of the next loop to delete because there // may still be some pending operations queued for this stream. - BaseObjectPtr strong_ref{this}; - env()->SetImmediate([this, strong_ref](Environment* env) { - // Free any remaining outgoing data chunks here. This should be done - // here because it's possible for destroy to have been called while - // we still have queued outbound writes. - while (!queue_.empty()) { - NgHttp2StreamWrite& head = queue_.front(); - if (head.req_wrap != nullptr) - head.req_wrap->Done(UV_ECANCELED); - queue_.pop(); - } + BaseObjectPtr strong_ref = session_->RemoveStream(id_); + if (strong_ref) { + env()->SetImmediate([this, strong_ref = std::move(strong_ref)]( + Environment* env) { + // Free any remaining outgoing data chunks here. This should be done + // here because it's possible for destroy to have been called while + // we still have queued outbound writes. + while (!queue_.empty()) { + NgHttp2StreamWrite& head = queue_.front(); + if (head.req_wrap != nullptr) + head.req_wrap->Done(UV_ECANCELED); + queue_.pop(); + } - // We can destroy the stream now if there are no writes for it - // already on the socket. Otherwise, we'll wait for the garbage collector - // to take care of cleaning up. - if (session() == nullptr || !session()->HasWritesOnSocketForStream(this)) { - // Delete once strong_ref goes out of scope. - Detach(); - } - }); + // We can destroy the stream now if there are no writes for it + // already on the socket. Otherwise, we'll wait for the garbage collector + // to take care of cleaning up. + if (session() == nullptr || + !session()->HasWritesOnSocketForStream(this)) { + // Delete once strong_ref goes out of scope. + Detach(); + } + }); + } statistics_.end_time = uv_hrtime(); session_->statistics_.stream_average_duration = @@ -2255,7 +2268,8 @@ ssize_t Http2Stream::Provider::Stream::OnRead(nghttp2_session* handle, void* user_data) { Http2Session* session = static_cast(user_data); Debug(session, "reading outbound data for stream %d", id); - Http2Stream* stream = session->FindStream(id); + BaseObjectPtr stream = session->FindStream(id); + if (!stream) return 0; if (stream->statistics_.first_byte_sent == 0) stream->statistics_.first_byte_sent = uv_hrtime(); CHECK_EQ(id, stream->id()); @@ -2326,7 +2340,7 @@ void Http2Stream::DecrementAvailableOutboundLength(size_t amount) { // back to JS land void HttpErrorString(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); - uint32_t val = args[0]->Uint32Value(env->context()).FromMaybe(0); + uint32_t val = args[0]->Uint32Value(env->context()).ToChecked(); args.GetReturnValue().Set( OneByteString( env->isolate(), @@ -2355,7 +2369,7 @@ void Http2Session::SetNextStreamID(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); Http2Session* session; ASSIGN_OR_RETURN_UNWRAP(&session, args.Holder()); - int32_t id = args[0]->Int32Value(env->context()).FromMaybe(0); + int32_t id = args[0]->Int32Value(env->context()).ToChecked(); if (nghttp2_session_set_next_stream_id(session->session(), id) < 0) { Debug(session, "failed to set next stream id to %d", id); return args.GetReturnValue().Set(false); @@ -2415,8 +2429,7 @@ void Http2Session::New(const FunctionCallbackInfo& args) { CHECK(args.IsConstructCall()); SessionType type = static_cast( - args[0]->Int32Value(env->context()) - .FromMaybe(NGHTTP2_SESSION_SERVER)); + args[0]->Int32Value(env->context()).ToChecked()); Http2Session* session = new Http2Session(state, args.This(), type); session->get_async_id(); // avoid compiler warning Debug(session, "session created"); @@ -2439,7 +2452,7 @@ void Http2Session::Destroy(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); Local context = env->context(); - uint32_t code = args[0]->Uint32Value(context).FromMaybe(0); + uint32_t code = args[0]->Uint32Value(context).ToChecked(); session->Close(code, args[1]->IsTrue()); } @@ -2451,7 +2464,7 @@ void Http2Session::Request(const FunctionCallbackInfo& args) { Environment* env = session->env(); Local headers = args[0].As(); - int32_t options = args[1]->Int32Value(env->context()).FromMaybe(0); + int32_t options = args[1]->Int32Value(env->context()).ToChecked(); Debug(session, "request submitted"); @@ -2500,8 +2513,8 @@ void Http2Session::Goaway(const FunctionCallbackInfo& args) { Http2Session* session; ASSIGN_OR_RETURN_UNWRAP(&session, args.Holder()); - uint32_t code = args[0]->Uint32Value(context).FromMaybe(0); - int32_t lastStreamID = args[1]->Int32Value(context).FromMaybe(-1); + uint32_t code = args[0]->Uint32Value(context).ToChecked(); + int32_t lastStreamID = args[1]->Int32Value(context).ToChecked(); ArrayBufferViewContents opaque_data; if (args[2]->IsArrayBufferView()) { @@ -2537,7 +2550,7 @@ void Http2Stream::RstStream(const FunctionCallbackInfo& args) { Local context = env->context(); Http2Stream* stream; ASSIGN_OR_RETURN_UNWRAP(&stream, args.Holder()); - uint32_t code = args[0]->Uint32Value(context).FromMaybe(0); + uint32_t code = args[0]->Uint32Value(context).ToChecked(); Debug(stream, "sending rst_stream with code %d", code); stream->SubmitRstStream(code); } @@ -2550,7 +2563,7 @@ void Http2Stream::Respond(const FunctionCallbackInfo& args) { ASSIGN_OR_RETURN_UNWRAP(&stream, args.Holder()); Local headers = args[0].As(); - int32_t options = args[1]->Int32Value(env->context()).FromMaybe(0); + int32_t options = args[1]->Int32Value(env->context()).ToChecked(); args.GetReturnValue().Set( stream->SubmitResponse( @@ -2605,7 +2618,7 @@ void Http2Stream::PushPromise(const FunctionCallbackInfo& args) { ASSIGN_OR_RETURN_UNWRAP(&parent, args.Holder()); Local headers = args[0].As(); - int32_t options = args[1]->Int32Value(env->context()).FromMaybe(0); + int32_t options = args[1]->Int32Value(env->context()).ToChecked(); Debug(parent, "creating push promise"); @@ -2700,13 +2713,11 @@ void Http2Session::AltSvc(const FunctionCallbackInfo& args) { Http2Session* session; ASSIGN_OR_RETURN_UNWRAP(&session, args.Holder()); - int32_t id = args[0]->Int32Value(env->context()).FromMaybe(0); + int32_t id = args[0]->Int32Value(env->context()).ToChecked(); // origin and value are both required to be ASCII, handle them as such. - Local origin_str = - args[1]->ToString(env->context()).FromMaybe(Local()); - Local value_str = - args[2]->ToString(env->context()).FromMaybe(Local()); + Local origin_str = args[1]->ToString(env->context()).ToLocalChecked(); + Local value_str = args[2]->ToString(env->context()).ToLocalChecked(); if (origin_str.IsEmpty() || value_str.IsEmpty()) return; @@ -2734,7 +2745,7 @@ void Http2Session::Origin(const FunctionCallbackInfo& args) { ASSIGN_OR_RETURN_UNWRAP(&session, args.Holder()); Local origin_string = args[0].As(); - size_t count = args[1]->Int32Value(context).FromMaybe(0); + size_t count = args[1]->Int32Value(context).ToChecked(); session->Origin(Origins(env, origin_string, count)); } @@ -2802,7 +2813,7 @@ bool Http2Session::AddPing(const uint8_t* payload, Local callback) { // trip to be measured. ping->Send(payload); - outstanding_pings_.push(std::move(ping)); + outstanding_pings_.emplace(std::move(ping)); return true; } @@ -2829,9 +2840,14 @@ bool Http2Session::AddSettings(Local callback) { if (!settings) return false; + if (outstanding_settings_.size() == max_outstanding_settings_) { + settings->Done(false); + return false; + } + IncrementCurrentSessionMemory(sizeof(*settings)); settings->Send(); - outstanding_settings_.push(std::move(settings)); + outstanding_settings_.emplace(std::move(settings)); return true; } @@ -2845,8 +2861,12 @@ Http2Ping::Http2Ping( callback_.Reset(env()->isolate(), callback); } +void Http2Ping::MemoryInfo(MemoryTracker* tracker) const { + tracker->TrackField("callback", callback_); +} + Local Http2Ping::callback() const { - return Local::New(env()->isolate(), callback_); + return callback_.Get(env()->isolate()); } void Http2Ping::Send(const uint8_t* payload) { @@ -2876,7 +2896,7 @@ void Http2Ping::Done(bool ack, const uint8_t* payload) { if (payload != nullptr) { buf = Buffer::Copy(isolate, reinterpret_cast(payload), - 8).FromMaybe(buf); + 8).ToLocalChecked(); } Local argv[] = { @@ -3048,7 +3068,6 @@ void Initialize(Local target, #define V(name) FIXED_ONE_BYTE_STRING(isolate, #name), Local error_code_names[] = { HTTP2_ERROR_CODES(V) - Local() // Unused. }; #undef V @@ -3056,7 +3075,7 @@ void Initialize(Local target, Array::New( isolate, error_code_names, - arraysize(error_code_names) - 1); + arraysize(error_code_names)); target->Set(context, FIXED_ONE_BYTE_STRING(isolate, "nameForErrorCode"), diff --git a/src/node_http2.h b/src/node_http2.h index dabaec15e84f81..6b11535f84e121 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -647,7 +647,7 @@ class Http2Session : public AsyncWrap, void MaybeStopReading(); // Returns pointer to the stream, or nullptr if stream does not exist - Http2Stream* FindStream(int32_t id); + BaseObjectPtr FindStream(int32_t id); bool CanAddStream(); @@ -655,7 +655,7 @@ class Http2Session : public AsyncWrap, void AddStream(Http2Stream* stream); // Removes a stream instance from this session - void RemoveStream(Http2Stream* stream); + BaseObjectPtr RemoveStream(int32_t id); // Indicates whether there currently exist outgoing buffers for this stream. bool HasWritesOnSocketForStream(Http2Stream* stream); @@ -1019,7 +1019,7 @@ class Http2Ping : public AsyncWrap { v8::Local obj, v8::Local callback); - SET_NO_MEMORY_INFO() + void MemoryInfo(MemoryTracker* tracker) const override; SET_MEMORY_INFO_NAME(Http2Ping) SET_SELF_SIZE(Http2Ping) @@ -1045,7 +1045,7 @@ class Http2Settings : public AsyncWrap { v8::Local callback, uint64_t start_time = uv_hrtime()); - SET_NO_MEMORY_INFO(); + void MemoryInfo(MemoryTracker* tracker) const override; SET_MEMORY_INFO_NAME(Http2Settings) SET_SELF_SIZE(Http2Settings) diff --git a/test/parallel/test-http2-getpackedsettings.js b/test/parallel/test-http2-getpackedsettings.js index 4aa5747a053bd1..a54ab4499e1f89 100644 --- a/test/parallel/test-http2-getpackedsettings.js +++ b/test/parallel/test-http2-getpackedsettings.js @@ -7,11 +7,11 @@ const assert = require('assert'); const http2 = require('http2'); const check = Buffer.from([0x00, 0x01, 0x00, 0x00, 0x10, 0x00, + 0x00, 0x02, 0x00, 0x00, 0x00, 0x01, 0x00, 0x03, 0xff, 0xff, 0xff, 0xff, - 0x00, 0x05, 0x00, 0x00, 0x40, 0x00, 0x00, 0x04, 0x00, 0x00, 0xff, 0xff, + 0x00, 0x05, 0x00, 0x00, 0x40, 0x00, 0x00, 0x06, 0x00, 0x00, 0xff, 0xff, - 0x00, 0x02, 0x00, 0x00, 0x00, 0x01, 0x00, 0x08, 0x00, 0x00, 0x00, 0x00]); const val = http2.getPackedSettings(http2.getDefaultSettings()); assert.deepStrictEqual(val, check); @@ -83,12 +83,13 @@ http2.getPackedSettings({ enablePush: false }); { const check = Buffer.from([ 0x00, 0x01, 0x00, 0x00, 0x00, 0x64, + 0x00, 0x02, 0x00, 0x00, 0x00, 0x01, 0x00, 0x03, 0x00, 0x00, 0x00, 0xc8, - 0x00, 0x05, 0x00, 0x00, 0x4e, 0x20, 0x00, 0x04, 0x00, 0x00, 0x00, 0x64, + 0x00, 0x05, 0x00, 0x00, 0x4e, 0x20, 0x00, 0x06, 0x00, 0x00, 0x00, 0x64, - 0x00, 0x02, 0x00, 0x00, 0x00, 0x01, - 0x00, 0x08, 0x00, 0x00, 0x00, 0x00]); + 0x00, 0x08, 0x00, 0x00, 0x00, 0x00 + ]); const packed = http2.getPackedSettings({ headerTableSize: 100,