FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

http2: refactor ping + settings object lifetime management · nodejs/node@d0de204 · GitHub

/ node Public

Commit d0de204

Browse files
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

5 files changed

Lines changed: 111 additions & 55 deletions

File tree

‎src/base_object-inl.h‎

Lines 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_;

‎src/base_object.h‎

Lines 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);

‎src/node_http2.cc‎

Lines 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)

‎src/node_http2.h‎

Lines 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

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

Lines 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)
Back | FazBrowse Home | New Git URL