Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit d0de204

Browse filesBrowse files
addaleaxtargos
addaleax
authored and
targos
committed
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: #28088 PR-URL: #28150 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
1 parent e517b03 commit d0de204
Copy full SHA for d0de204

File tree

Expand file treeCollapse file tree

5 files changed

+111
-55
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

5 files changed

+111
-55
lines changed
Open diff view settings
Collapse file

‎src/base_object-inl.h‎

Copy file name to clipboardExpand all lines: src/base_object-inl.h
+4-2Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -40,9 +40,8 @@ BaseObject::BaseObject(Environment* env, v8::Local<v8::Object> object)
4040
env_->AddCleanupHook(DeleteMe, static_cast<void*>(this));
4141
}
4242

43-
4443
BaseObject::~BaseObject() {
45-
env_->RemoveCleanupHook(DeleteMe, static_cast<void*>(this));
44+
RemoveCleanupHook();
4645

4746
if (persistent_handle_.IsEmpty()) {
4847
// This most likely happened because the weak callback below cleared it.
@@ -55,6 +54,9 @@ BaseObject::~BaseObject() {
5554
}
5655
}
5756

57+
void BaseObject::RemoveCleanupHook() {
58+
env_->RemoveCleanupHook(DeleteMe, static_cast<void*>(this));
59+
}
5860

5961
v8::Global<v8::Object>& BaseObject::persistent() {
6062
return persistent_handle_;
Collapse file

‎src/base_object.h‎

Copy file name to clipboardExpand all lines: src/base_object.h
+6Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,12 @@ class BaseObject : public MemoryRetainer {
8383
v8::Local<v8::Value> value,
8484
const v8::PropertyCallbackInfo<void>& info);
8585

86+
protected:
87+
// Can be used to avoid the automatic object deletion when the Environment
88+
// exits, for example when this object is owned and deleted by another
89+
// BaseObject at that point.
90+
inline void RemoveCleanupHook();
91+
8692
private:
8793
v8::Local<v8::Object> WrappedObject() const override;
8894
static void DeleteMe(void* data);
Collapse file

‎src/node_http2.cc‎

Copy file name to clipboardExpand all lines: src/node_http2.cc
+58-45Lines changed: 58 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -237,6 +237,7 @@ Http2Session::Http2Settings::Http2Settings(Environment* env,
237237
: AsyncWrap(env, obj, PROVIDER_HTTP2SETTINGS),
238238
session_(session),
239239
startTime_(start_time) {
240+
RemoveCleanupHook(); // This object is owned by the Http2Session.
240241
Init();
241242
}
242243

@@ -311,12 +312,11 @@ void Http2Session::Http2Settings::Done(bool ack) {
311312
uint64_t end = uv_hrtime();
312313
double duration = (end - startTime_) / 1e6;
313314

314-
Local<Value> argv[2] = {
315+
Local<Value> argv[] = {
315316
Boolean::New(env()->isolate(), ack),
316317
Number::New(env()->isolate(), duration)
317318
};
318319
MakeCallback(env()->ondone_string(), arraysize(argv), argv);
319-
delete this;
320320
}
321321

322322
// The Http2Priority class initializes an appropriate nghttp2_priority_spec
@@ -746,11 +746,14 @@ void Http2Session::Close(uint32_t code, bool socket_closed) {
746746
// If there are outstanding pings, those will need to be canceled, do
747747
// so on the next iteration of the event loop to avoid calling out into
748748
// javascript since this may be called during garbage collection.
749-
while (!outstanding_pings_.empty()) {
750-
Http2Session::Http2Ping* ping = PopPing();
751-
env()->SetImmediate([](Environment* env, void* data) {
752-
static_cast<Http2Session::Http2Ping*>(data)->Done(false);
753-
}, static_cast<void*>(ping));
749+
while (std::unique_ptr<Http2Ping> ping = PopPing()) {
750+
ping->DetachFromSession();
751+
env()->SetImmediate(
752+
[](Environment* env, void* data) {
753+
std::unique_ptr<Http2Ping> ping{static_cast<Http2Ping*>(data)};
754+
ping->Done(false);
755+
},
756+
static_cast<void*>(ping.release()));
754757
}
755758

756759
statistics_.end_time = uv_hrtime();
@@ -1435,9 +1438,9 @@ void Http2Session::HandlePingFrame(const nghttp2_frame* frame) {
14351438
Local<Value> arg;
14361439
bool ack = frame->hd.flags & NGHTTP2_FLAG_ACK;
14371440
if (ack) {
1438-
Http2Ping* ping = PopPing();
1441+
std::unique_ptr<Http2Ping> ping = PopPing();
14391442

1440-
if (ping == nullptr) {
1443+
if (!ping) {
14411444
// PING Ack is unsolicited. Treat as a connection error. The HTTP/2
14421445
// spec does not require this, but there is no legitimate reason to
14431446
// receive an unsolicited PING ack on a connection. Either the peer
@@ -1470,8 +1473,8 @@ void Http2Session::HandleSettingsFrame(const nghttp2_frame* frame) {
14701473

14711474
// If this is an acknowledgement, we should have an Http2Settings
14721475
// object for it.
1473-
Http2Settings* settings = PopSettings();
1474-
if (settings != nullptr) {
1476+
std::unique_ptr<Http2Settings> settings = PopSettings();
1477+
if (settings) {
14751478
settings->Done(true);
14761479
return;
14771480
}
@@ -2775,15 +2778,12 @@ void Http2Session::Ping(const FunctionCallbackInfo<Value>& args) {
27752778
}
27762779
if (obj->Set(env->context(), env->ondone_string(), args[1]).IsNothing())
27772780
return;
2778-
Http2Session::Http2Ping* ping = new Http2Ping(session, obj);
27792781

2782+
Http2Ping* ping = session->AddPing(std::make_unique<Http2Ping>(session, obj));
27802783
// To prevent abuse, we strictly limit the number of unacknowledged PING
27812784
// frames that may be sent at any given time. This is configurable in the
27822785
// Options when creating a Http2Session.
2783-
if (!session->AddPing(ping)) {
2784-
ping->Done(false);
2785-
return args.GetReturnValue().Set(false);
2786-
}
2786+
if (ping == nullptr) return args.GetReturnValue().Set(false);
27872787

27882788
// The Ping itself is an Async resource. When the acknowledgement is received,
27892789
// the callback will be invoked and a notification sent out to JS land. The
@@ -2808,60 +2808,67 @@ void Http2Session::Settings(const FunctionCallbackInfo<Value>& args) {
28082808
if (obj->Set(env->context(), env->ondone_string(), args[0]).IsNothing())
28092809
return;
28102810

2811-
Http2Session::Http2Settings* settings =
2812-
new Http2Settings(session->env(), session, obj, 0);
2813-
if (!session->AddSettings(settings)) {
2814-
settings->Done(false);
2815-
return args.GetReturnValue().Set(false);
2816-
}
2811+
Http2Session::Http2Settings* settings = session->AddSettings(
2812+
std::make_unique<Http2Settings>(session->env(), session, obj, 0));
2813+
if (settings == nullptr) return args.GetReturnValue().Set(false);
28172814

28182815
settings->Send();
28192816
args.GetReturnValue().Set(true);
28202817
}
28212818

2822-
2823-
Http2Session::Http2Ping* Http2Session::PopPing() {
2824-
Http2Ping* ping = nullptr;
2819+
std::unique_ptr<Http2Session::Http2Ping> Http2Session::PopPing() {
2820+
std::unique_ptr<Http2Ping> ping;
28252821
if (!outstanding_pings_.empty()) {
2826-
ping = outstanding_pings_.front();
2822+
ping = std::move(outstanding_pings_.front());
28272823
outstanding_pings_.pop();
28282824
DecrementCurrentSessionMemory(sizeof(*ping));
28292825
}
28302826
return ping;
28312827
}
28322828

2833-
bool Http2Session::AddPing(Http2Session::Http2Ping* ping) {
2834-
if (outstanding_pings_.size() == max_outstanding_pings_)
2835-
return false;
2836-
outstanding_pings_.push(ping);
2829+
Http2Session::Http2Ping* Http2Session::AddPing(
2830+
std::unique_ptr<Http2Session::Http2Ping> ping) {
2831+
if (outstanding_pings_.size() == max_outstanding_pings_) {
2832+
ping->Done(false);
2833+
return nullptr;
2834+
}
2835+
Http2Ping* ptr = ping.get();
2836+
outstanding_pings_.emplace(std::move(ping));
28372837
IncrementCurrentSessionMemory(sizeof(*ping));
2838-
return true;
2838+
return ptr;
28392839
}
28402840

2841-
Http2Session::Http2Settings* Http2Session::PopSettings() {
2842-
Http2Settings* settings = nullptr;
2841+
std::unique_ptr<Http2Session::Http2Settings> Http2Session::PopSettings() {
2842+
std::unique_ptr<Http2Settings> settings;
28432843
if (!outstanding_settings_.empty()) {
2844-
settings = outstanding_settings_.front();
2844+
settings = std::move(outstanding_settings_.front());
28452845
outstanding_settings_.pop();
28462846
DecrementCurrentSessionMemory(sizeof(*settings));
28472847
}
28482848
return settings;
28492849
}
28502850

2851-
bool Http2Session::AddSettings(Http2Session::Http2Settings* settings) {
2852-
if (outstanding_settings_.size() == max_outstanding_settings_)
2853-
return false;
2854-
outstanding_settings_.push(settings);
2851+
Http2Session::Http2Settings* Http2Session::AddSettings(
2852+
std::unique_ptr<Http2Session::Http2Settings> settings) {
2853+
if (outstanding_settings_.size() == max_outstanding_settings_) {
2854+
settings->Done(false);
2855+
return nullptr;
2856+
}
2857+
Http2Settings* ptr = settings.get();
2858+
outstanding_settings_.emplace(std::move(settings));
28552859
IncrementCurrentSessionMemory(sizeof(*settings));
2856-
return true;
2860+
return ptr;
28572861
}
28582862

28592863
Http2Session::Http2Ping::Http2Ping(Http2Session* session, Local<Object> obj)
28602864
: AsyncWrap(session->env(), obj, AsyncWrap::PROVIDER_HTTP2PING),
28612865
session_(session),
2862-
startTime_(uv_hrtime()) {}
2866+
startTime_(uv_hrtime()) {
2867+
RemoveCleanupHook(); // This object is owned by the Http2Session.
2868+
}
28632869

28642870
void Http2Session::Http2Ping::Send(const uint8_t* payload) {
2871+
CHECK_NOT_NULL(session_);
28652872
uint8_t data[8];
28662873
if (payload == nullptr) {
28672874
memcpy(&data, &startTime_, arraysize(data));
@@ -2872,8 +2879,12 @@ void Http2Session::Http2Ping::Send(const uint8_t* payload) {
28722879
}
28732880

28742881
void Http2Session::Http2Ping::Done(bool ack, const uint8_t* payload) {
2875-
session_->statistics_.ping_rtt = uv_hrtime() - startTime_;
2876-
double duration = session_->statistics_.ping_rtt / 1e6;
2882+
uint64_t duration_ns = uv_hrtime() - startTime_;
2883+
double duration_ms = duration_ns / 1e6;
2884+
if (session_ != nullptr) session_->statistics_.ping_rtt = duration_ns;
2885+
2886+
HandleScope handle_scope(env()->isolate());
2887+
Context::Scope context_scope(env()->context());
28772888

28782889
Local<Value> buf = Undefined(env()->isolate());
28792890
if (payload != nullptr) {
@@ -2882,15 +2893,17 @@ void Http2Session::Http2Ping::Done(bool ack, const uint8_t* payload) {
28822893
8).ToLocalChecked();
28832894
}
28842895

2885-
Local<Value> argv[3] = {
2896+
Local<Value> argv[] = {
28862897
Boolean::New(env()->isolate(), ack),
2887-
Number::New(env()->isolate(), duration),
2898+
Number::New(env()->isolate(), duration_ms),
28882899
buf
28892900
};
28902901
MakeCallback(env()->ondone_string(), arraysize(argv), argv);
2891-
delete this;
28922902
}
28932903

2904+
void Http2Session::Http2Ping::DetachFromSession() {
2905+
session_ = nullptr;
2906+
}
28942907

28952908
void nghttp2_stream_write::MemoryInfo(MemoryTracker* tracker) const {
28962909
if (req_wrap != nullptr)
Collapse file

‎src/node_http2.h‎

Copy file name to clipboardExpand all lines: src/node_http2.h
+7-8Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -803,11 +803,11 @@ class Http2Session : public AsyncWrap, public StreamListener {
803803
return env()->event_loop();
804804
}
805805

806-
Http2Ping* PopPing();
807-
bool AddPing(Http2Ping* ping);
806+
std::unique_ptr<Http2Ping> PopPing();
807+
Http2Ping* AddPing(std::unique_ptr<Http2Ping> ping);
808808

809-
Http2Settings* PopSettings();
810-
bool AddSettings(Http2Settings* settings);
809+
std::unique_ptr<Http2Settings> PopSettings();
810+
Http2Settings* AddSettings(std::unique_ptr<Http2Settings> settings);
811811

812812
void IncrementCurrentSessionMemory(uint64_t amount) {
813813
current_session_memory_ += amount;
@@ -976,10 +976,10 @@ class Http2Session : public AsyncWrap, public StreamListener {
976976
v8::Local<v8::ArrayBuffer> stream_buf_ab_;
977977

978978
size_t max_outstanding_pings_ = DEFAULT_MAX_PINGS;
979-
std::queue<Http2Ping*> outstanding_pings_;
979+
std::queue<std::unique_ptr<Http2Ping>> outstanding_pings_;
980980

981981
size_t max_outstanding_settings_ = DEFAULT_MAX_SETTINGS;
982-
std::queue<Http2Settings*> outstanding_settings_;
982+
std::queue<std::unique_ptr<Http2Settings>> outstanding_settings_;
983983

984984
std::vector<nghttp2_stream_write> outgoing_buffers_;
985985
std::vector<uint8_t> outgoing_storage_;
@@ -1086,12 +1086,11 @@ class Http2Session::Http2Ping : public AsyncWrap {
10861086

10871087
void Send(const uint8_t* payload);
10881088
void Done(bool ack, const uint8_t* payload = nullptr);
1089+
void DetachFromSession();
10891090

10901091
private:
10911092
Http2Session* session_;
10921093
uint64_t startTime_;
1093-
1094-
friend class Http2Session;
10951094
};
10961095

10971096
// The Http2Settings class is used to parse the settings passed in for
Collapse file

‎test/parallel/test-http2-ping-settings-heapdump.js‎

Copy file name to clipboard
+36Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
'use strict';
2+
const common = require('../common');
3+
if (!common.hasCrypto)
4+
common.skip('missing crypto');
5+
6+
const http2 = require('http2');
7+
const v8 = require('v8');
8+
9+
// Regression test for https://github.com/nodejs/node/issues/28088:
10+
// Verify that Http2Settings and Http2Ping objects don't reference the session
11+
// after it is destroyed, either because they are detached from it or have been
12+
// destroyed themselves.
13+
14+
for (const variant of ['ping', 'settings']) {
15+
const server = http2.createServer();
16+
server.on('session', common.mustCall((session) => {
17+
if (variant === 'ping') {
18+
session.ping(common.expectsError({
19+
code: 'ERR_HTTP2_PING_CANCEL'
20+
}));
21+
} else {
22+
session.settings(undefined, common.mustNotCall());
23+
}
24+
25+
session.on('close', common.mustCall(() => {
26+
v8.getHeapSnapshot().resume();
27+
server.close();
28+
}));
29+
session.destroy();
30+
}));
31+
32+
server.listen(0, common.mustCall(() => {
33+
http2.connect(`http://localhost:${server.address().port}`,
34+
common.mustCall());
35+
}));
36+
}

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.