From 594a98be7aaebf359c2948fb2b6174a1137180c3 Mon Sep 17 00:00:00 2001 From: Viktor Chynarov Date: Fri, 23 Aug 2019 15:03:10 -0700 Subject: [PATCH] Stop setting clientConnId in ServerStateMachine [2/x] Summary: Removes clientConnId completely from ServerConnectionIdParams. This diff first fixes an incorrect assumption; it calls `shortHeader.getConnectionId()` which is actually the destination id (server) not the client connection id. Next, this entire block is unnecessary, because this will be called after the transport is created, so the clientConnectionId will always be set. This also setsconn.serverConnectionId earlier (shouldn't depend on connClientId). Reviewed By: yangchi Differential Revision: D16792866 fbshipit-source-id: 537ba12baa9939c9d5512e46eb914c1d3a7a9aa2 --- quic/api/test/QuicTransportBaseTest.cpp | 2 -- quic/client/test/QuicClientTransportTest.cpp | 2 -- quic/codec/DefaultConnectionIdAlgo.cpp | 1 - quic/codec/QuicConnectionId.h | 1 - quic/codec/test/TypesTest.cpp | 2 -- quic/loss/test/QuicLossFunctionsTest.cpp | 2 -- quic/server/state/ServerStateMachine.cpp | 27 +++---------------- .../stream/test/StreamStateMachineTest.cpp | 2 -- 8 files changed, 4 insertions(+), 35 deletions(-) diff --git a/quic/api/test/QuicTransportBaseTest.cpp b/quic/api/test/QuicTransportBaseTest.cpp index ceec1bd80..f53a6f037 100644 --- a/quic/api/test/QuicTransportBaseTest.cpp +++ b/quic/api/test/QuicTransportBaseTest.cpp @@ -163,7 +163,6 @@ class TestQuicTransport while (!cursor.isAtEnd()) { // create server chosen connId with processId = 0 and workerId = 0 ServerConnectionIdParams params(0, 0, 0); - params.clientConnId = *conn_->clientConnectionId; conn_->serverConnectionId = connIdAlgo_->encodeConnectionId(params); auto type = static_cast(cursor.readBE()); if (type == TestFrameType::CRYPTO) { @@ -312,7 +311,6 @@ class TestQuicTransport void setServerConnectionId() { // create server chosen connId with processId = 0 and workerId = 0 ServerConnectionIdParams params(0, 0, 0); - params.clientConnId = *conn_->clientConnectionId; conn_->serverConnectionId = connIdAlgo_->encodeConnectionId(params); } diff --git a/quic/client/test/QuicClientTransportTest.cpp b/quic/client/test/QuicClientTransportTest.cpp index 839733b96..a157328e6 100644 --- a/quic/client/test/QuicClientTransportTest.cpp +++ b/quic/client/test/QuicClientTransportTest.cpp @@ -1298,7 +1298,6 @@ class QuicClientTransportTest : public Test { void setConnectionIds() { originalConnId = client->getConn().clientConnectionId; ServerConnectionIdParams params(0, 0, 0); - params.clientConnId = *client->getConn().clientConnectionId; serverChosenConnId = connIdAlgo_->encodeConnectionId(params); } @@ -2471,7 +2470,6 @@ class QuicClientTransportVersionAndRetryTest originalConnId = client->getConn().clientConnectionId; // create server chosen connId with processId = 0 and workerId = 0 ServerConnectionIdParams params(0, 0, 0); - params.clientConnId = *client->getConn().clientConnectionId; serverChosenConnId = connIdAlgo_->encodeConnectionId(params); // The tests that we do here create streams before crypto is finished, // so we initialize the peer streams, to allow for this behavior. TODO: when diff --git a/quic/codec/DefaultConnectionIdAlgo.cpp b/quic/codec/DefaultConnectionIdAlgo.cpp index 498f48a36..8fb48a205 100644 --- a/quic/codec/DefaultConnectionIdAlgo.cpp +++ b/quic/codec/DefaultConnectionIdAlgo.cpp @@ -180,7 +180,6 @@ ServerConnectionIdParams DefaultConnectionIdAlgo::parseConnectionId( getHostIdBitsInConnId(id), getProcessIdBitsFromConnId(id), getWorkerIdFromConnId(id)); - serverConnIdParams.clientConnId.assign(id); return serverConnIdParams; } diff --git a/quic/codec/QuicConnectionId.h b/quic/codec/QuicConnectionId.h index 9bc975efc..3255e2cfc 100644 --- a/quic/codec/QuicConnectionId.h +++ b/quic/codec/QuicConnectionId.h @@ -110,7 +110,6 @@ struct ServerConnectionIdParams { */ void setWorkerId(uint8_t workerIdIn); - folly::Optional clientConnId; // Quic connection-id short version uint8_t version{0}; // Quic Host id diff --git a/quic/codec/test/TypesTest.cpp b/quic/codec/test/TypesTest.cpp index d44bc53be..ad7eebc87 100644 --- a/quic/codec/test/TypesTest.cpp +++ b/quic/codec/test/TypesTest.cpp @@ -199,13 +199,11 @@ TEST_F(TypesTest, KeyPhase) { TEST_F(TypesTest, TestConnIdWorkerId) { std::vector connIdData(kDefaultConnectionIdSize); folly::Random::secureRandom(connIdData.data(), connIdData.size()); - ConnectionId randConnId(connIdData); auto connIdAlgo = std::make_unique(); for (uint8_t i = 0; i <= 254; i++) { uint8_t processId = i % 2; uint16_t hostId = folly::Random::rand32() % 4095; ServerConnectionIdParams params(hostId, processId, i); - params.clientConnId = randConnId; auto paramsAfterEncode = connIdAlgo->parseConnectionId(connIdAlgo->encodeConnectionId(params)); EXPECT_TRUE(connIdAlgo->canParse(connIdAlgo->encodeConnectionId(params))); diff --git a/quic/loss/test/QuicLossFunctionsTest.cpp b/quic/loss/test/QuicLossFunctionsTest.cpp index 24482d9bc..a92147297 100644 --- a/quic/loss/test/QuicLossFunctionsTest.cpp +++ b/quic/loss/test/QuicLossFunctionsTest.cpp @@ -80,7 +80,6 @@ class QuicLossFunctionsTest : public TestWithParam { // create a serverConnectionId that is different from the client connId // with bits for processId and workerId set to 0 ServerConnectionIdParams params(0, 0, 0); - params.clientConnId = *conn->clientConnectionId; conn->connIdAlgo = connIdAlgo_.get(); conn->serverConnectionId = connIdAlgo_->encodeConnectionId(params); return conn; @@ -105,7 +104,6 @@ class QuicLossFunctionsTest : public TestWithParam { // create a serverConnectionId that is different from the client connId // with bits for processId and workerId set to 0 ServerConnectionIdParams params(0, 0, 0); - params.clientConnId = *conn->clientConnectionId; conn->connIdAlgo = connIdAlgo_.get(); conn->serverConnectionId = connIdAlgo_->encodeConnectionId(params); return conn; diff --git a/quic/server/state/ServerStateMachine.cpp b/quic/server/state/ServerStateMachine.cpp index 634460c27..d6ee6c731 100644 --- a/quic/server/state/ServerStateMachine.cpp +++ b/quic/server/state/ServerStateMachine.cpp @@ -501,6 +501,7 @@ void onServerReadDataFromOpen( FizzCryptoFactory(&fizzFactory) .getClientInitialCipher(initialDestinationConnectionId, version)); conn.readCodec->setClientConnectionId(clientConnectionId); + conn.readCodec->setServerConnectionId(*conn.serverConnectionId); if (conn.qLogger) { conn.qLogger->scid = conn.serverConnectionId; conn.qLogger->dcid = clientConnectionId; @@ -686,31 +687,11 @@ void onServerReadDataFromOpen( } } - // TODO: remove this when we actually negotiate connid and version - if (!conn.clientConnectionId) { - conn.clientConnectionId = folly::variant_match( - regularPacket.header, - [](const LongHeader& longHeader) { - return longHeader.getSourceConnId(); - }, - [](const ShortHeader& shortHeader) { - return shortHeader.getConnectionId(); - }); - // change the connection id when we switch - CHECK(conn.clientConnectionId); - // TODO: if conn.serverConnIdParams->clientConnId != conn.clientConnId, - // we need to update sourceAddressMap_. - // TODO: need to remove ServerConnectionIdParams::clientConnId, it is no - // longer needed. - conn.serverConnIdParams->clientConnId = *conn.clientConnectionId; - conn.readCodec->setServerConnectionId(*conn.serverConnectionId); - if (conn.qLogger) { - conn.qLogger->dcid = conn.clientConnectionId; - conn.qLogger->scid = conn.serverConnectionId; - } - } + CHECK(conn.clientConnectionId); if (conn.qLogger) { conn.qLogger->addPacket(regularPacket, packetSize); + conn.qLogger->dcid = conn.clientConnectionId; + conn.qLogger->scid = conn.serverConnectionId; } QUIC_TRACE(packet_recvd, conn, packetNum, packetSize); // We assume that the higher layer takes care of validating that the version diff --git a/quic/state/stream/test/StreamStateMachineTest.cpp b/quic/state/stream/test/StreamStateMachineTest.cpp index 399e03e9d..884b710ae 100644 --- a/quic/state/stream/test/StreamStateMachineTest.cpp +++ b/quic/state/stream/test/StreamStateMachineTest.cpp @@ -430,7 +430,6 @@ TEST_F(QuicHalfClosedRemoteStateTest, AckStream) { auto conn = createConn(); // create server chosen connId with processId = 0 and workerId = 0 ServerConnectionIdParams params(0, 0, 0); - params.clientConnId = *conn->clientConnectionId; auto connIdAlgo = std::make_unique(); folly::Optional serverChosenConnId = connIdAlgo->encodeConnectionId(params); @@ -470,7 +469,6 @@ TEST_F(QuicHalfClosedRemoteStateTest, AckStreamAfterSkip) { auto conn = createConn(); // create server chosen connId with processId = 0 and workerId = 0 ServerConnectionIdParams params(0, 0, 0); - params.clientConnId = *conn->clientConnectionId; auto connIdAlgo = std::make_unique(); folly::Optional serverChosenConnId = connIdAlgo->encodeConnectionId(params);