Skip to content

Commit

Permalink
quic: fixup set_socket, fix skipped test
Browse files Browse the repository at this point in the history
PR-URL: #34669
Reviewed-By: Anna Henningsen <anna@addaleax.net>
  • Loading branch information
jasnell committed Aug 17, 2020
1 parent 344c5e4 commit 10d5047
Show file tree
Hide file tree
Showing 3 changed files with 41 additions and 25 deletions.
8 changes: 5 additions & 3 deletions lib/internal/quic/core.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -2460,7 +2462,7 @@ class QuicClientSession extends QuicSession {
this[kSetSocket](undefined);
}
socket[kAddSession](this);
this[kSetSocket](socket);
this[kSetSocket](socket, natRebinding);
}
}

Expand Down
44 changes: 25 additions & 19 deletions src/quic/node_quic_session.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand All @@ -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);
Expand All @@ -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;
Expand Down Expand Up @@ -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
Expand Down
14 changes: 11 additions & 3 deletions test/parallel/test-quic-simple-client-migrate.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down Expand Up @@ -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));
Expand Down

0 comments on commit 10d5047

Please sign in to comment.