Skip to content

Commit 10d5047

Browse files
committed
quic: fixup set_socket, fix skipped test
PR-URL: #34669 Reviewed-By: Anna Henningsen <[email protected]>
1 parent 344c5e4 commit 10d5047

File tree

3 files changed

+41
-25
lines changed

3 files changed

+41
-25
lines changed

lib/internal/quic/core.js

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1803,8 +1803,10 @@ class QuicSession extends EventEmitter {
18031803
}, depth, options);
18041804
}
18051805

1806-
[kSetSocket](socket) {
1806+
[kSetSocket](socket, natRebinding = false) {
18071807
this[kInternalState].socket = socket;
1808+
if (socket !== undefined)
1809+
this[kHandle].setSocket(socket[kHandle], natRebinding);
18081810
}
18091811

18101812
// Called at the completion of the TLS handshake for the local peer
@@ -2432,7 +2434,7 @@ class QuicClientSession extends QuicSession {
24322434
{};
24332435
}
24342436

2435-
async setSocket(socket) {
2437+
async setSocket(socket, natRebinding = false) {
24362438
if (this.destroyed)
24372439
throw new ERR_INVALID_STATE('QuicClientSession is already destroyed');
24382440
if (this.closing)
@@ -2460,7 +2462,7 @@ class QuicClientSession extends QuicSession {
24602462
this[kSetSocket](undefined);
24612463
}
24622464
socket[kAddSession](this);
2463-
this[kSetSocket](socket);
2465+
this[kSetSocket](socket, natRebinding);
24642466
}
24652467
}
24662468

src/quic/node_quic_session.cc

Lines changed: 25 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -2479,7 +2479,6 @@ int QuicSession::set_session(SSL_SESSION* session) {
24792479
}
24802480

24812481
// A client QuicSession can be migrated to a different QuicSocket instance.
2482-
// TODO(@jasnell): This will be revisited.
24832482
bool QuicSession::set_socket(QuicSocket* socket, bool nat_rebinding) {
24842483
CHECK(!is_server());
24852484
CHECK(!is_destroyed());
@@ -2492,8 +2491,6 @@ bool QuicSession::set_socket(QuicSocket* socket, bool nat_rebinding) {
24922491

24932492
Debug(this, "Migrating to %s", socket->diagnostic_name());
24942493

2495-
SendSessionScope send(this);
2496-
24972494
// Ensure that we maintain a reference to keep this from being
24982495
// destroyed while we are starting the migration.
24992496
BaseObjectPtr<QuicSession> ptr(this);
@@ -2511,22 +2508,31 @@ bool QuicSession::set_socket(QuicSocket* socket, bool nat_rebinding) {
25112508

25122509
// Step 4: Update ngtcp2
25132510
auto local_address = socket->local_address();
2514-
if (nat_rebinding) {
2515-
ngtcp2_addr addr;
2516-
ngtcp2_addr_init(
2517-
&addr,
2518-
local_address.data(),
2519-
local_address.length(),
2520-
nullptr);
2521-
ngtcp2_conn_set_local_addr(connection(), &addr);
2522-
} else {
2511+
2512+
// The nat_rebinding option here should rarely, if ever
2513+
// be used in a real application. It is intended to serve
2514+
// as a way of simulating a silent local address change,
2515+
// such as when the NAT binding changes. Currently, Node.js
2516+
// does not really have an effective way of detecting that.
2517+
// Manual user code intervention to handle the migration
2518+
// to the new QuicSocket is required, which should always
2519+
// trigger path validation using the ngtcp2_conn_initiate_migration.
2520+
if (LIKELY(!nat_rebinding)) {
2521+
SendSessionScope send(this);
25232522
QuicPath path(local_address, remote_address_);
2524-
if (ngtcp2_conn_initiate_migration(
2525-
connection(),
2526-
&path,
2527-
uv_hrtime()) != 0) {
2528-
return false;
2529-
}
2523+
return ngtcp2_conn_initiate_migration(
2524+
connection(),
2525+
&path,
2526+
uv_hrtime()) == 0;
2527+
} else {
2528+
ngtcp2_addr addr;
2529+
ngtcp2_conn_set_local_addr(
2530+
connection(),
2531+
ngtcp2_addr_init(
2532+
&addr,
2533+
local_address.data(),
2534+
local_address.length(),
2535+
nullptr));
25302536
}
25312537

25322538
return true;
@@ -3671,7 +3677,7 @@ void QuicSessionSetSocket(const FunctionCallbackInfo<Value>& args) {
36713677
ASSIGN_OR_RETURN_UNWRAP(&session, args.Holder());
36723678
CHECK(args[0]->IsObject());
36733679
ASSIGN_OR_RETURN_UNWRAP(&socket, args[0].As<Object>());
3674-
args.GetReturnValue().Set(session->set_socket(socket));
3680+
args.GetReturnValue().Set(session->set_socket(socket, args[1]->IsTrue()));
36753681
}
36763682

36773683
// GracefulClose flips a flag that prevents new local streams

test/parallel/test-quic-simple-client-migrate.js

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,10 +5,9 @@ const common = require('../common');
55
if (!common.hasQuic)
66
common.skip('missing quic');
77

8-
common.skip('Not working correct yet... need to refactor');
9-
108
const assert = require('assert');
119
const { key, cert, ca } = require('../common/quic');
10+
1211
const { once } = require('events');
1312
const { createQuicSocket } = require('net');
1413
const { pipeline } = require('stream');
@@ -58,14 +57,23 @@ const server = createQuicSocket({ server: options });
5857
stream.on('end', common.mustCall(() => {
5958
assert.strictEqual(data, 'Hello from the client');
6059
}));
61-
stream.on('close', common.mustCall());
60+
stream.on('close', common.mustCall(() => {
61+
req.close();
62+
}));
6263
// Send some data on one connection...
6364
stream.write('Hello ');
6465

6566
// Wait just a bit, then migrate to a different
6667
// QuicSocket and continue sending.
6768
setTimeout(common.mustCall(async () => {
69+
const s1 = req.socket;
70+
const a1 = req.socket.endpoints[0].address;
71+
6872
await req.setSocket(client2);
73+
74+
// Verify that it is using a different network endpoint
75+
assert.notStrictEqual(s1, req.socket);
76+
assert.notDeepStrictEqual(a1, req.socket.endpoints[0].address);
6977
client.close();
7078
stream.end('from the client');
7179
}), common.platformTimeout(100));

0 commit comments

Comments
 (0)