Skip to content
Open
Show file tree
Hide file tree
Changes from 2 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
1 change: 1 addition & 0 deletions src/quic/session.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2232,6 +2232,7 @@ void Session::ExtendOffset(size_t amount) {
}

void Session::UpdateDataStats() {
if (!impl_) return;
Debug(this, "Updating data stats");
auto& stats_ = impl_->stats_;
ngtcp2_conn_info info;
Expand Down
54 changes: 54 additions & 0 deletions test/parallel/test-quic-session-update-data-stats-after-close.mjs
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't crash on main for me, it just gets killed for a non-settling promise. With NODE_DEBUG=quic:

QUIC 662671: endpoint created
QUIC 662671: endpoint listening as a server
QUIC 662671: endpoint created
QUIC 662671: endpoint connecting as a client
QUIC 662671: session created
QUIC 662671: new server session callback QuicEndpoint { ... } Session { ... }
QUIC 662671: session created
QUIC 662671: session handshake callback localhost h3 TLS_AES_128_GCM_SHA256 TLSv1.3 unable to get local issuer certificate UNABLE_TO_GET_ISSUER_CERT_LOCALLY
QUIC 662671: gracefully closing the session
Warning: Detected unsettled top-level await at file:///tmp/node/test/parallel/test-quic-session-update-data-stats-after-close.mjs:52
await clientSession.closed;
^

Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
// Flags: --experimental-quic --no-warnings
// Regression test for https://github.com/nodejs/node/pull/62126
// UpdateDataStats() must not crash when called after the session's impl_ has
// been reset to null (i.e. after the session is destroyed).
//
// The crash path is in Session::SendDatagram():
// auto on_exit = OnScopeLeave([&] { ...; UpdateDataStats(); });
// If the send encounters an error it calls Close(SILENT) → Destroy() →
// impl_.reset(). The on_exit lambda then fires with impl_ == nullptr.

import { hasQuic, skip, mustCall } from '../common/index.mjs';
import * as fixtures from '../common/fixtures.mjs';

if (!hasQuic) {
skip('QUIC is not enabled');
}

const { listen, connect } = await import('node:quic');
const { createPrivateKey } = await import('node:crypto');

const keys = createPrivateKey(fixtures.readKey('agent1-key.pem'));
const certs = fixtures.readKey('agent1-cert.pem');

// Datagrams must be enabled on both sides for sendDatagram() to work.
const kDatagramOptions = {
transportParams: {
maxDatagramFrameSize: 1200n,
},
};

const serverEndpoint = await listen(
mustCall((serverSession) => {
serverSession.opened.then(mustCall(async () => {
// Send a datagram then immediately close. This exercises the
// UpdateDataStats() call that fires via on_exit after SendDatagram —
// the close can race with or precede the stats update, leaving
// impl_ == nullptr. Before the fix this would crash.
serverSession.sendDatagram(Buffer.from('hello')).catch(() => {});
serverSession.close();
}));
}),
{ keys, certs, ...kDatagramOptions },
);

const clientSession = await connect(serverEndpoint.address, kDatagramOptions);
await clientSession.opened;

// Mirror the race on the client side.
clientSession.sendDatagram(Buffer.from('world')).catch(() => {});
clientSession.close();

await clientSession.closed;
serverEndpoint.close();
await serverEndpoint.closed;