From 2a9f1ad4b0c725e0362d33e265dc22271c3466e7 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 10 Jun 2019 15:02:06 +0200 Subject: [PATCH] http2: refactor ping + settings object lifetime management Have clearer ownership relations between the `Http2Ping`, `Http2Settings` and `Http2Session` objects. Ping and Settings objects are now owned by the `Http2Session` instance, and deleted along with it, so neither type of object refers to the session after it is gone. In the case of `Http2Ping`s, that deletion is slightly delayed, so we explicitly reset its `session_` property. Fixes: https://github.com/nodejs/node/issues/28088 PR-URL: https://github.com/nodejs/node/pull/28150 Reviewed-By: Matteo Collina Reviewed-By: James M Snell Reviewed-By: Ujjwal Sharma Reviewed-By: Rich Trott --- src/base_object-inl.h | 6 +- src/base_object.h | 6 + src/node_http2.cc | 103 ++++++++++-------- src/node_http2.h | 15 ++- .../test-http2-ping-settings-heapdump.js | 36 ++++++ 5 files changed, 111 insertions(+), 55 deletions(-) create mode 100644 test/parallel/test-http2-ping-settings-heapdump.js diff --git a/src/base_object-inl.h b/src/base_object-inl.h index 50d8aba4a94ad5..af69084f4a5595 100644 --- a/src/base_object-inl.h +++ b/src/base_object-inl.h @@ -40,9 +40,8 @@ BaseObject::BaseObject(Environment* env, v8::Local object) env_->AddCleanupHook(DeleteMe, static_cast(this)); } - BaseObject::~BaseObject() { - env_->RemoveCleanupHook(DeleteMe, static_cast(this)); + RemoveCleanupHook(); if (persistent_handle_.IsEmpty()) { // This most likely happened because the weak callback below cleared it. @@ -55,6 +54,9 @@ BaseObject::~BaseObject() { } } +void BaseObject::RemoveCleanupHook() { + env_->RemoveCleanupHook(DeleteMe, static_cast(this)); +} v8::Global& BaseObject::persistent() { return persistent_handle_; diff --git a/src/base_object.h b/src/base_object.h index f616108a1d9486..53bd4b00d6044a 100644 --- a/src/base_object.h +++ b/src/base_object.h @@ -83,6 +83,12 @@ class BaseObject : public MemoryRetainer { v8::Local value, const v8::PropertyCallbackInfo& info); + protected: + // Can be used to avoid the automatic object deletion when the Environment + // exits, for example when this object is owned and deleted by another + // BaseObject at that point. + inline void RemoveCleanupHook(); + private: v8::Local WrappedObject() const override; static void DeleteMe(void* data); diff --git a/src/node_http2.cc b/src/node_http2.cc index 3a7591f31afda7..9047171e114606 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -237,6 +237,7 @@ Http2Session::Http2Settings::Http2Settings(Environment* env, : AsyncWrap(env, obj, PROVIDER_HTTP2SETTINGS), session_(session), startTime_(start_time) { + RemoveCleanupHook(); // This object is owned by the Http2Session. Init(); } @@ -311,12 +312,11 @@ void Http2Session::Http2Settings::Done(bool ack) { uint64_t end = uv_hrtime(); double duration = (end - startTime_) / 1e6; - Local argv[2] = { + Local argv[] = { Boolean::New(env()->isolate(), ack), Number::New(env()->isolate(), duration) }; MakeCallback(env()->ondone_string(), arraysize(argv), argv); - delete this; } // The Http2Priority class initializes an appropriate nghttp2_priority_spec @@ -746,11 +746,14 @@ void Http2Session::Close(uint32_t code, bool socket_closed) { // If there are outstanding pings, those will need to be canceled, do // so on the next iteration of the event loop to avoid calling out into // javascript since this may be called during garbage collection. - while (!outstanding_pings_.empty()) { - Http2Session::Http2Ping* ping = PopPing(); - env()->SetImmediate([](Environment* env, void* data) { - static_cast(data)->Done(false); - }, static_cast(ping)); + while (std::unique_ptr ping = PopPing()) { + ping->DetachFromSession(); + env()->SetImmediate( + [](Environment* env, void* data) { + std::unique_ptr ping{static_cast(data)}; + ping->Done(false); + }, + static_cast(ping.release())); } statistics_.end_time = uv_hrtime(); @@ -1435,9 +1438,9 @@ void Http2Session::HandlePingFrame(const nghttp2_frame* frame) { Local arg; bool ack = frame->hd.flags & NGHTTP2_FLAG_ACK; if (ack) { - Http2Ping* ping = PopPing(); + std::unique_ptr ping = PopPing(); - if (ping == nullptr) { + if (!ping) { // PING Ack is unsolicited. Treat as a connection error. The HTTP/2 // spec does not require this, but there is no legitimate reason to // receive an unsolicited PING ack on a connection. Either the peer @@ -1470,8 +1473,8 @@ void Http2Session::HandleSettingsFrame(const nghttp2_frame* frame) { // If this is an acknowledgement, we should have an Http2Settings // object for it. - Http2Settings* settings = PopSettings(); - if (settings != nullptr) { + std::unique_ptr settings = PopSettings(); + if (settings) { settings->Done(true); return; } @@ -2775,15 +2778,12 @@ void Http2Session::Ping(const FunctionCallbackInfo& args) { } if (obj->Set(env->context(), env->ondone_string(), args[1]).IsNothing()) return; - Http2Session::Http2Ping* ping = new Http2Ping(session, obj); + Http2Ping* ping = session->AddPing(std::make_unique(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 (!session->AddPing(ping)) { - ping->Done(false); - return args.GetReturnValue().Set(false); - } + 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 @@ -2808,60 +2808,67 @@ void Http2Session::Settings(const FunctionCallbackInfo& args) { if (obj->Set(env->context(), env->ondone_string(), args[0]).IsNothing()) return; - Http2Session::Http2Settings* settings = - new Http2Settings(session->env(), session, obj, 0); - if (!session->AddSettings(settings)) { - settings->Done(false); - return args.GetReturnValue().Set(false); - } + Http2Session::Http2Settings* settings = session->AddSettings( + std::make_unique(session->env(), session, obj, 0)); + if (settings == nullptr) return args.GetReturnValue().Set(false); settings->Send(); args.GetReturnValue().Set(true); } - -Http2Session::Http2Ping* Http2Session::PopPing() { - Http2Ping* ping = nullptr; +std::unique_ptr Http2Session::PopPing() { + std::unique_ptr ping; if (!outstanding_pings_.empty()) { - ping = outstanding_pings_.front(); + ping = std::move(outstanding_pings_.front()); outstanding_pings_.pop(); DecrementCurrentSessionMemory(sizeof(*ping)); } return ping; } -bool Http2Session::AddPing(Http2Session::Http2Ping* ping) { - if (outstanding_pings_.size() == max_outstanding_pings_) - return false; - outstanding_pings_.push(ping); +Http2Session::Http2Ping* Http2Session::AddPing( + std::unique_ptr ping) { + if (outstanding_pings_.size() == max_outstanding_pings_) { + ping->Done(false); + return nullptr; + } + Http2Ping* ptr = ping.get(); + outstanding_pings_.emplace(std::move(ping)); IncrementCurrentSessionMemory(sizeof(*ping)); - return true; + return ptr; } -Http2Session::Http2Settings* Http2Session::PopSettings() { - Http2Settings* settings = nullptr; +std::unique_ptr Http2Session::PopSettings() { + std::unique_ptr settings; if (!outstanding_settings_.empty()) { - settings = outstanding_settings_.front(); + settings = std::move(outstanding_settings_.front()); outstanding_settings_.pop(); DecrementCurrentSessionMemory(sizeof(*settings)); } return settings; } -bool Http2Session::AddSettings(Http2Session::Http2Settings* settings) { - if (outstanding_settings_.size() == max_outstanding_settings_) - return false; - outstanding_settings_.push(settings); +Http2Session::Http2Settings* Http2Session::AddSettings( + std::unique_ptr settings) { + if (outstanding_settings_.size() == max_outstanding_settings_) { + settings->Done(false); + return nullptr; + } + Http2Settings* ptr = settings.get(); + outstanding_settings_.emplace(std::move(settings)); IncrementCurrentSessionMemory(sizeof(*settings)); - return true; + return ptr; } Http2Session::Http2Ping::Http2Ping(Http2Session* session, Local obj) : AsyncWrap(session->env(), obj, AsyncWrap::PROVIDER_HTTP2PING), session_(session), - startTime_(uv_hrtime()) {} + startTime_(uv_hrtime()) { + RemoveCleanupHook(); // This object is owned by the Http2Session. +} void Http2Session::Http2Ping::Send(const uint8_t* payload) { + CHECK_NOT_NULL(session_); uint8_t data[8]; if (payload == nullptr) { memcpy(&data, &startTime_, arraysize(data)); @@ -2872,8 +2879,12 @@ void Http2Session::Http2Ping::Send(const uint8_t* payload) { } void Http2Session::Http2Ping::Done(bool ack, const uint8_t* payload) { - session_->statistics_.ping_rtt = uv_hrtime() - startTime_; - double duration = session_->statistics_.ping_rtt / 1e6; + uint64_t duration_ns = uv_hrtime() - startTime_; + double duration_ms = duration_ns / 1e6; + if (session_ != nullptr) session_->statistics_.ping_rtt = duration_ns; + + HandleScope handle_scope(env()->isolate()); + Context::Scope context_scope(env()->context()); Local buf = Undefined(env()->isolate()); if (payload != nullptr) { @@ -2882,15 +2893,17 @@ void Http2Session::Http2Ping::Done(bool ack, const uint8_t* payload) { 8).ToLocalChecked(); } - Local argv[3] = { + Local argv[] = { Boolean::New(env()->isolate(), ack), - Number::New(env()->isolate(), duration), + Number::New(env()->isolate(), duration_ms), buf }; MakeCallback(env()->ondone_string(), arraysize(argv), argv); - delete this; } +void Http2Session::Http2Ping::DetachFromSession() { + session_ = nullptr; +} void nghttp2_stream_write::MemoryInfo(MemoryTracker* tracker) const { if (req_wrap != nullptr) diff --git a/src/node_http2.h b/src/node_http2.h index c8ffa69f2e49a8..d9636628c25939 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -803,11 +803,11 @@ class Http2Session : public AsyncWrap, public StreamListener { return env()->event_loop(); } - Http2Ping* PopPing(); - bool AddPing(Http2Ping* ping); + std::unique_ptr PopPing(); + Http2Ping* AddPing(std::unique_ptr ping); - Http2Settings* PopSettings(); - bool AddSettings(Http2Settings* settings); + std::unique_ptr PopSettings(); + Http2Settings* AddSettings(std::unique_ptr settings); void IncrementCurrentSessionMemory(uint64_t amount) { current_session_memory_ += amount; @@ -976,10 +976,10 @@ class Http2Session : public AsyncWrap, public StreamListener { v8::Local stream_buf_ab_; size_t max_outstanding_pings_ = DEFAULT_MAX_PINGS; - std::queue outstanding_pings_; + std::queue> outstanding_pings_; size_t max_outstanding_settings_ = DEFAULT_MAX_SETTINGS; - std::queue outstanding_settings_; + std::queue> outstanding_settings_; std::vector outgoing_buffers_; std::vector outgoing_storage_; @@ -1086,12 +1086,11 @@ class Http2Session::Http2Ping : public AsyncWrap { void Send(const uint8_t* payload); void Done(bool ack, const uint8_t* payload = nullptr); + void DetachFromSession(); private: Http2Session* session_; uint64_t startTime_; - - friend class Http2Session; }; // The Http2Settings class is used to parse the settings passed in for diff --git a/test/parallel/test-http2-ping-settings-heapdump.js b/test/parallel/test-http2-ping-settings-heapdump.js new file mode 100644 index 00000000000000..78b3c8cd74f506 --- /dev/null +++ b/test/parallel/test-http2-ping-settings-heapdump.js @@ -0,0 +1,36 @@ +'use strict'; +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); + +const http2 = require('http2'); +const v8 = require('v8'); + +// Regression test for https://github.com/nodejs/node/issues/28088: +// Verify that Http2Settings and Http2Ping objects don't reference the session +// after it is destroyed, either because they are detached from it or have been +// destroyed themselves. + +for (const variant of ['ping', 'settings']) { + const server = http2.createServer(); + server.on('session', common.mustCall((session) => { + if (variant === 'ping') { + session.ping(common.expectsError({ + code: 'ERR_HTTP2_PING_CANCEL' + })); + } else { + session.settings(undefined, common.mustNotCall()); + } + + session.on('close', common.mustCall(() => { + v8.getHeapSnapshot().resume(); + server.close(); + })); + session.destroy(); + })); + + server.listen(0, common.mustCall(() => { + http2.connect(`http://localhost:${server.address().port}`, + common.mustCall()); + })); +}