quic: release stream arenas before cleanup · nodejs/node@2c552cd · GitHub
Skip to content

Commit 2c552cd

Browse files
trivikraduh95
authored andcommitted
quic: release stream arenas before cleanup
Release QUIC stream stats and state arena slots when the stream is destroyed instead of from the Stream destructor. Realm cleanup destroys QUIC binding data before draining remaining BaseObjects, so a stream that survives until process teardown must not need BindingData from its destructor. HTTP/3 header callbacks can synchronously destroy their stream. Check whether the stream was destroyed after those callbacks before accessing its arena-backed state, and make repeated native destruction safe after the slots have been released. Signed-off-by: Kamat, Trivikram <16024985+trivikr@users.noreply.github.com> Assisted-by: codex:gpt-5.6-sol PR-URL: #65410 Fixes: #65408 Reviewed-By: James M Snell <jasnell@gmail.com>
1 parent 4586640 commit 2c552cd

4 files changed

Lines changed: 51 additions & 6 deletions

File tree

‎src/quic/http3.cc‎

Lines changed: 8 additions & 0 deletions

‎src/quic/streams.cc‎

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1211,11 +1211,9 @@ Stream::Stream(BaseObjectWeakPtr<Session> session,
12111211
STAT_SET(Stats, max_offset, params->initial_max_data);
12121212
}
12131213

1214-
Stream::~Stream() {
1215-
// Make sure that Destroy() was called before Stream is actually destructed.
1216-
DCHECK_NE(stats()->destroyed_at, 0);
1214+
Stream::~Stream() = default;
12171215

1218-
// Release arena slots back to the freelist.
1216+
void Stream::ReleaseArenaSlots() {
12191217
auto& binding = BindingData::Get(env());
12201218
if (stats_slot_) {
12211219
GetStreamStatsArena(binding).ReleaseSlot(stats_slot_);
@@ -1306,6 +1304,7 @@ bool Stream::is_pending() const {
13061304
}
13071305

13081306
bool Stream::is_destroyed() const {
1307+
if (!stats_slot_) return true;
13091308
return stats()->destroyed_at != 0;
13101309
}
13111310

@@ -1621,7 +1620,7 @@ void Stream::EndReadable(std::optional<uint64_t> maybe_final_size) {
16211620
}
16221621

16231622
void Stream::Destroy(QuicError error) {
1624-
if (stats()->destroyed_at != 0) return;
1623+
if (is_destroyed()) return;
16251624

16261625
// Record the destroyed at timestamp before notifying the JavaScript side
16271626
// that the stream is being destroyed.
@@ -1666,14 +1665,17 @@ void Stream::Destroy(QuicError error) {
16661665
// handle.
16671666
EmitClose(error);
16681667

1668+
stream_id id_to_remove = id();
1669+
ReleaseArenaSlots();
1670+
16691671
auto session = session_;
16701672
session_.reset();
16711673
// EmitClose above triggers MakeCallback which can destroy the session
16721674
// via JS re-entrancy. The weak pointer may still be non-null (the
16731675
// Session BaseObject can be kept alive by a BaseObjectPtr elsewhere,
16741676
// e.g. OnTimeout's ref) even though impl_ has been reset. We must
16751677
// check is_destroyed() to avoid dereferencing the null impl_.
1676-
if (session && !session->is_destroyed()) session->RemoveStream(id());
1678+
if (session && !session->is_destroyed()) session->RemoveStream(id_to_remove);
16771679

16781680
// Critically, make sure that the RemoveStream call is the last thing
16791681
// trying to use this stream object. Once that call is made, the stream

‎src/quic/streams.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -408,6 +408,7 @@ class Stream final : public AsyncWrap,
408408

409409
bool is_local_unidirectional() const;
410410
bool is_remote_unidirectional() const;
411+
void ReleaseArenaSlots();
411412

412413
// JavaScript callouts
413414

Lines changed: 34 additions & 0 deletions

0 commit comments

Comments
 (0)