quic: remove onSessionDestroy callback

jasnell · jasnell · commit e4d369e96e0f · 2020-07-05T14:17:29.000-07:00
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>
diff --git 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,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
diff --git 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
@@ -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);
diff --git 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
@@ -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
@@ -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();
       }));
     }));
 
