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 <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>
This commit is contained in:
parent
14be3aa6e6
commit
2a9f1ad4b0
@ -40,9 +40,8 @@ BaseObject::BaseObject(Environment* env, v8::Local<v8::Object> object)
|
|||||||
env_->AddCleanupHook(DeleteMe, static_cast<void*>(this));
|
env_->AddCleanupHook(DeleteMe, static_cast<void*>(this));
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
||||||
BaseObject::~BaseObject() {
|
BaseObject::~BaseObject() {
|
||||||
env_->RemoveCleanupHook(DeleteMe, static_cast<void*>(this));
|
RemoveCleanupHook();
|
||||||
|
|
||||||
if (persistent_handle_.IsEmpty()) {
|
if (persistent_handle_.IsEmpty()) {
|
||||||
// This most likely happened because the weak callback below cleared it.
|
// 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<void*>(this));
|
||||||
|
}
|
||||||
|
|
||||||
v8::Global<v8::Object>& BaseObject::persistent() {
|
v8::Global<v8::Object>& BaseObject::persistent() {
|
||||||
return persistent_handle_;
|
return persistent_handle_;
|
||||||
|
@ -83,6 +83,12 @@ class BaseObject : public MemoryRetainer {
|
|||||||
v8::Local<v8::Value> value,
|
v8::Local<v8::Value> value,
|
||||||
const v8::PropertyCallbackInfo<void>& info);
|
const v8::PropertyCallbackInfo<void>& 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:
|
private:
|
||||||
v8::Local<v8::Object> WrappedObject() const override;
|
v8::Local<v8::Object> WrappedObject() const override;
|
||||||
static void DeleteMe(void* data);
|
static void DeleteMe(void* data);
|
||||||
|
@ -237,6 +237,7 @@ Http2Session::Http2Settings::Http2Settings(Environment* env,
|
|||||||
: AsyncWrap(env, obj, PROVIDER_HTTP2SETTINGS),
|
: AsyncWrap(env, obj, PROVIDER_HTTP2SETTINGS),
|
||||||
session_(session),
|
session_(session),
|
||||||
startTime_(start_time) {
|
startTime_(start_time) {
|
||||||
|
RemoveCleanupHook(); // This object is owned by the Http2Session.
|
||||||
Init();
|
Init();
|
||||||
}
|
}
|
||||||
|
|
||||||
@ -311,12 +312,11 @@ void Http2Session::Http2Settings::Done(bool ack) {
|
|||||||
uint64_t end = uv_hrtime();
|
uint64_t end = uv_hrtime();
|
||||||
double duration = (end - startTime_) / 1e6;
|
double duration = (end - startTime_) / 1e6;
|
||||||
|
|
||||||
Local<Value> argv[2] = {
|
Local<Value> argv[] = {
|
||||||
Boolean::New(env()->isolate(), ack),
|
Boolean::New(env()->isolate(), ack),
|
||||||
Number::New(env()->isolate(), duration)
|
Number::New(env()->isolate(), duration)
|
||||||
};
|
};
|
||||||
MakeCallback(env()->ondone_string(), arraysize(argv), argv);
|
MakeCallback(env()->ondone_string(), arraysize(argv), argv);
|
||||||
delete this;
|
|
||||||
}
|
}
|
||||||
|
|
||||||
// The Http2Priority class initializes an appropriate nghttp2_priority_spec
|
// 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
|
// 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
|
// so on the next iteration of the event loop to avoid calling out into
|
||||||
// javascript since this may be called during garbage collection.
|
// javascript since this may be called during garbage collection.
|
||||||
while (!outstanding_pings_.empty()) {
|
while (std::unique_ptr<Http2Ping> ping = PopPing()) {
|
||||||
Http2Session::Http2Ping* ping = PopPing();
|
ping->DetachFromSession();
|
||||||
env()->SetImmediate([](Environment* env, void* data) {
|
env()->SetImmediate(
|
||||||
static_cast<Http2Session::Http2Ping*>(data)->Done(false);
|
[](Environment* env, void* data) {
|
||||||
}, static_cast<void*>(ping));
|
std::unique_ptr<Http2Ping> ping{static_cast<Http2Ping*>(data)};
|
||||||
|
ping->Done(false);
|
||||||
|
},
|
||||||
|
static_cast<void*>(ping.release()));
|
||||||
}
|
}
|
||||||
|
|
||||||
statistics_.end_time = uv_hrtime();
|
statistics_.end_time = uv_hrtime();
|
||||||
@ -1435,9 +1438,9 @@ void Http2Session::HandlePingFrame(const nghttp2_frame* frame) {
|
|||||||
Local<Value> arg;
|
Local<Value> arg;
|
||||||
bool ack = frame->hd.flags & NGHTTP2_FLAG_ACK;
|
bool ack = frame->hd.flags & NGHTTP2_FLAG_ACK;
|
||||||
if (ack) {
|
if (ack) {
|
||||||
Http2Ping* ping = PopPing();
|
std::unique_ptr<Http2Ping> ping = PopPing();
|
||||||
|
|
||||||
if (ping == nullptr) {
|
if (!ping) {
|
||||||
// PING Ack is unsolicited. Treat as a connection error. The HTTP/2
|
// PING Ack is unsolicited. Treat as a connection error. The HTTP/2
|
||||||
// spec does not require this, but there is no legitimate reason to
|
// spec does not require this, but there is no legitimate reason to
|
||||||
// receive an unsolicited PING ack on a connection. Either the peer
|
// 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
|
// If this is an acknowledgement, we should have an Http2Settings
|
||||||
// object for it.
|
// object for it.
|
||||||
Http2Settings* settings = PopSettings();
|
std::unique_ptr<Http2Settings> settings = PopSettings();
|
||||||
if (settings != nullptr) {
|
if (settings) {
|
||||||
settings->Done(true);
|
settings->Done(true);
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
@ -2775,15 +2778,12 @@ void Http2Session::Ping(const FunctionCallbackInfo<Value>& args) {
|
|||||||
}
|
}
|
||||||
if (obj->Set(env->context(), env->ondone_string(), args[1]).IsNothing())
|
if (obj->Set(env->context(), env->ondone_string(), args[1]).IsNothing())
|
||||||
return;
|
return;
|
||||||
Http2Session::Http2Ping* ping = new Http2Ping(session, obj);
|
|
||||||
|
|
||||||
|
Http2Ping* ping = session->AddPing(std::make_unique<Http2Ping>(session, obj));
|
||||||
// To prevent abuse, we strictly limit the number of unacknowledged PING
|
// 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
|
// frames that may be sent at any given time. This is configurable in the
|
||||||
// Options when creating a Http2Session.
|
// Options when creating a Http2Session.
|
||||||
if (!session->AddPing(ping)) {
|
if (ping == nullptr) return args.GetReturnValue().Set(false);
|
||||||
ping->Done(false);
|
|
||||||
return args.GetReturnValue().Set(false);
|
|
||||||
}
|
|
||||||
|
|
||||||
// The Ping itself is an Async resource. When the acknowledgement is received,
|
// 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
|
// 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) {
|
|||||||
if (obj->Set(env->context(), env->ondone_string(), args[0]).IsNothing())
|
if (obj->Set(env->context(), env->ondone_string(), args[0]).IsNothing())
|
||||||
return;
|
return;
|
||||||
|
|
||||||
Http2Session::Http2Settings* settings =
|
Http2Session::Http2Settings* settings = session->AddSettings(
|
||||||
new Http2Settings(session->env(), session, obj, 0);
|
std::make_unique<Http2Settings>(session->env(), session, obj, 0));
|
||||||
if (!session->AddSettings(settings)) {
|
if (settings == nullptr) return args.GetReturnValue().Set(false);
|
||||||
settings->Done(false);
|
|
||||||
return args.GetReturnValue().Set(false);
|
|
||||||
}
|
|
||||||
|
|
||||||
settings->Send();
|
settings->Send();
|
||||||
args.GetReturnValue().Set(true);
|
args.GetReturnValue().Set(true);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
std::unique_ptr<Http2Session::Http2Ping> Http2Session::PopPing() {
|
||||||
Http2Session::Http2Ping* Http2Session::PopPing() {
|
std::unique_ptr<Http2Ping> ping;
|
||||||
Http2Ping* ping = nullptr;
|
|
||||||
if (!outstanding_pings_.empty()) {
|
if (!outstanding_pings_.empty()) {
|
||||||
ping = outstanding_pings_.front();
|
ping = std::move(outstanding_pings_.front());
|
||||||
outstanding_pings_.pop();
|
outstanding_pings_.pop();
|
||||||
DecrementCurrentSessionMemory(sizeof(*ping));
|
DecrementCurrentSessionMemory(sizeof(*ping));
|
||||||
}
|
}
|
||||||
return ping;
|
return ping;
|
||||||
}
|
}
|
||||||
|
|
||||||
bool Http2Session::AddPing(Http2Session::Http2Ping* ping) {
|
Http2Session::Http2Ping* Http2Session::AddPing(
|
||||||
if (outstanding_pings_.size() == max_outstanding_pings_)
|
std::unique_ptr<Http2Session::Http2Ping> ping) {
|
||||||
return false;
|
if (outstanding_pings_.size() == max_outstanding_pings_) {
|
||||||
outstanding_pings_.push(ping);
|
ping->Done(false);
|
||||||
|
return nullptr;
|
||||||
|
}
|
||||||
|
Http2Ping* ptr = ping.get();
|
||||||
|
outstanding_pings_.emplace(std::move(ping));
|
||||||
IncrementCurrentSessionMemory(sizeof(*ping));
|
IncrementCurrentSessionMemory(sizeof(*ping));
|
||||||
return true;
|
return ptr;
|
||||||
}
|
}
|
||||||
|
|
||||||
Http2Session::Http2Settings* Http2Session::PopSettings() {
|
std::unique_ptr<Http2Session::Http2Settings> Http2Session::PopSettings() {
|
||||||
Http2Settings* settings = nullptr;
|
std::unique_ptr<Http2Settings> settings;
|
||||||
if (!outstanding_settings_.empty()) {
|
if (!outstanding_settings_.empty()) {
|
||||||
settings = outstanding_settings_.front();
|
settings = std::move(outstanding_settings_.front());
|
||||||
outstanding_settings_.pop();
|
outstanding_settings_.pop();
|
||||||
DecrementCurrentSessionMemory(sizeof(*settings));
|
DecrementCurrentSessionMemory(sizeof(*settings));
|
||||||
}
|
}
|
||||||
return settings;
|
return settings;
|
||||||
}
|
}
|
||||||
|
|
||||||
bool Http2Session::AddSettings(Http2Session::Http2Settings* settings) {
|
Http2Session::Http2Settings* Http2Session::AddSettings(
|
||||||
if (outstanding_settings_.size() == max_outstanding_settings_)
|
std::unique_ptr<Http2Session::Http2Settings> settings) {
|
||||||
return false;
|
if (outstanding_settings_.size() == max_outstanding_settings_) {
|
||||||
outstanding_settings_.push(settings);
|
settings->Done(false);
|
||||||
|
return nullptr;
|
||||||
|
}
|
||||||
|
Http2Settings* ptr = settings.get();
|
||||||
|
outstanding_settings_.emplace(std::move(settings));
|
||||||
IncrementCurrentSessionMemory(sizeof(*settings));
|
IncrementCurrentSessionMemory(sizeof(*settings));
|
||||||
return true;
|
return ptr;
|
||||||
}
|
}
|
||||||
|
|
||||||
Http2Session::Http2Ping::Http2Ping(Http2Session* session, Local<Object> obj)
|
Http2Session::Http2Ping::Http2Ping(Http2Session* session, Local<Object> obj)
|
||||||
: AsyncWrap(session->env(), obj, AsyncWrap::PROVIDER_HTTP2PING),
|
: AsyncWrap(session->env(), obj, AsyncWrap::PROVIDER_HTTP2PING),
|
||||||
session_(session),
|
session_(session),
|
||||||
startTime_(uv_hrtime()) {}
|
startTime_(uv_hrtime()) {
|
||||||
|
RemoveCleanupHook(); // This object is owned by the Http2Session.
|
||||||
|
}
|
||||||
|
|
||||||
void Http2Session::Http2Ping::Send(const uint8_t* payload) {
|
void Http2Session::Http2Ping::Send(const uint8_t* payload) {
|
||||||
|
CHECK_NOT_NULL(session_);
|
||||||
uint8_t data[8];
|
uint8_t data[8];
|
||||||
if (payload == nullptr) {
|
if (payload == nullptr) {
|
||||||
memcpy(&data, &startTime_, arraysize(data));
|
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) {
|
void Http2Session::Http2Ping::Done(bool ack, const uint8_t* payload) {
|
||||||
session_->statistics_.ping_rtt = uv_hrtime() - startTime_;
|
uint64_t duration_ns = uv_hrtime() - startTime_;
|
||||||
double duration = session_->statistics_.ping_rtt / 1e6;
|
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<Value> buf = Undefined(env()->isolate());
|
Local<Value> buf = Undefined(env()->isolate());
|
||||||
if (payload != nullptr) {
|
if (payload != nullptr) {
|
||||||
@ -2882,15 +2893,17 @@ void Http2Session::Http2Ping::Done(bool ack, const uint8_t* payload) {
|
|||||||
8).ToLocalChecked();
|
8).ToLocalChecked();
|
||||||
}
|
}
|
||||||
|
|
||||||
Local<Value> argv[3] = {
|
Local<Value> argv[] = {
|
||||||
Boolean::New(env()->isolate(), ack),
|
Boolean::New(env()->isolate(), ack),
|
||||||
Number::New(env()->isolate(), duration),
|
Number::New(env()->isolate(), duration_ms),
|
||||||
buf
|
buf
|
||||||
};
|
};
|
||||||
MakeCallback(env()->ondone_string(), arraysize(argv), argv);
|
MakeCallback(env()->ondone_string(), arraysize(argv), argv);
|
||||||
delete this;
|
|
||||||
}
|
}
|
||||||
|
|
||||||
|
void Http2Session::Http2Ping::DetachFromSession() {
|
||||||
|
session_ = nullptr;
|
||||||
|
}
|
||||||
|
|
||||||
void nghttp2_stream_write::MemoryInfo(MemoryTracker* tracker) const {
|
void nghttp2_stream_write::MemoryInfo(MemoryTracker* tracker) const {
|
||||||
if (req_wrap != nullptr)
|
if (req_wrap != nullptr)
|
||||||
|
@ -803,11 +803,11 @@ class Http2Session : public AsyncWrap, public StreamListener {
|
|||||||
return env()->event_loop();
|
return env()->event_loop();
|
||||||
}
|
}
|
||||||
|
|
||||||
Http2Ping* PopPing();
|
std::unique_ptr<Http2Ping> PopPing();
|
||||||
bool AddPing(Http2Ping* ping);
|
Http2Ping* AddPing(std::unique_ptr<Http2Ping> ping);
|
||||||
|
|
||||||
Http2Settings* PopSettings();
|
std::unique_ptr<Http2Settings> PopSettings();
|
||||||
bool AddSettings(Http2Settings* settings);
|
Http2Settings* AddSettings(std::unique_ptr<Http2Settings> settings);
|
||||||
|
|
||||||
void IncrementCurrentSessionMemory(uint64_t amount) {
|
void IncrementCurrentSessionMemory(uint64_t amount) {
|
||||||
current_session_memory_ += amount;
|
current_session_memory_ += amount;
|
||||||
@ -976,10 +976,10 @@ class Http2Session : public AsyncWrap, public StreamListener {
|
|||||||
v8::Local<v8::ArrayBuffer> stream_buf_ab_;
|
v8::Local<v8::ArrayBuffer> stream_buf_ab_;
|
||||||
|
|
||||||
size_t max_outstanding_pings_ = DEFAULT_MAX_PINGS;
|
size_t max_outstanding_pings_ = DEFAULT_MAX_PINGS;
|
||||||
std::queue<Http2Ping*> outstanding_pings_;
|
std::queue<std::unique_ptr<Http2Ping>> outstanding_pings_;
|
||||||
|
|
||||||
size_t max_outstanding_settings_ = DEFAULT_MAX_SETTINGS;
|
size_t max_outstanding_settings_ = DEFAULT_MAX_SETTINGS;
|
||||||
std::queue<Http2Settings*> outstanding_settings_;
|
std::queue<std::unique_ptr<Http2Settings>> outstanding_settings_;
|
||||||
|
|
||||||
std::vector<nghttp2_stream_write> outgoing_buffers_;
|
std::vector<nghttp2_stream_write> outgoing_buffers_;
|
||||||
std::vector<uint8_t> outgoing_storage_;
|
std::vector<uint8_t> outgoing_storage_;
|
||||||
@ -1086,12 +1086,11 @@ class Http2Session::Http2Ping : public AsyncWrap {
|
|||||||
|
|
||||||
void Send(const uint8_t* payload);
|
void Send(const uint8_t* payload);
|
||||||
void Done(bool ack, const uint8_t* payload = nullptr);
|
void Done(bool ack, const uint8_t* payload = nullptr);
|
||||||
|
void DetachFromSession();
|
||||||
|
|
||||||
private:
|
private:
|
||||||
Http2Session* session_;
|
Http2Session* session_;
|
||||||
uint64_t startTime_;
|
uint64_t startTime_;
|
||||||
|
|
||||||
friend class Http2Session;
|
|
||||||
};
|
};
|
||||||
|
|
||||||
// The Http2Settings class is used to parse the settings passed in for
|
// The Http2Settings class is used to parse the settings passed in for
|
||||||
|
36
test/parallel/test-http2-ping-settings-heapdump.js
Normal file
36
test/parallel/test-http2-ping-settings-heapdump.js
Normal file
@ -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());
|
||||||
|
}));
|
||||||
|
}
|
Loading…
x
Reference in New Issue
Block a user