Skip to content

Commit

Permalink
quic: remove onSessionDestroy callback
Browse files Browse the repository at this point in the history
The QuicSession can be destroyed during garbage collection and
the onSessionDestroy callback was happening in the destructor.

PR-URL: #34160
Reviewed-By: Anna Henningsen <anna@addaleax.net>
  • Loading branch information
jasnell committed Jul 5, 2020
1 parent 3acdd6a commit e4d369e
Show file tree
Hide file tree
Showing 6 changed files with 9 additions and 46 deletions.
30 changes: 6 additions & 24 deletions lib/internal/quic/core.js
Original file line number Diff line number Diff line change
Expand Up @@ -288,21 +288,6 @@ function onSessionClose(code, family, silent, statelessReset) {
this[owner_symbol][kDestroy](code, family, silent, statelessReset);
}

// Called by the C++ internals when a QuicSession has been destroyed.
// When this is called, the QuicSession is no longer usable. Removing
// the handle and emitting close is the only action.
// TODO(@jasnell): In the future, this will need to act differently
// for QuicClientSessions when autoResume is enabled.
function onSessionDestroyed() {
const session = this[owner_symbol];
this[owner_symbol] = undefined;

if (session) {
session[kSetHandle]();
process.nextTick(emit.bind(session, 'close'));
}
}

// Used only within the onSessionClientHello function. Invoked
// to complete the client hello process.
function clientHelloCallback(err, ...args) {
Expand Down Expand Up @@ -558,7 +543,6 @@ setCallbacks({
onSessionCert,
onSessionClientHello,
onSessionClose,
onSessionDestroyed,
onSessionHandshake,
onSessionKeylog,
onSessionQlog,
Expand Down Expand Up @@ -1908,28 +1892,26 @@ class QuicSession extends EventEmitter {
this.removeListener('newListener', onNewListener);
this.removeListener('removeListener', onRemoveListener);

// If we are destroying with an error, schedule the
// error to be emitted on process.nextTick.
if (error) process.nextTick(emit.bind(this, 'error', error));

const handle = this[kHandle];
this[kHandle] = undefined;

this[kHandle] = undefined
if (handle !== undefined) {
// Copy the stats for use after destruction
state.stats = new BigInt64Array(handle.stats);
state.idleTimeout = this[kInternalState].state.idleTimeout;

// Destroy the underlying QuicSession handle
handle.destroy(state.closeCode, state.closeFamily);
} else {
process.nextTick(emit.bind(this, 'close'));
}

// Remove the QuicSession JavaScript object from the
// associated QuicSocket.
state.socket[kRemoveSession](this);
state.socket = undefined;

// If we are destroying with an error, schedule the
// error to be emitted on process.nextTick.
if (error) process.nextTick(emit.bind(this, 'error', error));
process.nextTick(emit.bind(this, 'close'));
}

// For server QuicSession instances, true if earlyData is
Expand Down
1 change: 0 additions & 1 deletion src/env.h
Original file line number Diff line number Diff line change
Expand Up @@ -454,7 +454,6 @@ constexpr size_t kFsStatsBufferLength =
V(quic_on_session_cert_function, v8::Function) \
V(quic_on_session_client_hello_function, v8::Function) \
V(quic_on_session_close_function, v8::Function) \
V(quic_on_session_destroyed_function, v8::Function) \
V(quic_on_session_handshake_function, v8::Function) \
V(quic_on_session_keylog_function, v8::Function) \
V(quic_on_session_path_validation_function, v8::Function) \
Expand Down
1 change: 0 additions & 1 deletion src/quic/node_quic.cc
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,6 @@ void QuicSetCallbacks(const FunctionCallbackInfo<Value>& args) {
SETFUNCTION("onSessionCert", session_cert);
SETFUNCTION("onSessionClientHello", session_client_hello);
SETFUNCTION("onSessionClose", session_close);
SETFUNCTION("onSessionDestroyed", session_destroyed);
SETFUNCTION("onSessionHandshake", session_handshake);
SETFUNCTION("onSessionKeylog", session_keylog);
SETFUNCTION("onSessionUsePreferredAddress", session_use_preferred_address);
Expand Down
17 changes: 1 addition & 16 deletions src/quic/node_quic_session.cc
Original file line number Diff line number Diff line change
Expand Up @@ -289,11 +289,6 @@ void QuicSessionListener::OnStreamReset(
previous_listener_->OnStreamReset(stream_id, app_error_code);
}

void QuicSessionListener::OnSessionDestroyed() {
if (previous_listener_ != nullptr)
previous_listener_->OnSessionDestroyed();
}

void QuicSessionListener::OnSessionClose(QuicError error, int flags) {
if (previous_listener_ != nullptr)
previous_listener_->OnSessionClose(error, flags);
Expand Down Expand Up @@ -509,16 +504,6 @@ void JSQuicSessionListener::OnStreamReset(
argv);
}

void JSQuicSessionListener::OnSessionDestroyed() {
Environment* env = session()->env();
HandleScope scope(env->isolate());
Context::Scope context_scope(env->context());
// Emit the 'close' event in JS. This needs to happen after destroying the
// connection, because doing so also releases the last qlog data.
session()->MakeCallback(
env->quic_on_session_destroyed_function(), 0, nullptr);
}

void JSQuicSessionListener::OnSessionClose(QuicError error, int flags) {
Environment* env = session()->env();
HandleScope scope(env->isolate());
Expand Down Expand Up @@ -1412,6 +1397,7 @@ QuicSession::QuicSession(
QuicCID(),
options,
preferred_address_strategy) {
set_wrapped();
InitClient(
local_addr,
remote_addr,
Expand Down Expand Up @@ -1472,7 +1458,6 @@ QuicSession::~QuicSession() {
CHECK(!Ngtcp2CallbackScope::InNgtcp2CallbackScope(this));

QuicSessionListener* listener_ = listener();
listener_->OnSessionDestroyed();
if (listener_ == listener())
RemoveListener(listener_);

Expand Down
2 changes: 0 additions & 2 deletions src/quic/node_quic_session.h
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,6 @@ class QuicSessionListener {
virtual void OnStreamReset(
int64_t stream_id,
uint64_t app_error_code);
virtual void OnSessionDestroyed();
virtual void OnSessionClose(
QuicError error,
int flags = SESSION_CLOSE_FLAG_NONE);
Expand Down Expand Up @@ -299,7 +298,6 @@ class JSQuicSessionListener : public QuicSessionListener {
void OnStreamReset(
int64_t stream_id,
uint64_t app_error_code) override;
void OnSessionDestroyed() override;
void OnSessionClose(
QuicError error,
int flags = SESSION_CLOSE_FLAG_NONE) override;
Expand Down
4 changes: 2 additions & 2 deletions test/parallel/test-quic-maxconnectionsperhost.js
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,8 @@ const kALPN = 'zzz';
countdown.dec();
// Shutdown the remaining open sessions.
setImmediate(common.mustCall(() => {
for (const req of sessions)
req.close();
for (const req of sessions)
req.close();
}));
}));

Expand Down

0 comments on commit e4d369e

Please sign in to comment.