From 6d19c622b2100a3f0ae6d4a534dffbfa921d22ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Amaury=20S=C3=A9chet?= Date: Mon, 4 Nov 2019 16:04:53 -0800 Subject: [PATCH] Use a factory to create ClientHandshake (#59) Summary: This will allow to be able to create different kind of handshake going forward. Pull Request resolved: https://github.com/facebookincubator/mvfst/pull/59 Reviewed By: siyengar Differential Revision: D18088574 Pulled By: mjoras fbshipit-source-id: 0732bb63a9e243fef77cdaf4f76e711fcb09ecdc --- quic/api/test/IoBufQuicBatchTest.cpp | 5 +- quic/api/test/QuicPacketSchedulerTest.cpp | 55 +++++++++++++------ quic/client/CMakeLists.txt | 2 + quic/client/QuicClientTransport.cpp | 6 +- quic/client/QuicClientTransport.h | 2 +- .../client/handshake/ClientHandshakeFactory.h | 32 +++++++++++ quic/client/handshake/FizzClientHandshake.cpp | 18 ++++++ quic/client/handshake/FizzClientHandshake.h | 27 +++++++++ .../FizzClientQuicHandshakeContext.cpp | 21 +++++++ .../FizzClientQuicHandshakeContext.h | 25 +++++++++ quic/client/state/ClientStateMachine.cpp | 3 +- quic/client/state/ClientStateMachine.h | 14 ++++- quic/client/test/QuicClientTransportTest.cpp | 2 +- quic/loss/test/QuicLossFunctionsTest.cpp | 4 +- quic/state/test/QuicStreamFunctionsTest.cpp | 10 +++- 15 files changed, 196 insertions(+), 30 deletions(-) create mode 100644 quic/client/handshake/ClientHandshakeFactory.h create mode 100644 quic/client/handshake/FizzClientHandshake.cpp create mode 100644 quic/client/handshake/FizzClientHandshake.h create mode 100644 quic/client/handshake/FizzClientQuicHandshakeContext.cpp create mode 100644 quic/client/handshake/FizzClientQuicHandshakeContext.h diff --git a/quic/api/test/IoBufQuicBatchTest.cpp b/quic/api/test/IoBufQuicBatchTest.cpp index 9ced916e9..dc810dc6b 100644 --- a/quic/api/test/IoBufQuicBatchTest.cpp +++ b/quic/api/test/IoBufQuicBatchTest.cpp @@ -7,7 +7,9 @@ */ #include + #include +#include #include #include @@ -53,7 +55,8 @@ void RunTest(int numBatch) { auto batchWriter = std::make_unique(numBatch); folly::SocketAddress peerAddress{"127.0.0.1", 1234}; - QuicClientConnectionState conn; + QuicClientConnectionState conn( + std::make_shared()); QuicConnectionStateBase::HappyEyeballsState happyEyeballsState; IOBufQuicBatch ioBufBatch( diff --git a/quic/api/test/QuicPacketSchedulerTest.cpp b/quic/api/test/QuicPacketSchedulerTest.cpp index 1f0d0cccb..742c2639e 100644 --- a/quic/api/test/QuicPacketSchedulerTest.cpp +++ b/quic/api/test/QuicPacketSchedulerTest.cpp @@ -11,6 +11,7 @@ #include #include +#include #include #include #include @@ -115,7 +116,8 @@ TEST_F(QuicPacketSchedulerTest, NoopScheduler) { } TEST_F(QuicPacketSchedulerTest, CryptoPaddingInitialPacket) { - QuicClientConnectionState conn; + QuicClientConnectionState conn( + std::make_shared()); auto connId = getTestConnectionId(); LongHeader longHeader1( LongHeader::Types::Initial, @@ -174,7 +176,8 @@ TEST_F(QuicPacketSchedulerTest, CryptoServerInitialNotPadded) { } TEST_F(QuicPacketSchedulerTest, CryptoPaddingRetransmissionClientInitial) { - QuicClientConnectionState conn; + QuicClientConnectionState conn( + std::make_shared()); auto connId = getTestConnectionId(); LongHeader longHeader( LongHeader::Types::Initial, @@ -221,7 +224,8 @@ TEST_F(QuicPacketSchedulerTest, CryptoSchedulerOnlySingleLossFits) { } TEST_F(QuicPacketSchedulerTest, CryptoWritePartialLossBuffer) { - QuicClientConnectionState conn; + QuicClientConnectionState conn( + std::make_shared()); auto connId = getTestConnectionId(); LongHeader longHeader( LongHeader::Types::Initial, @@ -308,7 +312,8 @@ TEST_F(QuicPacketSchedulerTest, StreamFrameSchedulerStreamNotExists) { } TEST_F(QuicPacketSchedulerTest, CloningSchedulerTest) { - QuicClientConnectionState conn; + QuicClientConnectionState conn( + std::make_shared()); FrameScheduler noopScheduler("frame"); ASSERT_FALSE(noopScheduler.hasData()); CloningScheduler cloningScheduler(noopScheduler, conn, "CopyCat", 0); @@ -335,7 +340,8 @@ TEST_F(QuicPacketSchedulerTest, CloningSchedulerTest) { } TEST_F(QuicPacketSchedulerTest, WriteOnlyOutstandingPacketsTest) { - QuicClientConnectionState conn; + QuicClientConnectionState conn( + std::make_shared()); FrameScheduler noopScheduler("frame"); ASSERT_FALSE(noopScheduler.hasData()); CloningScheduler cloningScheduler(noopScheduler, conn, "CopyCat", 0); @@ -402,7 +408,8 @@ TEST_F(QuicPacketSchedulerTest, WriteOnlyOutstandingPacketsTest) { } TEST_F(QuicPacketSchedulerTest, DoNotCloneProcessedClonedPacket) { - QuicClientConnectionState conn; + QuicClientConnectionState conn( + std::make_shared()); FrameScheduler noopScheduler("frame"); CloningScheduler cloningScheduler(noopScheduler, conn, "CopyCat", 0); // Add two outstanding packets, but then mark the second one processed by @@ -432,7 +439,8 @@ TEST_F(QuicPacketSchedulerTest, DoNotCloneProcessedClonedPacket) { } TEST_F(QuicPacketSchedulerTest, DoNotClonePureAck) { - QuicClientConnectionState conn; + QuicClientConnectionState conn( + std::make_shared()); FrameScheduler noopScheduler("frame"); CloningScheduler cloningScheduler(noopScheduler, conn, "CopyCat", 0); // Add two outstanding packets, with second one being pureAck @@ -458,7 +466,8 @@ TEST_F(QuicPacketSchedulerTest, DoNotClonePureAck) { } TEST_F(QuicPacketSchedulerTest, CloneSchedulerHasDataIgnoresNonAppData) { - QuicClientConnectionState conn; + QuicClientConnectionState conn( + std::make_shared()); FrameScheduler noopScheduler("frame"); CloningScheduler cloningScheduler(noopScheduler, conn, "CopyCat", 0); EXPECT_FALSE(cloningScheduler.hasData()); @@ -474,7 +483,8 @@ TEST_F(QuicPacketSchedulerTest, CloneSchedulerHasDataIgnoresNonAppData) { } TEST_F(QuicPacketSchedulerTest, DoNotCloneHandshake) { - QuicClientConnectionState conn; + QuicClientConnectionState conn( + std::make_shared()); FrameScheduler noopScheduler("frame"); CloningScheduler cloningScheduler(noopScheduler, conn, "CopyCat", 0); // Add two outstanding packets, with second one being handshake @@ -501,7 +511,8 @@ TEST_F(QuicPacketSchedulerTest, DoNotCloneHandshake) { } TEST_F(QuicPacketSchedulerTest, CloneSchedulerUseNormalSchedulerFirst) { - QuicClientConnectionState conn; + QuicClientConnectionState conn( + std::make_shared()); MockFrameScheduler mockScheduler; CloningScheduler cloningScheduler(mockScheduler, conn, "Mocker", 0); ShortHeader header( @@ -549,7 +560,8 @@ TEST_F(QuicPacketSchedulerTest, CloneSchedulerUseNormalSchedulerFirst) { } TEST_F(QuicPacketSchedulerTest, CloneWillGenerateNewWindowUpdate) { - QuicClientConnectionState conn; + QuicClientConnectionState conn( + std::make_shared()); conn.streamManager->setMaxLocalBidirectionalStreams(10); auto stream = conn.streamManager->createNextBidirectionalStream().value(); FrameScheduler noopScheduler("frame"); @@ -630,7 +642,8 @@ TEST_F(QuicPacketSchedulerTest, CloneWillGenerateNewWindowUpdate) { class AckSchedulingTest : public TestWithParam {}; TEST_F(QuicPacketSchedulerTest, AckStateHasAcksToSchedule) { - QuicClientConnectionState conn; + QuicClientConnectionState conn( + std::make_shared()); EXPECT_FALSE(hasAcksToSchedule(conn.ackStates.initialAckState)); EXPECT_FALSE(hasAcksToSchedule(conn.ackStates.handshakeAckState)); EXPECT_FALSE(hasAcksToSchedule(conn.ackStates.appDataAckState)); @@ -647,7 +660,8 @@ TEST_F(QuicPacketSchedulerTest, AckStateHasAcksToSchedule) { } TEST_F(QuicPacketSchedulerTest, AckSchedulerHasAcksToSchedule) { - QuicClientConnectionState conn; + QuicClientConnectionState conn( + std::make_shared()); AckScheduler initialAckScheduler( conn, getAckState(conn, PacketNumberSpace::Initial)); AckScheduler handshakeAckScheduler( @@ -670,7 +684,8 @@ TEST_F(QuicPacketSchedulerTest, AckSchedulerHasAcksToSchedule) { } TEST_F(QuicPacketSchedulerTest, LargestAckToSend) { - QuicClientConnectionState conn; + QuicClientConnectionState conn( + std::make_shared()); EXPECT_EQ(folly::none, largestAckToSend(conn.ackStates.initialAckState)); EXPECT_EQ(folly::none, largestAckToSend(conn.ackStates.handshakeAckState)); EXPECT_EQ(folly::none, largestAckToSend(conn.ackStates.appDataAckState)); @@ -685,7 +700,8 @@ TEST_F(QuicPacketSchedulerTest, LargestAckToSend) { } TEST_F(QuicPacketSchedulerTest, StreamFrameSchedulerAllFit) { - QuicClientConnectionState conn; + QuicClientConnectionState conn( + std::make_shared()); conn.streamManager->setMaxLocalBidirectionalStreams(10); conn.flowControlState.peerAdvertisedMaxOffset = 100000; conn.flowControlState.peerAdvertisedInitialMaxStreamOffsetBidiRemote = 100000; @@ -710,7 +726,8 @@ TEST_F(QuicPacketSchedulerTest, StreamFrameSchedulerAllFit) { } TEST_F(QuicPacketSchedulerTest, StreamFrameSchedulerRoundRobin) { - QuicClientConnectionState conn; + QuicClientConnectionState conn( + std::make_shared()); conn.streamManager->setMaxLocalBidirectionalStreams(10); conn.flowControlState.peerAdvertisedMaxOffset = 100000; conn.flowControlState.peerAdvertisedInitialMaxStreamOffsetBidiRemote = 100000; @@ -827,7 +844,8 @@ TEST_F(QuicPacketSchedulerTest, StreamFrameSchedulerRoundRobinControl) { } TEST_F(QuicPacketSchedulerTest, StreamFrameSchedulerOneStream) { - QuicClientConnectionState conn; + QuicClientConnectionState conn( + std::make_shared()); conn.streamManager->setMaxLocalBidirectionalStreams(10); conn.flowControlState.peerAdvertisedMaxOffset = 100000; conn.flowControlState.peerAdvertisedInitialMaxStreamOffsetBidiRemote = 100000; @@ -848,7 +866,8 @@ TEST_F(QuicPacketSchedulerTest, StreamFrameSchedulerOneStream) { } TEST_F(QuicPacketSchedulerTest, StreamFrameSchedulerRemoveOne) { - QuicClientConnectionState conn; + QuicClientConnectionState conn( + std::make_shared()); conn.streamManager->setMaxLocalBidirectionalStreams(10); conn.flowControlState.peerAdvertisedMaxOffset = 100000; conn.flowControlState.peerAdvertisedInitialMaxStreamOffsetBidiRemote = 100000; diff --git a/quic/client/CMakeLists.txt b/quic/client/CMakeLists.txt index fed3297bb..10c1162a9 100644 --- a/quic/client/CMakeLists.txt +++ b/quic/client/CMakeLists.txt @@ -7,6 +7,8 @@ add_library( mvfst_client STATIC QuicClientTransport.cpp handshake/ClientHandshake.cpp + handshake/FizzClientQuicHandshakeContext.cpp + handshake/FizzClientHandshake.cpp state/ClientStateMachine.cpp ) diff --git a/quic/client/QuicClientTransport.cpp b/quic/client/QuicClientTransport.cpp index c51af2a0b..20ac3fb84 100644 --- a/quic/client/QuicClientTransport.cpp +++ b/quic/client/QuicClientTransport.cpp @@ -12,6 +12,7 @@ #include #include +#include #include #include #include @@ -40,7 +41,8 @@ QuicClientTransport::QuicClientTransport( connectionIdSize == 0 || (connectionIdSize >= kMinInitialDestinationConnIdLength && connectionIdSize <= kMaxConnectionIdSize)); - auto tempConn = std::make_unique(); + auto tempConn = std::make_unique( + std::make_shared()); clientConn_ = tempConn.get(); conn_ = std::move(tempConn); std::vector connIdData( @@ -1168,7 +1170,7 @@ void QuicClientTransport::setHostname(const std::string& hostname) { hostname_ = hostname; } -void QuicClientTransport::setFizzClientContext( +void QuicClientTransport::setFizzClientQuicHandshakeContext( std::shared_ptr ctx) { ctx_ = std::move(ctx); } diff --git a/quic/client/QuicClientTransport.h b/quic/client/QuicClientTransport.h index c9060944e..51ba6b784 100644 --- a/quic/client/QuicClientTransport.h +++ b/quic/client/QuicClientTransport.h @@ -62,7 +62,7 @@ class QuicClientTransport /** * Set the client context for fizz. Must be set before start() */ - void setFizzClientContext( + void setFizzClientQuicHandshakeContext( std::shared_ptr ctx); /** diff --git a/quic/client/handshake/ClientHandshakeFactory.h b/quic/client/handshake/ClientHandshakeFactory.h new file mode 100644 index 000000000..453951099 --- /dev/null +++ b/quic/client/handshake/ClientHandshakeFactory.h @@ -0,0 +1,32 @@ +/* + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + */ + +#pragma once + +#include + +namespace quic { + +class ClientHandshake; +struct QuicCryptoState; + +class ClientHandshakeFactory { + public: + virtual ~ClientHandshakeFactory() = default; + + /** + * Construct a new client handshake. + * /!\ The ClientHandshake constructed might keep a reference to cryptoState. + * It is up to the caller to ensure the lifetime of cryptoState exceed the one + * of the ClientHandshake. + */ + virtual std::unique_ptr makeClientHandshake( + QuicCryptoState& cryptoState) = 0; +}; + +} // namespace quic diff --git a/quic/client/handshake/FizzClientHandshake.cpp b/quic/client/handshake/FizzClientHandshake.cpp new file mode 100644 index 000000000..d8bb24b1d --- /dev/null +++ b/quic/client/handshake/FizzClientHandshake.cpp @@ -0,0 +1,18 @@ +/* + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + */ + +#include + +namespace quic { + +FizzClientHandshake::FizzClientHandshake( + QuicCryptoState& cryptoState, + std::shared_ptr fizzContext) + : ClientHandshake(cryptoState), fizzContext_(std::move(fizzContext)) {} + +} // namespace quic diff --git a/quic/client/handshake/FizzClientHandshake.h b/quic/client/handshake/FizzClientHandshake.h new file mode 100644 index 000000000..2d8a366f5 --- /dev/null +++ b/quic/client/handshake/FizzClientHandshake.h @@ -0,0 +1,27 @@ +/* + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + */ + +#pragma once + +#include + +namespace quic { + +class FizzClientQuicHandshakeContext; + +class FizzClientHandshake : public ClientHandshake { + public: + FizzClientHandshake( + QuicCryptoState& cryptoState, + std::shared_ptr fizzContext); + + private: + std::shared_ptr fizzContext_; +}; + +} // namespace quic diff --git a/quic/client/handshake/FizzClientQuicHandshakeContext.cpp b/quic/client/handshake/FizzClientQuicHandshakeContext.cpp new file mode 100644 index 000000000..8990262d7 --- /dev/null +++ b/quic/client/handshake/FizzClientQuicHandshakeContext.cpp @@ -0,0 +1,21 @@ +/* + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + */ + +#include + +#include + +namespace quic { + +std::unique_ptr +FizzClientQuicHandshakeContext::makeClientHandshake( + QuicCryptoState& cryptoState) { + return std::make_unique(cryptoState, shared_from_this()); +} + +} // namespace quic diff --git a/quic/client/handshake/FizzClientQuicHandshakeContext.h b/quic/client/handshake/FizzClientQuicHandshakeContext.h new file mode 100644 index 000000000..99d7ce2b0 --- /dev/null +++ b/quic/client/handshake/FizzClientQuicHandshakeContext.h @@ -0,0 +1,25 @@ +/* + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + */ + +#pragma once + +#include + +namespace quic { + +class FizzClientHandshake; + +class FizzClientQuicHandshakeContext + : public ClientHandshakeFactory, + public std::enable_shared_from_this { + public: + std::unique_ptr makeClientHandshake( + QuicCryptoState& cryptoState) override; +}; + +} // namespace quic diff --git a/quic/client/state/ClientStateMachine.cpp b/quic/client/state/ClientStateMachine.cpp index 6e944744c..48e5dd79f 100644 --- a/quic/client/state/ClientStateMachine.cpp +++ b/quic/client/state/ClientStateMachine.cpp @@ -22,7 +22,8 @@ std::unique_ptr undoAllClientStateCommon( std::unique_ptr conn) { // Create a new connection state and copy over properties that don't change // across stateless retry. - auto newConn = std::make_unique(); + auto newConn = std::make_unique( + std::move(conn->handshakeFactory)); newConn->qLogger = conn->qLogger; newConn->clientConnectionId = conn->clientConnectionId; newConn->initialDestinationConnectionId = diff --git a/quic/client/state/ClientStateMachine.h b/quic/client/state/ClientStateMachine.h index 388eabde5..af73c26d4 100644 --- a/quic/client/state/ClientStateMachine.h +++ b/quic/client/state/ClientStateMachine.h @@ -10,6 +10,7 @@ #include #include +#include #include #include #include @@ -30,6 +31,7 @@ struct QuicClientConnectionState : public QuicConnectionStateBase { // Initial destination connection id. folly::Optional initialDestinationConnectionId; + std::shared_ptr handshakeFactory; ClientHandshake* clientHandshakeLayer; // Save the server transport params here so that client can access the value @@ -43,15 +45,21 @@ struct QuicClientConnectionState : public QuicConnectionStateBase { // TODO: use this to get rid of the data in the crypto stream. // folly::Optional clientInitialPacketNum; - QuicClientConnectionState() : QuicConnectionStateBase(QuicNodeType::Client) { + explicit QuicClientConnectionState( + std::shared_ptr handshakeFactoryIn) + : QuicConnectionStateBase(QuicNodeType::Client), + handshakeFactory(std::move(handshakeFactoryIn)) { cryptoState = std::make_unique(); congestionController = std::make_unique(*this); // TODO: this is wrong, it should be the handshake finish time. But i need // a relatively sane time now to make the timestamps all sane. connectionTime = Clock::now(); originalVersion = QuicVersion::MVFST; - clientHandshakeLayer = new ClientHandshake(*cryptoState); - handshakeLayer.reset(clientHandshakeLayer); + DCHECK(handshakeFactory); + auto tmpClientHandshake = + handshakeFactory->makeClientHandshake(*cryptoState); + clientHandshakeLayer = tmpClientHandshake.get(); + handshakeLayer.reset(tmpClientHandshake.release()); // We shouldn't normally need to set this until we're starting the // transport, however writing unit tests is much easier if we set this here. updateFlowControlStateWithSettings(flowControlState, transportSettings); diff --git a/quic/client/test/QuicClientTransportTest.cpp b/quic/client/test/QuicClientTransportTest.cpp index 72fd39c18..ec4ee2075 100644 --- a/quic/client/test/QuicClientTransportTest.cpp +++ b/quic/client/test/QuicClientTransportTest.cpp @@ -203,7 +203,7 @@ class QuicClientTransportIntegrationTest : public TestWithParam { client->setCongestionControllerFactory( std::make_shared()); client->setHostname(hostname); - client->setFizzClientContext(clientCtx); + client->setFizzClientQuicHandshakeContext(clientCtx); client->setCertificateVerifier(createTestCertificateVerifier()); client->addNewPeerAddress(serverAddr); client->setPskCache(pskCache_); diff --git a/quic/loss/test/QuicLossFunctionsTest.cpp b/quic/loss/test/QuicLossFunctionsTest.cpp index 8ea9bb966..593aa96d5 100644 --- a/quic/loss/test/QuicLossFunctionsTest.cpp +++ b/quic/loss/test/QuicLossFunctionsTest.cpp @@ -13,6 +13,7 @@ #include #include #include +#include #include #include #include @@ -85,7 +86,8 @@ class QuicLossFunctionsTest : public TestWithParam { } std::unique_ptr createClientConn() { - auto conn = std::make_unique(); + auto conn = std::make_unique( + std::make_shared()); conn->clientConnectionId = getTestConnectionId(); conn->version = QuicVersion::MVFST; conn->ackStates.initialAckState.nextPacketNum = 1; diff --git a/quic/state/test/QuicStreamFunctionsTest.cpp b/quic/state/test/QuicStreamFunctionsTest.cpp index 5281315fd..59466d7eb 100644 --- a/quic/state/test/QuicStreamFunctionsTest.cpp +++ b/quic/state/test/QuicStreamFunctionsTest.cpp @@ -11,6 +11,7 @@ #include +#include #include #include #include @@ -27,6 +28,9 @@ using PeekIterator = std::deque::const_iterator; class QuicStreamFunctionsTest : public Test { public: + QuicStreamFunctionsTest() + : conn(std::make_shared()) {} + void SetUp() override { conn.flowControlState.peerAdvertisedInitialMaxStreamOffsetBidiLocal = kDefaultStreamWindowSize; @@ -1083,7 +1087,8 @@ TEST_F(QuicStreamFunctionsTest, IsBidirectionalStream) { } TEST_F(QuicStreamFunctionsTest, IsSendingStream) { - QuicClientConnectionState clientState; + QuicClientConnectionState clientState( + std::make_shared()); QuicServerConnectionState serverState; QuicNodeType nodeType; StreamId id; @@ -1110,7 +1115,8 @@ TEST_F(QuicStreamFunctionsTest, IsSendingStream) { } TEST_F(QuicStreamFunctionsTest, IsReceivingStream) { - QuicClientConnectionState clientState; + QuicClientConnectionState clientState( + std::make_shared()); QuicServerConnectionState serverState; QuicNodeType nodeType; StreamId id;