Skip to content

Commit

Permalink
http2: use custom BaseObject smart pointers
Browse files Browse the repository at this point in the history
Refs: nodejs/quic#141
Reviewed-By: James M Snell <[email protected]>

PR-URL: #30374
Refs: nodejs/quic#149
Refs: nodejs/quic#165
Reviewed-By: David Carlier <[email protected]>
  • Loading branch information
addaleax authored and MylesBorins committed Nov 21, 2019
1 parent a2dbadc commit 6665881
Show file tree
Hide file tree
Showing 4 changed files with 27 additions and 34 deletions.
4 changes: 0 additions & 4 deletions src/base_object-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -63,10 +63,6 @@ BaseObject::~BaseObject() {
}
}

void BaseObject::RemoveCleanupHook() {
env_->RemoveCleanupHook(DeleteMe, static_cast<void*>(this));
}

void BaseObject::Detach() {
CHECK_GT(pointer_data()->strong_ptr_count, 0);
pointer_data()->is_detached = true;
Expand Down
1 change: 0 additions & 1 deletion src/base_object.h
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,6 @@ class BaseObject : public MemoryRetainer {
inline void Detach();

protected:
inline void RemoveCleanupHook(); // TODO(addaleax): Remove.
virtual inline void OnGCCollect();

private:
Expand Down
38 changes: 18 additions & 20 deletions src/node_http2.cc
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,6 @@ Http2Session::Http2Settings::Http2Settings(Environment* env,
: AsyncWrap(env, obj, PROVIDER_HTTP2SETTINGS),
session_(session),
startTime_(start_time) {
RemoveCleanupHook(); // This object is owned by the Http2Session.
Init();
}

Expand Down Expand Up @@ -658,8 +657,6 @@ Http2Session::Http2Session(Environment* env,
Http2Session::~Http2Session() {
CHECK_EQ(flags_ & SESSION_STATE_HAS_SCOPE, 0);
Debug(this, "freeing nghttp2 session");
for (const auto& iter : streams_)
iter.second->session_ = nullptr;
nghttp2_session_del(session_);
CHECK_EQ(current_nghttp2_memory_, 0);
}
Expand Down Expand Up @@ -767,7 +764,7 @@ 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 (std::unique_ptr<Http2Ping> ping = PopPing()) {
while (BaseObjectPtr<Http2Ping> ping = PopPing()) {
ping->DetachFromSession();
env()->SetImmediate(
[ping = std::move(ping)](Environment* env) {
Expand Down Expand Up @@ -1483,7 +1480,7 @@ void Http2Session::HandlePingFrame(const nghttp2_frame* frame) {
Local<Value> arg;
bool ack = frame->hd.flags & NGHTTP2_FLAG_ACK;
if (ack) {
std::unique_ptr<Http2Ping> ping = PopPing();
BaseObjectPtr<Http2Ping> ping = PopPing();

if (!ping) {
// PING Ack is unsolicited. Treat as a connection error. The HTTP/2
Expand Down Expand Up @@ -1522,7 +1519,7 @@ void Http2Session::HandleSettingsFrame(const nghttp2_frame* frame) {

// If this is an acknowledgement, we should have an Http2Settings
// object for it.
std::unique_ptr<Http2Settings> settings = PopSettings();
BaseObjectPtr<Http2Settings> settings = PopSettings();
if (settings) {
settings->Done(true);
return;
Expand Down Expand Up @@ -1982,12 +1979,11 @@ Http2Stream::~Http2Stream() {
nghttp2_rcbuf_decref(header.value);
}

if (session_ == nullptr)
if (!session_)
return;
Debug(this, "tearing down stream");
session_->DecrementCurrentSessionMemory(current_headers_length_);
session_->RemoveStream(this);
session_ = nullptr;
}

std::string Http2Stream::diagnostic_name() const {
Expand Down Expand Up @@ -2189,8 +2185,10 @@ Http2Stream* Http2Stream::SubmitPushPromise(nghttp2_nv* nva,
id_, nva, len, nullptr);
CHECK_NE(*ret, NGHTTP2_ERR_NOMEM);
Http2Stream* stream = nullptr;
if (*ret > 0)
stream = Http2Stream::New(session_, *ret, NGHTTP2_HCAT_HEADERS, options);
if (*ret > 0) {
stream = Http2Stream::New(
session_.get(), *ret, NGHTTP2_HCAT_HEADERS, options);
}

return stream;
}
Expand Down Expand Up @@ -2855,7 +2853,8 @@ void Http2Session::Ping(const FunctionCallbackInfo<Value>& args) {
if (obj->Set(env->context(), env->ondone_string(), args[1]).IsNothing())
return;

Http2Ping* ping = session->AddPing(std::make_unique<Http2Ping>(session, obj));
Http2Ping* ping = session->AddPing(
MakeDetachedBaseObject<Http2Ping>(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.
Expand Down Expand Up @@ -2884,16 +2883,16 @@ void Http2Session::Settings(const FunctionCallbackInfo<Value>& args) {
if (obj->Set(env->context(), env->ondone_string(), args[0]).IsNothing())
return;

Http2Session::Http2Settings* settings = session->AddSettings(
std::make_unique<Http2Settings>(session->env(), session, obj, 0));
Http2Settings* settings = session->AddSettings(
MakeDetachedBaseObject<Http2Settings>(session->env(), session, obj, 0));
if (settings == nullptr) return args.GetReturnValue().Set(false);

settings->Send();
args.GetReturnValue().Set(true);
}

std::unique_ptr<Http2Session::Http2Ping> Http2Session::PopPing() {
std::unique_ptr<Http2Ping> ping;
BaseObjectPtr<Http2Session::Http2Ping> Http2Session::PopPing() {
BaseObjectPtr<Http2Ping> ping;
if (!outstanding_pings_.empty()) {
ping = std::move(outstanding_pings_.front());
outstanding_pings_.pop();
Expand All @@ -2903,7 +2902,7 @@ std::unique_ptr<Http2Session::Http2Ping> Http2Session::PopPing() {
}

Http2Session::Http2Ping* Http2Session::AddPing(
std::unique_ptr<Http2Session::Http2Ping> ping) {
BaseObjectPtr<Http2Session::Http2Ping> ping) {
if (outstanding_pings_.size() == max_outstanding_pings_) {
ping->Done(false);
return nullptr;
Expand All @@ -2914,8 +2913,8 @@ Http2Session::Http2Ping* Http2Session::AddPing(
return ptr;
}

std::unique_ptr<Http2Session::Http2Settings> Http2Session::PopSettings() {
std::unique_ptr<Http2Settings> settings;
BaseObjectPtr<Http2Session::Http2Settings> Http2Session::PopSettings() {
BaseObjectPtr<Http2Settings> settings;
if (!outstanding_settings_.empty()) {
settings = std::move(outstanding_settings_.front());
outstanding_settings_.pop();
Expand All @@ -2925,7 +2924,7 @@ std::unique_ptr<Http2Session::Http2Settings> Http2Session::PopSettings() {
}

Http2Session::Http2Settings* Http2Session::AddSettings(
std::unique_ptr<Http2Session::Http2Settings> settings) {
BaseObjectPtr<Http2Session::Http2Settings> settings) {
if (outstanding_settings_.size() == max_outstanding_settings_) {
settings->Done(false);
return nullptr;
Expand All @@ -2940,7 +2939,6 @@ Http2Session::Http2Ping::Http2Ping(Http2Session* session, Local<Object> obj)
: AsyncWrap(session->env(), obj, AsyncWrap::PROVIDER_HTTP2PING),
session_(session),
startTime_(uv_hrtime()) {
RemoveCleanupHook(); // This object is owned by the Http2Session.
}

void Http2Session::Http2Ping::Send(const uint8_t* payload) {
Expand Down
18 changes: 9 additions & 9 deletions src/node_http2.h
Original file line number Diff line number Diff line change
Expand Up @@ -456,8 +456,8 @@ class Http2Stream : public AsyncWrap,

nghttp2_stream* operator*();

Http2Session* session() { return session_; }
const Http2Session* session() const { return session_; }
Http2Session* session() { return session_.get(); }
const Http2Session* session() const { return session_.get(); }

void EmitStatistics();

Expand Down Expand Up @@ -609,7 +609,7 @@ class Http2Stream : public AsyncWrap,
nghttp2_headers_category category,
int options);

Http2Session* session_ = nullptr; // The Parent HTTP/2 Session
BaseObjectWeakPtr<Http2Session> session_; // The Parent HTTP/2 Session
int32_t id_ = 0; // The Stream Identifier
int32_t code_ = NGHTTP2_NO_ERROR; // The RST_STREAM code (if any)
int flags_ = NGHTTP2_STREAM_FLAG_NONE; // Internal state flags
Expand Down Expand Up @@ -822,11 +822,11 @@ class Http2Session : public AsyncWrap, public StreamListener {
return env()->event_loop();
}

std::unique_ptr<Http2Ping> PopPing();
Http2Ping* AddPing(std::unique_ptr<Http2Ping> ping);
BaseObjectPtr<Http2Ping> PopPing();
Http2Ping* AddPing(BaseObjectPtr<Http2Ping> ping);

std::unique_ptr<Http2Settings> PopSettings();
Http2Settings* AddSettings(std::unique_ptr<Http2Settings> settings);
BaseObjectPtr<Http2Settings> PopSettings();
Http2Settings* AddSettings(BaseObjectPtr<Http2Settings> settings);

void IncrementCurrentSessionMemory(uint64_t amount) {
current_session_memory_ += amount;
Expand Down Expand Up @@ -1001,10 +1001,10 @@ class Http2Session : public AsyncWrap, public StreamListener {
size_t stream_buf_offset_ = 0;

size_t max_outstanding_pings_ = DEFAULT_MAX_PINGS;
std::queue<std::unique_ptr<Http2Ping>> outstanding_pings_;
std::queue<BaseObjectPtr<Http2Ping>> outstanding_pings_;

size_t max_outstanding_settings_ = DEFAULT_MAX_SETTINGS;
std::queue<std::unique_ptr<Http2Settings>> outstanding_settings_;
std::queue<BaseObjectPtr<Http2Settings>> outstanding_settings_;

std::vector<nghttp2_stream_write> outgoing_buffers_;
std::vector<uint8_t> outgoing_storage_;
Expand Down

0 comments on commit 6665881

Please sign in to comment.