From 10d5047a4fbe2cbae4c0895dee4509341e8e77a0 Mon Sep 17 00:00:00 2001
From: James M Snell <jasnell@gmail.com>
Date: Fri, 7 Aug 2020 12:17:47 -0700
Subject: [PATCH] quic: fixup set_socket, fix skipped test

PR-URL: https://github.com/nodejs/node/pull/34669
Reviewed-By: Anna Henningsen <anna@addaleax.net>
---
 lib/internal/quic/core.js                     |  8 ++--
 src/quic/node_quic_session.cc                 | 44 +++++++++++--------
 .../test-quic-simple-client-migrate.js        | 14 ++++--
 3 files changed, 41 insertions(+), 25 deletions(-)

diff --git a/lib/internal/quic/core.js b/lib/internal/quic/core.js
index d311852d950410..93849d0dcd8ecc 100644
--- a/lib/internal/quic/core.js
+++ b/lib/internal/quic/core.js
@@ -1803,8 +1803,10 @@ class QuicSession extends EventEmitter {
     }, depth, options);
   }
 
-  [kSetSocket](socket) {
+  [kSetSocket](socket, natRebinding = false) {
     this[kInternalState].socket = socket;
+    if (socket !== undefined)
+      this[kHandle].setSocket(socket[kHandle], natRebinding);
   }
 
   // Called at the completion of the TLS handshake for the local peer
@@ -2432,7 +2434,7 @@ class QuicClientSession extends QuicSession {
       {};
   }
 
-  async setSocket(socket) {
+  async setSocket(socket, natRebinding = false) {
     if (this.destroyed)
       throw new ERR_INVALID_STATE('QuicClientSession is already destroyed');
     if (this.closing)
@@ -2460,7 +2462,7 @@ class QuicClientSession extends QuicSession {
       this[kSetSocket](undefined);
     }
     socket[kAddSession](this);
-    this[kSetSocket](socket);
+    this[kSetSocket](socket, natRebinding);
   }
 }
 
diff --git a/src/quic/node_quic_session.cc b/src/quic/node_quic_session.cc
index 999dbd71fda689..e75d33e636e4e1 100644
--- a/src/quic/node_quic_session.cc
+++ b/src/quic/node_quic_session.cc
@@ -2479,7 +2479,6 @@ int QuicSession::set_session(SSL_SESSION* session) {
 }
 
 // A client QuicSession can be migrated to a different QuicSocket instance.
-// TODO(@jasnell): This will be revisited.
 bool QuicSession::set_socket(QuicSocket* socket, bool nat_rebinding) {
   CHECK(!is_server());
   CHECK(!is_destroyed());
@@ -2492,8 +2491,6 @@ bool QuicSession::set_socket(QuicSocket* socket, bool nat_rebinding) {
 
   Debug(this, "Migrating to %s", socket->diagnostic_name());
 
-  SendSessionScope send(this);
-
   // Ensure that we maintain a reference to keep this from being
   // destroyed while we are starting the migration.
   BaseObjectPtr<QuicSession> ptr(this);
@@ -2511,22 +2508,31 @@ bool QuicSession::set_socket(QuicSocket* socket, bool nat_rebinding) {
 
   // Step 4: Update ngtcp2
   auto local_address = socket->local_address();
-  if (nat_rebinding) {
-    ngtcp2_addr addr;
-    ngtcp2_addr_init(
-        &addr,
-        local_address.data(),
-        local_address.length(),
-        nullptr);
-    ngtcp2_conn_set_local_addr(connection(), &addr);
-  } else {
+
+  // The nat_rebinding option here should rarely, if ever
+  // be used in a real application. It is intended to serve
+  // as a way of simulating a silent local address change,
+  // such as when the NAT binding changes. Currently, Node.js
+  // does not really have an effective way of detecting that.
+  // Manual user code intervention to handle the migration
+  // to the new QuicSocket is required, which should always
+  // trigger path validation using the ngtcp2_conn_initiate_migration.
+  if (LIKELY(!nat_rebinding)) {
+    SendSessionScope send(this);
     QuicPath path(local_address, remote_address_);
-    if (ngtcp2_conn_initiate_migration(
-            connection(),
-            &path,
-            uv_hrtime()) != 0) {
-      return false;
-    }
+    return ngtcp2_conn_initiate_migration(
+        connection(),
+        &path,
+        uv_hrtime()) == 0;
+  } else {
+    ngtcp2_addr addr;
+    ngtcp2_conn_set_local_addr(
+        connection(),
+        ngtcp2_addr_init(
+            &addr,
+            local_address.data(),
+            local_address.length(),
+            nullptr));
   }
 
   return true;
@@ -3671,7 +3677,7 @@ void QuicSessionSetSocket(const FunctionCallbackInfo<Value>& args) {
   ASSIGN_OR_RETURN_UNWRAP(&session, args.Holder());
   CHECK(args[0]->IsObject());
   ASSIGN_OR_RETURN_UNWRAP(&socket, args[0].As<Object>());
-  args.GetReturnValue().Set(session->set_socket(socket));
+  args.GetReturnValue().Set(session->set_socket(socket, args[1]->IsTrue()));
 }
 
 // GracefulClose flips a flag that prevents new local streams
diff --git a/test/parallel/test-quic-simple-client-migrate.js b/test/parallel/test-quic-simple-client-migrate.js
index d40a693ba3b93d..af15456c855fe6 100644
--- a/test/parallel/test-quic-simple-client-migrate.js
+++ b/test/parallel/test-quic-simple-client-migrate.js
@@ -5,10 +5,9 @@ const common = require('../common');
 if (!common.hasQuic)
   common.skip('missing quic');
 
-common.skip('Not working correct yet... need to refactor');
-
 const assert = require('assert');
 const { key, cert, ca } = require('../common/quic');
+
 const { once } = require('events');
 const { createQuicSocket } = require('net');
 const { pipeline } = require('stream');
@@ -58,14 +57,23 @@ const server = createQuicSocket({ server: options });
   stream.on('end', common.mustCall(() => {
     assert.strictEqual(data, 'Hello from the client');
   }));
-  stream.on('close', common.mustCall());
+  stream.on('close', common.mustCall(() => {
+    req.close();
+  }));
   // Send some data on one connection...
   stream.write('Hello ');
 
   // Wait just a bit, then migrate to a different
   // QuicSocket and continue sending.
   setTimeout(common.mustCall(async () => {
+    const s1 = req.socket;
+    const a1 = req.socket.endpoints[0].address;
+
     await req.setSocket(client2);
+
+    // Verify that it is using a different network endpoint
+    assert.notStrictEqual(s1, req.socket);
+    assert.notDeepStrictEqual(a1, req.socket.endpoints[0].address);
     client.close();
     stream.end('from the client');
   }), common.platformTimeout(100));