Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

http2: fix GC issue & typos #21194

Closed
wants to merge 4 commits into from
Closed
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 13 additions & 10 deletions src/node_http2.cc
Original file line number Diff line number Diff line change
Expand Up @@ -499,6 +499,8 @@ Http2Session::Http2Session(Environment* env,
Http2Session::~Http2Session() {
CHECK_EQ(flags_ & SESSION_STATE_HAS_SCOPE, 0);
Debug(this, "freeing nghttp2 session");
for (auto stream : streams_)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw, const auto&? I know it’s a pair, so copying overhead isn’t high, but that just looks more correct that way

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed. Thanks!

stream.second->session_ = nullptr;
nghttp2_session_del(session_);
}

Expand All @@ -519,7 +521,7 @@ void Http2Stream::EmitStatistics() {
Http2StreamPerformanceEntry* entry =
new Http2StreamPerformanceEntry(env(), id_, statistics_);
env()->SetImmediate([](Environment* env, void* data) {
// This takes ownership, the entr is destroyed at the end of this scope.
// This takes ownership, the entry is destroyed at the end of this scope.
std::unique_ptr<Http2StreamPerformanceEntry> entry {
static_cast<Http2StreamPerformanceEntry*>(data) };
if (!HasHttp2Observer(env))
Expand Down Expand Up @@ -996,7 +998,7 @@ int Http2Session::OnDataChunkReceived(nghttp2_session* handle,
// `buf.base == nullptr` is the default Http2StreamListener's way
// of saying that it wants a pointer to the raw original.
// Since it has access to the original socket buffer from which the data
// was read in the first place, it can use that to minizime ArrayBuffer
// was read in the first place, it can use that to minimize ArrayBuffer
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😎

// allocations.
if (LIKELY(buf.base == nullptr))
buf.base = reinterpret_cast<char*>(const_cast<uint8_t*>(data));
Expand Down Expand Up @@ -1336,7 +1338,7 @@ void Http2Session::OnStreamAfterWrite(WriteWrap* w, int status) {
}

// If the underlying nghttp2_session struct has data pending in its outbound
// queue, MaybeScheduleWrite will schedule a SendPendingData() call to occcur
// queue, MaybeScheduleWrite will schedule a SendPendingData() call to occur
// on the next iteration of the Node.js event loop (using the SetImmediate
// queue), but only if a write has not already been scheduled.
void Http2Session::MaybeScheduleWrite() {
Expand Down Expand Up @@ -1706,11 +1708,11 @@ Http2Stream::Http2Stream(


Http2Stream::~Http2Stream() {
if (session_ == nullptr)
return;
Debug(this, "tearing down stream");
if (session_ != nullptr) {
session_->RemoveStream(this);
session_ = nullptr;
}
session_->RemoveStream(this);
session_ = nullptr;
}

std::string Http2Stream::diagnostic_name() const {
Expand Down Expand Up @@ -1785,7 +1787,8 @@ void Http2Stream::Destroy() {
// We can destroy the stream now if there are no writes for it
// already on the socket. Otherwise, we'll wait for the garbage collector
// to take care of cleaning up.
if (!stream->session()->HasWritesOnSocketForStream(stream))
if (stream->session() == nullptr ||
!stream->session()->HasWritesOnSocketForStream(stream))
delete stream;
}, this, this->object());

Expand Down Expand Up @@ -2177,7 +2180,7 @@ void Http2Session::RefreshSettings(const FunctionCallbackInfo<Value>& args) {

// A TypedArray instance is shared between C++ and JS land to contain state
// information of the current Http2Session. This updates the values in the
// TypedRray so those can be read in JS land.
// TypedArray so those can be read in JS land.
void Http2Session::RefreshState(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
Http2Session* session;
Expand Down Expand Up @@ -2510,7 +2513,7 @@ void Http2Session::AltSvc(int32_t id,
origin, origin_len, value, value_len), 0);
}

// Submits an AltSvc frame to the sent to the connected peer.
// Submits an AltSvc frame to be sent to the connected peer.
void Http2Session::AltSvc(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
Http2Session* session;
Expand Down