From 9c59d2de41ea9b6c34e66b9cddfa695865683e2d Mon Sep 17 00:00:00 2001 From: James M Snell Date: Sun, 29 Mar 2020 09:06:57 -0700 Subject: [PATCH 1/3] src: use smart pointers for nghttp2 objects Signed-off-by: James M Snell --- src/node_http2.cc | 117 ++++++++++++++++++++++++---------------------- src/node_http2.h | 41 ++++++++++------ 2 files changed, 89 insertions(+), 69 deletions(-) diff --git a/src/node_http2.cc b/src/node_http2.cc index 96a0fa631e0b3e..5780ad8b840f46 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -114,11 +114,14 @@ Http2Scope::~Http2Scope() { // uses a single TypedArray instance that is shared with the JavaScript side // to more efficiently pass values back and forth. Http2Options::Http2Options(Environment* env, nghttp2_session_type type) { - nghttp2_option_new(&options_); + nghttp2_option* option; + CHECK_EQ(nghttp2_option_new(&option), 0); + CHECK_NOT_NULL(option); + options_.reset(option); // Make sure closed connections aren't kept around, taking up memory. // Note that this breaks the priority tree, which we don't use. - nghttp2_option_set_no_closed_streams(options_, 1); + nghttp2_option_set_no_closed_streams(option, 1); // We manually handle flow control within a session in order to // implement backpressure -- that is, we only send WINDOW_UPDATE @@ -126,13 +129,13 @@ Http2Options::Http2Options(Environment* env, nghttp2_session_type type) { // code. This ensures that the flow of data over the connection // does not move too quickly and limits the amount of data we // are required to buffer. - nghttp2_option_set_no_auto_window_update(options_, 1); + nghttp2_option_set_no_auto_window_update(option, 1); // Enable built in support for receiving ALTSVC and ORIGIN frames (but // only on client side sessions if (type == NGHTTP2_SESSION_CLIENT) { - nghttp2_option_set_builtin_recv_extension_type(options_, NGHTTP2_ALTSVC); - nghttp2_option_set_builtin_recv_extension_type(options_, NGHTTP2_ORIGIN); + nghttp2_option_set_builtin_recv_extension_type(option, NGHTTP2_ALTSVC); + nghttp2_option_set_builtin_recv_extension_type(option, NGHTTP2_ORIGIN); } AliasedUint32Array& buffer = env->http2_state()->options_buffer; @@ -140,27 +143,27 @@ Http2Options::Http2Options(Environment* env, nghttp2_session_type type) { if (flags & (1 << IDX_OPTIONS_MAX_DEFLATE_DYNAMIC_TABLE_SIZE)) { nghttp2_option_set_max_deflate_dynamic_table_size( - options_, + option, buffer[IDX_OPTIONS_MAX_DEFLATE_DYNAMIC_TABLE_SIZE]); } if (flags & (1 << IDX_OPTIONS_MAX_RESERVED_REMOTE_STREAMS)) { nghttp2_option_set_max_reserved_remote_streams( - options_, + option, buffer[IDX_OPTIONS_MAX_RESERVED_REMOTE_STREAMS]); } if (flags & (1 << IDX_OPTIONS_MAX_SEND_HEADER_BLOCK_LENGTH)) { nghttp2_option_set_max_send_header_block_length( - options_, + option, buffer[IDX_OPTIONS_MAX_SEND_HEADER_BLOCK_LENGTH]); } // Recommended default - nghttp2_option_set_peer_max_concurrent_streams(options_, 100); + nghttp2_option_set_peer_max_concurrent_streams(option, 100); if (flags & (1 << IDX_OPTIONS_PEER_MAX_CONCURRENT_STREAMS)) { nghttp2_option_set_peer_max_concurrent_streams( - options_, + option, buffer[IDX_OPTIONS_PEER_MAX_CONCURRENT_STREAMS]); } @@ -179,27 +182,24 @@ Http2Options::Http2Options(Environment* env, nghttp2_session_type type) { // header pairs the session may accept. This is a hard limit.. that is, // 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)) { + if (flags & (1 << IDX_OPTIONS_MAX_HEADER_LIST_PAIRS)) SetMaxHeaderPairs(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 // abused as an attack vector, however, we place a strict upper limit // on the number of unacknowledged PINGS that can be sent at any given // time. - if (flags & (1 << IDX_OPTIONS_MAX_OUTSTANDING_PINGS)) { + if (flags & (1 << IDX_OPTIONS_MAX_OUTSTANDING_PINGS)) SetMaxOutstandingPings(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 // abused as an attack vector, however, we place a strict upper limit // on the number of unacknowledged SETTINGS that can be sent at any given // time. - if (flags & (1 << IDX_OPTIONS_MAX_OUTSTANDING_SETTINGS)) { + if (flags & (1 << IDX_OPTIONS_MAX_OUTSTANDING_SETTINGS)) SetMaxOutstandingSettings(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 @@ -209,9 +209,8 @@ Http2Options::Http2Options(Environment* env, nghttp2_session_type type) { // created. // 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)) { + if (flags & (1 << IDX_OPTIONS_MAX_SESSION_MEMORY)) SetMaxSessionMemory(buffer[IDX_OPTIONS_MAX_SESSION_MEMORY] * 1e6); - } } void Http2Session::Http2Settings::Init() { @@ -421,42 +420,39 @@ Origins::Origins(Isolate* isolate, // Sets the various callback functions that nghttp2 will use to notify us // about significant events while processing http2 stuff. Http2Session::Callbacks::Callbacks(bool kHasGetPaddingCallback) { - CHECK_EQ(nghttp2_session_callbacks_new(&callbacks), 0); + nghttp2_session_callbacks* callbacks_; + CHECK_EQ(nghttp2_session_callbacks_new(&callbacks_), 0); + callbacks.reset(callbacks_); nghttp2_session_callbacks_set_on_begin_headers_callback( - callbacks, OnBeginHeadersCallback); + callbacks_, OnBeginHeadersCallback); nghttp2_session_callbacks_set_on_header_callback2( - callbacks, OnHeaderCallback); + callbacks_, OnHeaderCallback); nghttp2_session_callbacks_set_on_frame_recv_callback( - callbacks, OnFrameReceive); + callbacks_, OnFrameReceive); nghttp2_session_callbacks_set_on_stream_close_callback( - callbacks, OnStreamClose); + callbacks_, OnStreamClose); nghttp2_session_callbacks_set_on_data_chunk_recv_callback( - callbacks, OnDataChunkReceived); + callbacks_, OnDataChunkReceived); nghttp2_session_callbacks_set_on_frame_not_send_callback( - callbacks, OnFrameNotSent); + callbacks_, OnFrameNotSent); nghttp2_session_callbacks_set_on_invalid_header_callback2( - callbacks, OnInvalidHeader); + callbacks_, OnInvalidHeader); nghttp2_session_callbacks_set_error_callback( - callbacks, OnNghttpError); + callbacks_, OnNghttpError); nghttp2_session_callbacks_set_send_data_callback( - callbacks, OnSendData); + callbacks_, OnSendData); nghttp2_session_callbacks_set_on_invalid_frame_recv_callback( - callbacks, OnInvalidFrame); + callbacks_, OnInvalidFrame); nghttp2_session_callbacks_set_on_frame_send_callback( - callbacks, OnFrameSent); + callbacks_, OnFrameSent); if (kHasGetPaddingCallback) { nghttp2_session_callbacks_set_select_padding_callback( - callbacks, OnSelectPadding); + callbacks_, OnSelectPadding); } } - -Http2Session::Callbacks::~Callbacks() { - nghttp2_session_callbacks_del(callbacks); -} - void Http2Session::StopTrackingRcbuf(nghttp2_rcbuf* buf) { StopTrackingMemory(buf); } @@ -500,9 +496,6 @@ Http2Session::Http2Session(Environment* env, bool hasGetPaddingCallback = padding_strategy_ != PADDING_STRATEGY_NONE; - nghttp2_session_callbacks* callbacks - = callback_struct_saved[hasGetPaddingCallback ? 1 : 0].callbacks; - auto fn = type == NGHTTP2_SESSION_SERVER ? nghttp2_session_server_new3 : nghttp2_session_client_new3; @@ -514,7 +507,14 @@ Http2Session::Http2Session(Environment* env, // of the options are out of acceptable range, which we should // be catching before it gets this far. Either way, crash if this // fails. - CHECK_EQ(fn(&session_, callbacks, this, *opts, &alloc_info), 0); + nghttp2_session* session; + CHECK_EQ(fn( + &session, + callback_struct_saved[hasGetPaddingCallback ? 1 : 0].callbacks.get(), + this, + *opts, + &alloc_info), 0); + session_.reset(session); outgoing_storage_.reserve(1024); outgoing_buffers_.reserve(32); @@ -533,7 +533,9 @@ Http2Session::Http2Session(Environment* env, Http2Session::~Http2Session() { CHECK_EQ(flags_ & SESSION_STATE_HAS_SCOPE, 0); Debug(this, "freeing nghttp2 session"); - nghttp2_session_del(session_); + // Explicitly reset session_ so the subsequent + // current_nghttp2_memory_ check passes. + session_.reset(); CHECK_EQ(current_nghttp2_memory_, 0); js_fields_->~SessionJSFields(); } @@ -630,7 +632,7 @@ void Http2Session::Close(uint32_t code, bool socket_closed) { // make a best effort. if (!socket_closed) { Debug(this, "terminating session with code %d", code); - CHECK_EQ(nghttp2_session_terminate_session(session_, code), 0); + CHECK_EQ(nghttp2_session_terminate_session(session_.get(), code), 0); SendPendingData(); } else if (stream_ != nullptr) { stream_->RemoveStreamListener(this); @@ -670,7 +672,7 @@ inline Http2Stream* Http2Session::FindStream(int32_t id) { inline bool Http2Session::CanAddStream() { uint32_t maxConcurrentStreams = nghttp2_session_get_local_settings( - session_, NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS); + session_.get(), NGHTTP2_SETTINGS_MAX_CONCURRENT_STREAMS); size_t maxSize = std::min(streams_.max_size(), static_cast(maxConcurrentStreams)); // We can add a new stream so long as we are less than the current @@ -736,10 +738,10 @@ ssize_t Http2Session::ConsumeHTTP2Data() { // multiple side effects. Debug(this, "receiving %d bytes [wants data? %d]", read_len, - nghttp2_session_want_read(session_)); + nghttp2_session_want_read(session_.get())); flags_ &= ~SESSION_STATE_NGHTTP2_RECV_PAUSED; ssize_t ret = - nghttp2_session_mem_recv(session_, + nghttp2_session_mem_recv(session_.get(), reinterpret_cast(stream_buf_.base) + stream_buf_offset_, read_len); @@ -1438,7 +1440,7 @@ void Http2Session::OnStreamAfterWrite(WriteWrap* w, int status) { if ((flags_ & SESSION_STATE_READING_STOPPED) && !(flags_ & SESSION_STATE_WRITE_IN_PROGRESS) && - nghttp2_session_want_read(session_)) { + nghttp2_session_want_read(session_.get())) { flags_ &= ~SESSION_STATE_READING_STOPPED; stream_->ReadStart(); } @@ -1466,16 +1468,16 @@ void Http2Session::OnStreamAfterWrite(WriteWrap* w, int status) { // queue), but only if a write has not already been scheduled. void Http2Session::MaybeScheduleWrite() { CHECK_EQ(flags_ & SESSION_STATE_WRITE_SCHEDULED, 0); - if (UNLIKELY(session_ == nullptr)) + if (UNLIKELY(!session_)) return; - if (nghttp2_session_want_write(session_)) { + if (nghttp2_session_want_write(session_.get())) { HandleScope handle_scope(env()->isolate()); Debug(this, "scheduling write"); flags_ |= SESSION_STATE_WRITE_SCHEDULED; BaseObjectPtr strong_ref{this}; env()->SetImmediate([this, strong_ref](Environment* env) { - if (session_ == nullptr || !(flags_ & SESSION_STATE_WRITE_SCHEDULED)) { + if (!session_ || !(flags_ & SESSION_STATE_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. @@ -1493,7 +1495,7 @@ void Http2Session::MaybeScheduleWrite() { void Http2Session::MaybeStopReading() { if (flags_ & SESSION_STATE_READING_STOPPED) return; - int want_read = nghttp2_session_want_read(session_); + 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; @@ -1591,7 +1593,7 @@ uint8_t Http2Session::SendPendingData() { // Part One: Gather data from nghttp2 - while ((src_length = nghttp2_session_mem_send(session_, &src)) > 0) { + while ((src_length = nghttp2_session_mem_send(session_.get(), &src)) > 0) { Debug(this, "nghttp2 has %d bytes to send", src_length); CopyDataIntoOutgoing(src, src_length); } @@ -1716,7 +1718,7 @@ Http2Stream* Http2Session::SubmitRequest( Http2Stream* stream = nullptr; Http2Stream::Provider::Stream prov(options); *ret = nghttp2_submit_request( - session_, + session_.get(), prispec, headers.data(), headers.length(), @@ -2486,9 +2488,9 @@ void Http2Session::Goaway(uint32_t code, Http2Scope h2scope(this); // the last proc stream id is the most recently created Http2Stream. if (lastStreamID <= 0) - lastStreamID = nghttp2_session_get_last_proc_stream_id(session_); + lastStreamID = nghttp2_session_get_last_proc_stream_id(session_.get()); Debug(this, "submitting goaway"); - nghttp2_submit_goaway(session_, NGHTTP2_FLAG_NONE, + nghttp2_submit_goaway(session_.get(), NGHTTP2_FLAG_NONE, lastStreamID, code, data, len); } @@ -2677,13 +2679,16 @@ void Http2Session::AltSvc(int32_t id, uint8_t* value, size_t value_len) { Http2Scope h2scope(this); - CHECK_EQ(nghttp2_submit_altsvc(session_, NGHTTP2_FLAG_NONE, id, + CHECK_EQ(nghttp2_submit_altsvc(session_.get(), NGHTTP2_FLAG_NONE, id, origin, origin_len, value, value_len), 0); } void Http2Session::Origin(nghttp2_origin_entry* ov, size_t count) { Http2Scope h2scope(this); - CHECK_EQ(nghttp2_submit_origin(session_, NGHTTP2_FLAG_NONE, ov, count), 0); + CHECK_EQ(nghttp2_submit_origin( + session_.get(), + NGHTTP2_FLAG_NONE, + ov, count), 0); } // Submits an AltSvc frame to be sent to the connected peer. diff --git a/src/node_http2.h b/src/node_http2.h index 96ee839f037059..b31676fbc91d43 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -44,6 +44,24 @@ namespace http2 { #define MIN_MAX_FRAME_SIZE DEFAULT_SETTINGS_MAX_FRAME_SIZE #define MAX_INITIAL_WINDOW_SIZE 2147483647 +template +struct Nghttp2Deleter { + void operator()(T* ptr) const noexcept { fn(ptr); } +}; + +using Nghttp2OptionPointer = + std::unique_ptr>; + +using Nghttp2SessionPointer = + std::unique_ptr>; + +using Nghttp2SessionCallbacksPointer = + std::unique_ptr>; + struct Http2HeadersTraits { typedef nghttp2_nv nv_t; static const uint8_t kNoneFlag = NGHTTP2_NV_FLAG_NONE; @@ -173,12 +191,10 @@ class Http2Options { public: Http2Options(Environment* env, nghttp2_session_type type); - ~Http2Options() { - nghttp2_option_del(options_); - } + ~Http2Options() = default; nghttp2_option* operator*() const { - return options_; + return options_.get(); } void SetMaxHeaderPairs(uint32_t max) { @@ -201,7 +217,7 @@ class Http2Options { max_outstanding_pings_ = max; } - size_t GetMaxOutstandingPings() { + size_t GetMaxOutstandingPings() const { return max_outstanding_pings_; } @@ -209,7 +225,7 @@ class Http2Options { max_outstanding_settings_ = max; } - size_t GetMaxOutstandingSettings() { + size_t GetMaxOutstandingSettings() const { return max_outstanding_settings_; } @@ -217,12 +233,12 @@ class Http2Options { max_session_memory_ = max; } - uint64_t GetMaxSessionMemory() { + uint64_t GetMaxSessionMemory() const { return max_session_memory_; } private: - nghttp2_option* options_; + Nghttp2OptionPointer options_; uint64_t max_session_memory_ = DEFAULT_MAX_SESSION_MEMORY; uint32_t max_header_pairs_ = DEFAULT_MAX_HEADER_LIST_PAIRS; padding_strategy_type padding_strategy_ = PADDING_STRATEGY_NONE; @@ -571,9 +587,9 @@ class Http2Session : public AsyncWrap, inline nghttp2_session_type type() const { return session_type_; } - inline nghttp2_session* session() const { return session_; } + inline nghttp2_session* session() const { return session_.get(); } - inline nghttp2_session* operator*() { return session_; } + inline nghttp2_session* operator*() { return session_.get(); } inline uint32_t GetMaxHeaderPairs() const { return max_header_pairs_; } @@ -799,16 +815,15 @@ class Http2Session : public AsyncWrap, struct Callbacks { inline explicit Callbacks(bool kHasGetPaddingCallback); - inline ~Callbacks(); - nghttp2_session_callbacks* callbacks; + Nghttp2SessionCallbacksPointer callbacks; }; /* Use callback_struct_saved[kHasGetPaddingCallback ? 1 : 0] */ static const Callbacks callback_struct_saved[2]; // The underlying nghttp2_session handle - nghttp2_session* session_; + Nghttp2SessionPointer session_; // JS-accessible numeric fields, as indexed by SessionUint8Fields. SessionJSFields* js_fields_ = nullptr; From 876ae5711a38dc18c7cf14c5452ac5c07fe8ac1c Mon Sep 17 00:00:00 2001 From: James M Snell Date: Sun, 29 Mar 2020 10:14:56 -0700 Subject: [PATCH 2/3] src: rename http2 class and suppress compile warnings Suppress compile warnings on Windows, rename class for consistent styling. Signed-off-by: James M Snell --- src/node_http2.cc | 77 ++++++++++++++++++++++++++--------------------- src/node_http2.h | 34 ++++++++++----------- 2 files changed, 60 insertions(+), 51 deletions(-) diff --git a/src/node_http2.cc b/src/node_http2.cc index 5780ad8b840f46..168817b236fee3 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -210,7 +210,7 @@ Http2Options::Http2Options(Environment* env, 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] * 1e6); + SetMaxSessionMemory(buffer[IDX_OPTIONS_MAX_SESSION_MEMORY] * 1000000); } void Http2Session::Http2Settings::Init() { @@ -350,8 +350,8 @@ Http2Priority::Http2Priority(Environment* env, int32_t weight_ = weight->Int32Value(context).ToChecked(); bool exclusive_ = exclusive->BooleanValue(env->isolate()); Debug(env, DebugCategory::HTTP2STREAM, - "Http2Priority: parent: %d, weight: %d, exclusive: %d\n", - parent_, weight_, exclusive_); + "Http2Priority: parent: %d, weight: %d, exclusive: %s\n", + parent_, weight_, exclusive_ ? "yes" : "no"); nghttp2_priority_spec_init(&spec, parent_, weight_, exclusive_ ? 1 : 0); } @@ -579,8 +579,10 @@ void Http2Stream::EmitStatistics() { } else { buffer[IDX_STREAM_STATS_TIMETOFIRSTBYTESENT] = 0; } - buffer[IDX_STREAM_STATS_SENTBYTES] = entry->sent_bytes(); - buffer[IDX_STREAM_STATS_RECEIVEDBYTES] = entry->received_bytes(); + buffer[IDX_STREAM_STATS_SENTBYTES] = + static_cast(entry->sent_bytes()); + buffer[IDX_STREAM_STATS_RECEIVEDBYTES] = + static_cast(entry->received_bytes()); Local obj; if (entry->ToObject().ToLocal(&obj)) entry->Notify(obj); }); @@ -603,10 +605,12 @@ void Http2Session::EmitStatistics() { buffer[IDX_SESSION_STATS_STREAMCOUNT] = entry->stream_count(); buffer[IDX_SESSION_STATS_STREAMAVERAGEDURATION] = entry->stream_average_duration(); - buffer[IDX_SESSION_STATS_DATA_SENT] = entry->data_sent(); - buffer[IDX_SESSION_STATS_DATA_RECEIVED] = entry->data_received(); + buffer[IDX_SESSION_STATS_DATA_SENT] = + static_cast(entry->data_sent()); + buffer[IDX_SESSION_STATS_DATA_RECEIVED] = + static_cast(entry->data_received()); buffer[IDX_SESSION_STATS_MAX_CONCURRENT_STREAMS] = - entry->max_concurrent_streams(); + static_cast(entry->max_concurrent_streams()); Local obj; if (entry->ToObject().ToLocal(&obj)) entry->Notify(obj); }); @@ -1514,9 +1518,9 @@ void Http2Session::ClearOutgoing(int status) { outgoing_storage_.clear(); outgoing_length_ = 0; - std::vector current_outgoing_buffers_; + std::vector current_outgoing_buffers_; current_outgoing_buffers_.swap(outgoing_buffers_); - for (const nghttp2_stream_write& wr : current_outgoing_buffers_) { + for (const NgHttp2StreamWrite& wr : current_outgoing_buffers_) { WriteWrap* wrap = wr.req_wrap; if (wrap != nullptr) { // TODO(addaleax): Pass `status` instead of 0, so that we actually error @@ -1543,7 +1547,7 @@ void Http2Session::ClearOutgoing(int status) { } } -void Http2Session::PushOutgoingBuffer(nghttp2_stream_write&& write) { +void Http2Session::PushOutgoingBuffer(NgHttp2StreamWrite&& write) { outgoing_length_ += write.buf.len; outgoing_buffers_.emplace_back(std::move(write)); } @@ -1560,7 +1564,7 @@ void Http2Session::CopyDataIntoOutgoing(const uint8_t* src, size_t src_length) { // of the outgoing_buffers_ vector may invalidate the pointer. // The correct base pointers will be set later, before writing to the // underlying socket. - PushOutgoingBuffer(nghttp2_stream_write { + PushOutgoingBuffer(NgHttp2StreamWrite { uv_buf_init(nullptr, src_length) }); } @@ -1623,7 +1627,7 @@ uint8_t Http2Session::SendPendingData() { // (Those are marked by having .base == nullptr.) size_t offset = 0; size_t i = 0; - for (const nghttp2_stream_write& write : outgoing_buffers_) { + for (const NgHttp2StreamWrite& write : outgoing_buffers_) { statistics_.data_sent += write.buf.len; if (write.buf.base == nullptr) { bufs[i++] = uv_buf_init( @@ -1679,7 +1683,7 @@ int Http2Session::OnSendData( // we told it so, which means that we *should* have data available. CHECK(!stream->queue_.empty()); - nghttp2_stream_write& write = stream->queue_.front(); + NgHttp2StreamWrite& write = stream->queue_.front(); if (write.buf.len <= length) { // This write does not suffice by itself, so we can consume it completely. length -= write.buf.len; @@ -1689,7 +1693,7 @@ int Http2Session::OnSendData( } // Slice off `length` bytes of the first write in the queue. - session->PushOutgoingBuffer(nghttp2_stream_write { + session->PushOutgoingBuffer(NgHttp2StreamWrite { uv_buf_init(write.buf.base, length) }); write.buf.base += length; @@ -1699,7 +1703,7 @@ int Http2Session::OnSendData( if (frame->data.padlen > 0) { // Send padding if that was requested. - session->PushOutgoingBuffer(nghttp2_stream_write { + session->PushOutgoingBuffer(NgHttp2StreamWrite { uv_buf_init(const_cast(zero_bytes_256), frame->data.padlen - 1) }); } @@ -1780,7 +1784,7 @@ void Http2Session::OnStreamRead(ssize_t nread, const uv_buf_t& buf_) { // Remember the current buffer, so that OnDataChunkReceived knows the // offset of a DATA frame's data into the socket read buffer. - stream_buf_ = uv_buf_init(buf.data(), nread); + stream_buf_ = uv_buf_init(buf.data(), static_cast(nread)); Isolate* isolate = env()->isolate(); @@ -1793,7 +1797,7 @@ void Http2Session::OnStreamRead(ssize_t nread, const uv_buf_t& buf_) { if (UNLIKELY(ret < 0)) { Debug(this, "fatal error receiving data: %d", ret); - Local arg = Integer::New(isolate, ret); + Local arg = Integer::New(isolate, static_cast(ret)); MakeCallback(env()->http2session_on_error_function(), 1, &arg); return; } @@ -1802,7 +1806,7 @@ void Http2Session::OnStreamRead(ssize_t nread, const uv_buf_t& buf_) { } bool Http2Session::HasWritesOnSocketForStream(Http2Stream* stream) { - for (const nghttp2_stream_write& wr : outgoing_buffers_) { + for (const NgHttp2StreamWrite& wr : outgoing_buffers_) { if (wr.req_wrap != nullptr && wr.req_wrap->stream() == stream) return true; } @@ -1949,7 +1953,7 @@ void Http2Stream::Destroy() { // here because it's possible for destroy to have been called while // we still have queued outbound writes. while (!queue_.empty()) { - nghttp2_stream_write& head = queue_.front(); + NgHttp2StreamWrite& head = queue_.front(); if (head.req_wrap != nullptr) head.req_wrap->Done(UV_ECANCELED); queue_.pop(); @@ -2167,7 +2171,7 @@ int Http2Stream::DoWrite(WriteWrap* req_wrap, for (size_t i = 0; i < nbufs; ++i) { // Store the req_wrap on the last write info in the queue, so that it is // only marked as finished once all buffers associated with it are finished. - queue_.emplace(nghttp2_stream_write { + queue_.emplace(NgHttp2StreamWrite { i == nbufs - 1 ? req_wrap : nullptr, bufs[i] }); @@ -2403,11 +2407,11 @@ void Http2Session::RefreshState(const FunctionCallbackInfo& args) { buffer[IDX_SESSION_STATE_REMOTE_WINDOW_SIZE] = nghttp2_session_get_remote_window_size(s); buffer[IDX_SESSION_STATE_OUTBOUND_QUEUE_SIZE] = - nghttp2_session_get_outbound_queue_size(s); + static_cast(nghttp2_session_get_outbound_queue_size(s)); buffer[IDX_SESSION_STATE_HD_DEFLATE_DYNAMIC_TABLE_SIZE] = - nghttp2_session_get_hd_deflate_dynamic_table_size(s); + static_cast(nghttp2_session_get_hd_deflate_dynamic_table_size(s)); buffer[IDX_SESSION_STATE_HD_INFLATE_DYNAMIC_TABLE_SIZE] = - nghttp2_session_get_hd_inflate_dynamic_table_size(s); + static_cast(nghttp2_session_get_hd_inflate_dynamic_table_size(s)); } @@ -2415,7 +2419,7 @@ void Http2Session::RefreshState(const FunctionCallbackInfo& args) { void Http2Session::New(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); CHECK(args.IsConstructCall()); - int val = args[0]->IntegerValue(env->context()).ToChecked(); + int32_t val = args[0]->Int32Value(env->context()).ToChecked(); nghttp2_session_type type = static_cast(val); Http2Session* session = new Http2Session(env, args.This(), type); session->get_async_id(); // avoid compiler warning @@ -2453,7 +2457,7 @@ void Http2Session::Request(const FunctionCallbackInfo& args) { Environment* env = session->env(); Local headers = args[0].As(); - int options = args[1]->IntegerValue(env->context()).ToChecked(); + int32_t options = args[1]->Int32Value(env->context()).ToChecked(); Http2Priority priority(env, args[2], args[3], args[4]); Debug(session, "request submitted"); @@ -2464,7 +2468,7 @@ void Http2Session::Request(const FunctionCallbackInfo& args) { *priority, Http2Headers(env, headers), &ret, - options); + static_cast(options)); if (ret <= 0 || stream == nullptr) { Debug(session, "could not submit request: %s", nghttp2_strerror(ret)); @@ -2553,10 +2557,12 @@ void Http2Stream::Respond(const FunctionCallbackInfo& args) { ASSIGN_OR_RETURN_UNWRAP(&stream, args.Holder()); Local headers = args[0].As(); - int options = args[1]->IntegerValue(env->context()).ToChecked(); + int32_t options = args[1]->Int32Value(env->context()).ToChecked(); args.GetReturnValue().Set( - stream->SubmitResponse(Http2Headers(env, headers), options)); + stream->SubmitResponse( + Http2Headers(env, headers), + static_cast(options))); Debug(stream, "response submitted"); } @@ -2606,13 +2612,16 @@ void Http2Stream::PushPromise(const FunctionCallbackInfo& args) { ASSIGN_OR_RETURN_UNWRAP(&parent, args.Holder()); Local headers = args[0].As(); - int options = args[1]->IntegerValue(env->context()).ToChecked(); + int32_t options = args[1]->Int32Value(env->context()).ToChecked(); Debug(parent, "creating push promise"); int32_t ret = 0; Http2Stream* stream = - parent->SubmitPushPromise(Http2Headers(env, headers), &ret, options); + parent->SubmitPushPromise( + Http2Headers(env, headers), + &ret, + static_cast(options)); if (ret <= 0 || stream == nullptr) { Debug(parent, "failed to create push stream: %d", ret); @@ -2726,13 +2735,13 @@ void Http2Session::Origin(const FunctionCallbackInfo& args) { ASSIGN_OR_RETURN_UNWRAP(&session, args.Holder()); Local origin_string = args[0].As(); - int count = args[1]->IntegerValue(context).ToChecked(); + int32_t count = args[1]->Int32Value(context).ToChecked(); Origins origins(env->isolate(), env->context(), origin_string, - count); + static_cast(count)); session->Origin(*origins, origins.length()); } @@ -2886,7 +2895,7 @@ void Http2Session::Http2Ping::DetachFromSession() { session_ = nullptr; } -void nghttp2_stream_write::MemoryInfo(MemoryTracker* tracker) const { +void NgHttp2StreamWrite::MemoryInfo(MemoryTracker* tracker) const { if (req_wrap != nullptr) tracker->TrackField("req_wrap", req_wrap->GetAsyncWrap()); tracker->TrackField("buf", buf); diff --git a/src/node_http2.h b/src/node_http2.h index b31676fbc91d43..11c8bff3d73190 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -24,13 +24,13 @@ namespace http2 { // 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 // option. -#define DEFAULT_MAX_PINGS 10 +constexpr size_t kDefaultMaxPings = 10; // Also strictly limit the number of outstanding SETTINGS frames a user sends -#define DEFAULT_MAX_SETTINGS 10 +constexpr size_t kDefaultMaxSettings = 10; // Default maximum total memory cap for Http2Session. -#define DEFAULT_MAX_SESSION_MEMORY 1e7 +constexpr uint64_t kDefaultMaxSessionMemory = 10000000; // These are the standard HTTP/2 defaults as specified by the RFC #define DEFAULT_SETTINGS_HEADER_TABLE_SIZE 4096 @@ -123,17 +123,17 @@ enum nghttp2_stream_options { STREAM_OPTION_GET_TRAILERS = 0x2, }; -struct nghttp2_stream_write : public MemoryRetainer { +struct NgHttp2StreamWrite : public MemoryRetainer { WriteWrap* req_wrap = nullptr; uv_buf_t buf; - inline explicit nghttp2_stream_write(uv_buf_t buf_) : buf(buf_) {} - inline nghttp2_stream_write(WriteWrap* req, uv_buf_t buf_) : + inline explicit NgHttp2StreamWrite(uv_buf_t buf_) : buf(buf_) {} + inline NgHttp2StreamWrite(WriteWrap* req, uv_buf_t buf_) : req_wrap(req), buf(buf_) {} void MemoryInfo(MemoryTracker* tracker) const override; - SET_MEMORY_INFO_NAME(nghttp2_stream_write) - SET_SELF_SIZE(nghttp2_stream_write) + SET_MEMORY_INFO_NAME(NgHttp2StreamWrite) + SET_SELF_SIZE(NgHttp2StreamWrite) }; // The Padding Strategy determines the method by which extra padding is @@ -239,11 +239,11 @@ class Http2Options { private: Nghttp2OptionPointer options_; - uint64_t max_session_memory_ = DEFAULT_MAX_SESSION_MEMORY; + uint64_t max_session_memory_ = kDefaultMaxSessionMemory; uint32_t max_header_pairs_ = DEFAULT_MAX_HEADER_LIST_PAIRS; padding_strategy_type padding_strategy_ = PADDING_STRATEGY_NONE; - size_t max_outstanding_pings_ = DEFAULT_MAX_PINGS; - size_t max_outstanding_settings_ = DEFAULT_MAX_SETTINGS; + size_t max_outstanding_pings_ = kDefaultMaxPings; + size_t max_outstanding_settings_ = kDefaultMaxSettings; }; class Http2Priority { @@ -473,7 +473,7 @@ class Http2Stream : public AsyncWrap, // Outbound Data... This is the data written by the JS layer that is // waiting to be written out to the socket. - std::queue queue_; + std::queue queue_; size_t available_outbound_length_ = 0; Http2StreamListener stream_listener_; @@ -836,7 +836,7 @@ class Http2Session : public AsyncWrap, uint32_t max_header_pairs_ = DEFAULT_MAX_HEADER_LIST_PAIRS; // The maximum amount of memory allocated for this session - uint64_t max_session_memory_ = DEFAULT_MAX_SESSION_MEMORY; + uint64_t max_session_memory_ = kDefaultMaxSessionMemory; uint64_t current_session_memory_ = 0; // The amount of memory allocated by nghttp2 internals uint64_t current_nghttp2_memory_ = 0; @@ -859,13 +859,13 @@ class Http2Session : public AsyncWrap, AllocatedBuffer stream_buf_allocation_; size_t stream_buf_offset_ = 0; - size_t max_outstanding_pings_ = DEFAULT_MAX_PINGS; + size_t max_outstanding_pings_ = kDefaultMaxPings; std::queue> outstanding_pings_; - size_t max_outstanding_settings_ = DEFAULT_MAX_SETTINGS; + size_t max_outstanding_settings_ = kDefaultMaxSettings; std::queue> outstanding_settings_; - std::vector outgoing_buffers_; + std::vector outgoing_buffers_; std::vector outgoing_storage_; size_t outgoing_length_ = 0; std::vector pending_rst_streams_; @@ -877,7 +877,7 @@ class Http2Session : public AsyncWrap, // Also use the invalid frame count as a measure for rejecting input frames. uint32_t invalid_frame_count_ = 0; - void PushOutgoingBuffer(nghttp2_stream_write&& write); + void PushOutgoingBuffer(NgHttp2StreamWrite&& write); void CopyDataIntoOutgoing(const uint8_t* src, size_t src_length); void ClearOutgoing(int status); From 9db4418b165e784d5deb29637f15cb20f1d51bc8 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Sun, 29 Mar 2020 10:38:24 -0700 Subject: [PATCH 3/3] src: minor http2 refactorings * Simplify Http2Priority struct * BooleanValue => IsTrue/IsFalse Signed-off-by: James M Snell --- src/node_http2.cc | 17 +++++++---------- src/node_http2.h | 9 +-------- 2 files changed, 8 insertions(+), 18 deletions(-) diff --git a/src/node_http2.cc b/src/node_http2.cc index 168817b236fee3..cd3b989a57e46e 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -348,11 +348,11 @@ Http2Priority::Http2Priority(Environment* env, Local context = env->context(); int32_t parent_ = parent->Int32Value(context).ToChecked(); int32_t weight_ = weight->Int32Value(context).ToChecked(); - bool exclusive_ = exclusive->BooleanValue(env->isolate()); + bool exclusive_ = exclusive->IsTrue(); Debug(env, DebugCategory::HTTP2STREAM, "Http2Priority: parent: %d, weight: %d, exclusive: %s\n", parent_, weight_, exclusive_ ? "yes" : "no"); - nghttp2_priority_spec_init(&spec, parent_, weight_, exclusive_ ? 1 : 0); + nghttp2_priority_spec_init(this, parent_, weight_, exclusive_ ? 1 : 0); } @@ -996,8 +996,7 @@ int Http2Session::OnStreamClose(nghttp2_session* handle, MaybeLocal answer = stream->MakeCallback(env->http2session_on_stream_close_function(), 1, &arg); - if (answer.IsEmpty() || - !(answer.ToLocalChecked()->BooleanValue(env->isolate()))) { + if (answer.IsEmpty() || answer.ToLocalChecked()->IsFalse()) { // Skip to destroy stream->Destroy(); } @@ -2444,9 +2443,7 @@ void Http2Session::Destroy(const FunctionCallbackInfo& args) { Local context = env->context(); uint32_t code = args[0]->Uint32Value(context).ToChecked(); - bool socketDestroyed = args[1]->BooleanValue(env->isolate()); - - session->Close(code, socketDestroyed); + session->Close(code, args[1]->IsTrue()); } // Submits a new request on the Http2Session and returns either an error code @@ -2465,7 +2462,7 @@ void Http2Session::Request(const FunctionCallbackInfo& args) { int32_t ret = 0; Http2Stream* stream = session->Http2Session::SubmitRequest( - *priority, + &priority, Http2Headers(env, headers), &ret, static_cast(options)); @@ -2638,9 +2635,9 @@ void Http2Stream::Priority(const FunctionCallbackInfo& args) { ASSIGN_OR_RETURN_UNWRAP(&stream, args.Holder()); Http2Priority priority(env, args[0], args[1], args[2]); - bool silent = args[3]->BooleanValue(env->isolate()); + bool silent = args[3]->IsTrue(); - CHECK_EQ(stream->SubmitPriority(*priority, silent), 0); + CHECK_EQ(stream->SubmitPriority(&priority, silent), 0); Debug(stream, "priority submitted"); } diff --git a/src/node_http2.h b/src/node_http2.h index 11c8bff3d73190..b07f9cf4130d7b 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -246,18 +246,11 @@ class Http2Options { size_t max_outstanding_settings_ = kDefaultMaxSettings; }; -class Http2Priority { - public: +struct Http2Priority : public nghttp2_priority_spec { Http2Priority(Environment* env, v8::Local parent, v8::Local weight, v8::Local exclusive); - - nghttp2_priority_spec* operator*() { - return &spec; - } - private: - nghttp2_priority_spec spec; }; class Http2StreamListener : public StreamListener {