From e4d369e96e0f75786c84dacb45f0a8790615b386 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Thu, 2 Jul 2020 07:07:14 -0700 Subject: [PATCH] quic: remove onSessionDestroy callback The QuicSession can be destroyed during garbage collection and the onSessionDestroy callback was happening in the destructor. PR-URL: https://github.com/nodejs/node/pull/34160 Reviewed-By: Anna Henningsen --- lib/internal/quic/core.js | 30 ++++--------------- src/env.h | 1 - src/quic/node_quic.cc | 1 - src/quic/node_quic_session.cc | 17 +---------- src/quic/node_quic_session.h | 2 -- .../test-quic-maxconnectionsperhost.js | 4 +-- 6 files changed, 9 insertions(+), 46 deletions(-) diff --git a/lib/internal/quic/core.js b/lib/internal/quic/core.js index 00907398b09b8d..babbdb39bcf6d9 100644 --- a/lib/internal/quic/core.js +++ b/lib/internal/quic/core.js @@ -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) { @@ -558,7 +543,6 @@ setCallbacks({ onSessionCert, onSessionClientHello, onSessionClose, - onSessionDestroyed, onSessionHandshake, onSessionKeylog, onSessionQlog, @@ -1908,13 +1892,8 @@ 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); @@ -1922,14 +1901,17 @@ class QuicSession extends EventEmitter { // 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 diff --git a/src/env.h b/src/env.h index 823aab9f55a301..07e8307f056640 100644 --- a/src/env.h +++ b/src/env.h @@ -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) \ diff --git a/src/quic/node_quic.cc b/src/quic/node_quic.cc index 49aba2e5db2870..a651c2f951ffdf 100644 --- a/src/quic/node_quic.cc +++ b/src/quic/node_quic.cc @@ -59,7 +59,6 @@ void QuicSetCallbacks(const FunctionCallbackInfo& 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); diff --git a/src/quic/node_quic_session.cc b/src/quic/node_quic_session.cc index ffc4f25bfcc9d8..0ac7d2ec6fa2a2 100644 --- a/src/quic/node_quic_session.cc +++ b/src/quic/node_quic_session.cc @@ -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); @@ -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()); @@ -1412,6 +1397,7 @@ QuicSession::QuicSession( QuicCID(), options, preferred_address_strategy) { + set_wrapped(); InitClient( local_addr, remote_addr, @@ -1472,7 +1458,6 @@ QuicSession::~QuicSession() { CHECK(!Ngtcp2CallbackScope::InNgtcp2CallbackScope(this)); QuicSessionListener* listener_ = listener(); - listener_->OnSessionDestroyed(); if (listener_ == listener()) RemoveListener(listener_); diff --git a/src/quic/node_quic_session.h b/src/quic/node_quic_session.h index acf433622b38d3..7556e2fc5878a3 100644 --- a/src/quic/node_quic_session.h +++ b/src/quic/node_quic_session.h @@ -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); @@ -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; diff --git a/test/parallel/test-quic-maxconnectionsperhost.js b/test/parallel/test-quic-maxconnectionsperhost.js index b94849b88272c3..1bfbb7b8e790ea 100644 --- a/test/parallel/test-quic-maxconnectionsperhost.js +++ b/test/parallel/test-quic-maxconnectionsperhost.js @@ -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(); })); }));