diff:
@@ -27,7 +27,6 @@ find_package(Boost 1.62
context
)
find_package(folly REQUIRED)
-find_package(Fizz REQUIRED)
find_package(Glog REQUIRED)
find_package(Threads)
@@ -75,6 +74,7 @@ list(APPEND
-Wnon-virtual-dtor
)
+include(BuildFizz)
if(BUILD_TESTS)
enable_testing()
include(QuicTest)

diff:
@@ -55,13 +55,9 @@ mkdir -p "$MVFST_BUILD_DIR"
if [ -z "${INSTALL_PREFIX-}" ]; then
FOLLY_INSTALL_DIR=$DEPS_DIR
- FIZZ_INSTALL_DIR=$DEPS_DIR
- GTEST_INSTALL_DIR=$DEPS_DIR
MVFST_INSTALL_DIR=$BWD
else
FOLLY_INSTALL_DIR=$INSTALL_PREFIX
- FIZZ_INSTALL_DIR=$INSTALL_PREFIX
- GTEST_INSTALL_DIR=$INSTALL_PREFIX
MVFST_INSTALL_DIR=$INSTALL_PREFIX
fi
@@ -143,27 +139,6 @@ function setup_folly() {
cd "$BWD" || exit
}
-function setup_fizz() {
- FIZZ_DIR=$DEPS_DIR/fizz
- FIZZ_BUILD_DIR=$DEPS_DIR/fizz/build/
- if [ ! -d "$FIZZ_DIR" ] ; then
- echo -e "${COLOR_GREEN}[ INFO ] Cloning fizz repo ${COLOR_OFF}"
- git clone https://github.com/facebookincubator/fizz "$FIZZ_DIR"
- echo -e "${COLOR_GREEN}[ INFO ] install dependencies ${COLOR_OFF}"
- fi
- echo -e "${COLOR_GREEN}Building Fizz ${COLOR_OFF}"
- mkdir -p "$FIZZ_BUILD_DIR"
- cd "$FIZZ_BUILD_DIR" || exit
- cmake -DCMAKE_BUILD_TYPE=RelWithDebInfo \
- -DCMAKE_PREFIX_PATH="$FIZZ_INSTALL_DIR" \
- -DCMAKE_INSTALL_PREFIX="$FIZZ_INSTALL_DIR" \
- "$FIZZ_DIR/fizz"
- make -j "$(nproc)"
- make install
- echo -e "${COLOR_GREEN}Folly is installed ${COLOR_OFF}"
- cd "$BWD" || exit
-}
-
function detect_platform() {
unameOut="$(uname -s)"
case "${unameOut}" in
@@ -176,7 +151,6 @@ function detect_platform() {
detect_platform
setup_folly
-setup_fizz
# build mvfst:
cd "$MVFST_BUILD_DIR" || exit
@@ -184,7 +158,6 @@ cmake -DCMAKE_PREFIX_PATH="$FOLLY_INSTALL_DIR" \
-DCMAKE_INSTALL_PREFIX="$MVFST_INSTALL_DIR" \
-DCMAKE_BUILD_TYPE=RelWithDebInfo \
-DBUILD_TESTS=On \
- -DFIZZ_PROJECT="$DEPS_DIR/fizz" \
../..
make -j "$(nproc)"
echo -e "${COLOR_GREEN}MVFST build is complete. To run unit test: \

diff:
+# 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(ExternalProject)
+# Construct platform aware cmake args since on OSX
+# it requires path to the openssl libs
+list(APPEND CMAKE_ARGS
+ -DCMAKE_BUILD_TYPE:STRING=RelWithDebInfo
+ -DCMAKE_PREFIX_PATH:STRING=${CMAKE_PREFIX_PATH}
+)
+if(UNIX AND APPLE)
+ list(APPEND CMAKE_ARGS
+ -DOPENSSL_ROOT_DIR:STRING=${OPENSSL_ROOT_DIR}
+ -DOPENSSL_LIBRARIES:STRING=${OPENSSL_LIBRARIES}
+)
+endif()
+
+# Consume Fizz as an external project
+ExternalProject_Add(
+ fizz_project
+ GIT_REPOSITORY https://github.com/facebookincubator/fizz
+ PREFIX fizz_project
+ SOURCE_SUBDIR fizz
+ # Disable install step
+ INSTALL_COMMAND ""
+ LOG_DOWNLOAD ON
+ LOG_UPDATE 1
+ LOG_CONFIGURE ON
+ LOG_BUILD ON
+ LOG_TEST 1
+ LOG_INSTALL 1
+ CMAKE_CACHE_ARGS ${CMAKE_ARGS}
+)
+set(FIZZ_TARGET fizz_project)
+ExternalProject_Get_Property(fizz_project source_dir)
+set(FIZZ_SOURCE_DIR ${source_dir})
+set(FIZZ_PROJECT ${source_dir})
+ExternalProject_Get_Property(fizz_project binary_dir)
+set(FIZZ_BINARY_DIR ${binary_dir})
+
+# Setup fizz libraries and include dirs
+set(LIBFIZZ_LIBRARY
+ "${FIZZ_BINARY_DIR}/${CMAKE_CFG_INTDIR}/lib/${CMAKE_STATIC_LIBRARY_PREFIX}fizz${CMAKE_STATIC_LIBRARY_SUFFIX}"
+)
+set(LIBFIZZ_INCLUDE_DIR "${source_dir}")

diff:
@@ -70,7 +70,8 @@ function(quic_add_test)
# implementation of 'main()' that calls folly::init
"${QUIC_FBCODE_ROOT}/quic/common/test/TestMain.cpp"
)
- add_dependencies(${QUIC_TEST_TARGET} googletest) # make sure googletest is built first
+ # make sure googletest and fizz are built first
+ add_dependencies(${QUIC_TEST_TARGET} googletest ${FIZZ_TARGET})
target_compile_options(
${QUIC_TEST_TARGET} PRIVATE
${_QUIC_BASE_COMPILE_OPTIONS}

diff:
@@ -15,6 +15,7 @@ add_library(
target_include_directories(
mvfst_transport PUBLIC
$<BUILD_INTERFACE:${QUIC_FBCODE_ROOT}>
+ $<BUILD_INTERFACE:${LIBFIZZ_INCLUDE_DIR}>
$<INSTALL_INTERFACE:include/>
)
@@ -26,6 +27,7 @@ target_compile_options(
add_dependencies(
mvfst_transport
+ ${FIZZ_TARGET}
mvfst_cc_algo
mvfst_codec
mvfst_codec_pktbuilder
@@ -48,7 +50,7 @@ add_dependencies(
target_link_libraries(
mvfst_transport PUBLIC
Folly::folly
- fizz::fizz
+ ${LIBFIZZ_LIBRARY}
mvfst_cc_algo
mvfst_codec
mvfst_codec_pktbuilder

diff:
+/*
+ * 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.
+ *
+ */
+
+// Copied from
+// https://github.com/facebookincubator/fizz/blob/master/fizz/crypto/aead/test/TestUtil.cpp
+#include <fizz/crypto/aead/test/TestUtil.h>
+
+#include <fizz/crypto/aead/IOBufUtil.h>
+
+using namespace folly;
+
+namespace fizz {
+namespace test {
+
+std::unique_ptr<folly::IOBuf> defaultCreator(size_t len, size_t) {
+ return IOBuf::create(len);
+}
+
+// Converts the hex encoded string to an IOBuf.
+std::unique_ptr<folly::IOBuf>
+toIOBuf(std::string hexData, size_t headroom, size_t tailroom) {
+ std::string out;
+ CHECK(folly::unhexlify(hexData, out));
+ return folly::IOBuf::copyBuffer(out, headroom, tailroom);
+}
+
+std::unique_ptr<IOBuf>
+chunkIOBuf(std::unique_ptr<IOBuf> input, size_t chunks, BufCreator creator) {
+ if (!creator) {
+ creator = defaultCreator;
+ }
+ // create IOBuf chunks
+ size_t inputLen = input->computeChainDataLength();
+ size_t chunkLen = floor((double)inputLen / (double)chunks);
+ std::unique_ptr<IOBuf> chunked;
+
+ for (size_t i = 0; i < chunks - 1; ++i) {
+ auto buf = creator(chunkLen, i);
+ buf->append(chunkLen);
+ if (!chunked) {
+ chunked = std::move(buf);
+ } else {
+ chunked->prependChain(std::move(buf));
+ }
+ }
+
+ size_t remainLen = inputLen - (chunks - 1) * chunkLen;
+ auto remain = creator(remainLen, chunks - 1);
+ remain->append(remainLen);
+ chunked->prependChain(std::move(remain));
+
+ transformBuffer(
+ *input, *chunked, [](uint8_t* out, const uint8_t* in, size_t len) {
+ memcpy(out, in, len);
+ });
+
+ CHECK_EQ(chunks, chunked->countChainElements());
+ return chunked;
+}
+} // namespace test
+} // namespace fizz

diff:
@@ -10,13 +10,14 @@ endif()
add_library(
mvfst_test_utils STATIC
TestUtils.cpp
- ${FIZZ_PROJECT}/fizz/crypto/aead/test/TestUtil.cpp
- ${FIZZ_PROJECT}/fizz/crypto/test/TestUtil.cpp
+ AeadTestUtil.cpp
+ CryptoTestUtil.cpp
)
+
target_include_directories(
mvfst_test_utils PUBLIC
$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}>
- $<BUILD_INTERFACE:${FIZZ_PROJECT}>
+ $<BUILD_INTERFACE:${LIBFIZZ_INCLUDE_DIR}>
${LIBGMOCK_INCLUDE_DIR}
${LIBGTEST_INCLUDE_DIR}
)
@@ -29,6 +30,7 @@ target_compile_options(
add_dependencies(
mvfst_test_utils
+ ${FIZZ_TARGET}
mvfst_client
mvfst_codec_pktbuilder
mvfst_codec_types
@@ -41,7 +43,7 @@ add_dependencies(
target_link_libraries(
mvfst_test_utils PUBLIC
Folly::folly
- fizz::fizz
+ ${LIBFIZZ_LIBRARY}
${LIBGMOCK_LIBRARIES}
${LIBGTEST_LIBRARIES}
mvfst_client
@@ -62,7 +64,7 @@ quic_add_test(TARGET QuicCommonUtilTest SOURCES
IntervalSetTest.cpp
DEPENDS
Folly::folly
- fizz::fizz
+ ${LIBFIZZ_LIBRARY}
mvfst_client
mvfst_codec_pktbuilder
mvfst_codec_types

diff:
+/*
+ * 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.
+ *
+ */
+
+// Copied from
+// https://github.com/facebookincubator/fizz/blob/master/fizz/crypto/test/TestUtil.cpp
+#include <fizz/crypto/test/TestUtil.h>
+
+#include <folly/String.h>
+#include <folly/ssl/OpenSSLCertUtils.h>
+#include <sodium/randombytes.h>
+
+using namespace folly;
+using namespace folly::ssl;
+
+namespace fizz {
+namespace test {
+
+EvpPkeyUniquePtr getPrivateKey(StringPiece key) {
+ BioUniquePtr bio(BIO_new(BIO_s_mem()));
+ CHECK(bio);
+ CHECK_EQ(BIO_write(bio.get(), key.data(), key.size()), key.size());
+ EvpPkeyUniquePtr pkey(
+ PEM_read_bio_PrivateKey(bio.get(), nullptr, nullptr, nullptr));
+ CHECK(pkey);
+ return pkey;
+}
+
+EvpPkeyUniquePtr getPublicKey(StringPiece key) {
+ BioUniquePtr bio(BIO_new(BIO_s_mem()));
+ CHECK(bio);
+ CHECK_EQ(BIO_write(bio.get(), key.data(), key.size()), key.size());
+ EvpPkeyUniquePtr pkey(
+ PEM_read_bio_PUBKEY(bio.get(), nullptr, nullptr, nullptr));
+ CHECK(pkey);
+ return pkey;
+}
+
+// Converts the hex encoded string to an IOBuf.
+std::unique_ptr<folly::IOBuf> toIOBuf(folly::StringPiece hexData) {
+ std::string out;
+ CHECK(folly::unhexlify(hexData, out));
+ return folly::IOBuf::copyBuffer(out);
+}
+
+folly::ssl::X509UniquePtr getCert(folly::StringPiece cert) {
+ BioUniquePtr bio(BIO_new(BIO_s_mem()));
+ CHECK(bio);
+ CHECK_EQ(BIO_write(bio.get(), cert.data(), cert.size()), cert.size());
+ X509UniquePtr x509(PEM_read_bio_X509(bio.get(), nullptr, nullptr, nullptr));
+ CHECK(x509);
+ return x509;
+}
+
+std::unique_ptr<folly::IOBuf> getCertData(folly::StringPiece cert) {
+ return OpenSSLCertUtils::derEncode(*getCert(cert));
+}
+
+static struct randombytes_implementation mockRandom = {
+ []() { return "test"; }, // implementation_name
+ []() { return (uint32_t)0x44444444; }, // random
+ nullptr, // stir
+ nullptr, // uniform
+ [](void* const buf, const size_t size) { memset(buf, 0x44, size); }, // buf
+ nullptr}; // close
+
+void useMockRandom() {
+ randombytes_set_implementation(&mockRandom);
+}
+} // namespace test
+} // namespace fizz

diff:
@@ -15,6 +15,7 @@ add_library(
target_include_directories(
mvfst_cc_algo PUBLIC
$<BUILD_INTERFACE:${QUIC_FBCODE_ROOT}>
+ $<BUILD_INTERFACE:${LIBFIZZ_INCLUDE_DIR}>
$<INSTALL_INTERFACE:include/>
)
@@ -26,6 +27,7 @@ target_compile_options(
add_dependencies(
mvfst_cc_algo
+ ${FIZZ_TARGET}
mvfst_constants
mvfst_exception
mvfst_state_machine
@@ -35,7 +37,7 @@ add_dependencies(
target_link_libraries(
mvfst_cc_algo PUBLIC
Folly::folly
- fizz::fizz
+ ${LIBFIZZ_LIBRARY}
mvfst_constants
mvfst_exception
mvfst_state_machine

diff:
@@ -13,6 +13,7 @@ add_library(
target_include_directories(
mvfst_handshake PUBLIC
$<BUILD_INTERFACE:${QUIC_FBCODE_ROOT}>
+ $<BUILD_INTERFACE:${LIBFIZZ_INCLUDE_DIR}>
$<INSTALL_INTERFACE:include/>
)
@@ -24,6 +25,7 @@ target_compile_options(
add_dependencies(
mvfst_handshake
+ ${FIZZ_TARGET}
mvfst_constants
mvfst_exception
mvfst_codec_types
@@ -33,7 +35,7 @@ add_dependencies(
target_link_libraries(
mvfst_handshake PUBLIC
Folly::folly
- fizz::fizz
+ ${LIBFIZZ_LIBRARY}
mvfst_constants
mvfst_exception
mvfst_codec_types

diff:
@@ -11,6 +11,7 @@ add_library(
target_include_directories(
mvfst_happyeyeballs PUBLIC
$<BUILD_INTERFACE:${QUIC_FBCODE_ROOT}>
+ $<BUILD_INTERFACE:${LIBFIZZ_INCLUDE_DIR}>
$<INSTALL_INTERFACE:include/>
)
@@ -22,13 +23,14 @@ target_compile_options(
add_dependencies(
mvfst_happyeyeballs
+ ${FIZZ_TARGET}
mvfst_state_machine
)
target_link_libraries(
mvfst_happyeyeballs PUBLIC
Folly::folly
- fizz::fizz
+ ${LIBFIZZ_LIBRARY}
mvfst_state_machine
)

diff:
@@ -19,6 +19,7 @@ add_library(
target_include_directories(
mvfst_server PUBLIC
$<BUILD_INTERFACE:${QUIC_FBCODE_ROOT}>
+ $<BUILD_INTERFACE:${LIBFIZZ_INCLUDE_DIR}>
$<INSTALL_INTERFACE:include/>
)
@@ -30,6 +31,7 @@ target_compile_options(
add_dependencies(
mvfst_server
+ ${FIZZ_TARGET}
mvfst_constants
mvfst_codec
mvfst_codec_types
@@ -40,7 +42,7 @@ add_dependencies(
target_link_libraries(
mvfst_server PUBLIC
Folly::folly
- fizz::fizz
+ ${LIBFIZZ_LIBRARY}
mvfst_constants
mvfst_codec
mvfst_codec_types

diff:
@@ -16,7 +16,7 @@ quic_add_test(TARGET ServerHandshakeTest
StatelessResetGeneratorTest.cpp
DEPENDS
Folly::folly
- fizz::fizz
+ ${LIBFIZZ_LIBRARY}
mvfst_client
mvfst_codec_types
mvfst_constants

original_message:
consume fizz as an external project in cmake

Summary: Favor standalone cmake config than relying on custom script

Reviewed By: siyengar

Differential Revision: D15227020

fbshipit-source-id: 949885b06daeef5e496bf5f1b67a2a3a89750ebf


change_type: MODIFY

diff:
# LICENSE file in the root directory of this source tree.
include(ExternalProject)
+list(APPEND PREFIX_PATH ${CMAKE_PREFIX_PATH} ${CMAKE_INSTALL_PREFIX})
# Construct platform aware cmake args since on OSX
# it requires path to the openssl libs
list(APPEND CMAKE_ARGS
-DCMAKE_BUILD_TYPE:STRING=RelWithDebInfo
- -DCMAKE_PREFIX_PATH:STRING=${CMAKE_PREFIX_PATH}
+ -DCMAKE_PREFIX_PATH:STRING=${PREFIX_PATH}
)
if(UNIX AND APPLE)
list(APPEND CMAKE_ARGS

license: MIT License

repo: facebookincubator/mvfst

original_message:
pass in install prefix path to external-projects

Summary:
This adds path of newly built dependencies so that 'external-projects' such as
fizz can find them

Reviewed By: lnicco

Differential Revision: D15253200

fbshipit-source-id: 993bb030e3a3800697215af089e24482b664d782


change_type: ADD

diff:
+#!/usr/bin/env python
+# Copyright (c) Facebook, Inc. and its affiliates.
+from __future__ import absolute_import
+from __future__ import division
+from __future__ import print_function
+from __future__ import unicode_literals
+
+import specs.folly as folly
+import specs.fizz as fizz
+
+
+def fbcode_builder_spec(builder):
+ return {
+ 'depends_on': [folly, fizz],
+ 'steps': [
+ builder.fb_github_cmake_install(
+ 'mvfst/build',
+ github_org='facebookincubator',
+ ),
+ ],
+ }

diff:
+#!/usr/bin/env python
+
+# 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.
+
+from __future__ import absolute_import
+from __future__ import division
+from __future__ import print_function
+from __future__ import unicode_literals
+
+'fbcode_builder steps to build & test mvfst'
+
+import specs.gmock as gmock
+import specs.folly as folly
+import specs.fizz as fizz
+
+from shell_quoting import ShellQuoted
+
+
+def fbcode_builder_spec(builder):
+ builder.add_option(
+ 'mvfst/_build:cmake_defines',
+ {
+ 'BUILD_SHARED_LIBS': 'OFF',
+ 'BUILD_TESTS': 'ON',
+ }
+ )
+ return {
+ 'depends_on': [gmock, folly, fizz],
+ 'steps': [
+ builder.fb_github_cmake_install('mvfst/_build', '..', 'facebookincubator'),
+ builder.step(
+ 'Run mvfst tests', [
+ builder.run(
+ ShellQuoted('ctest --output-on-failure -j {n}')
+ .format(n=builder.option('make_parallelism'), )
+ )
+ ]
+ ),
+ ]
+ }
+
+
+config = {
+ 'github_project': 'facebookincubator/mvfst',
+ 'fbcode_builder_spec': fbcode_builder_spec,
+}

original_message:
add fbcode_builder_config for mvfst build in oss

Summary: support for fbcode_build and travis build

Reviewed By: siyengar

Differential Revision: D15184766

fbshipit-source-id: 5117164c324d3d3c867592a88670a53b6f862477


diff:
@@ -362,7 +362,7 @@ TEST_F(QuicPacketSchedulerTest, WriteOnlyOutstandingPacketsTest) {
// Create few frames
ConnectionCloseFrame connCloseFrame(
TransportErrorCode::FRAME_ENCODING_ERROR, "The sun is in the sky.");
- MaxStreamsFrame maxStreamIdFrame(0x1024, true);
+ MaxStreamsFrame maxStreamFrame(999, true);
PingFrame pingFrame;
IntervalSet<PacketNum> ackBlocks;
ackBlocks.insert(10, 100);
@@ -371,7 +371,7 @@ TEST_F(QuicPacketSchedulerTest, WriteOnlyOutstandingPacketsTest) {
// Write those framses with a regular builder
writeFrame(connCloseFrame, regularBuilder);
- writeFrame(maxStreamIdFrame, regularBuilder);
+ writeFrame(maxStreamFrame, regularBuilder);
writeFrame(pingFrame, regularBuilder);
writeAckFrame(ackMeta, regularBuilder);

diff:
@@ -376,27 +376,25 @@ MaxStreamDataFrame decodeMaxStreamDataFrame(folly::io::Cursor& cursor) {
}
MaxStreamsFrame decodeBiDiMaxStreamsFrame(folly::io::Cursor& cursor) {
- auto streamId = decodeQuicInteger(cursor);
- if (UNLIKELY(!streamId || !isBidirectionalStream(streamId->first))) {
+ auto streamCount = decodeQuicInteger(cursor);
+ if (UNLIKELY(!streamCount)) {
throw QuicTransportException(
"Invalid Bi-directional streamId",
quic::TransportErrorCode::FRAME_ENCODING_ERROR,
quic::FrameType::MAX_STREAMS_BIDI);
}
- return MaxStreamsFrame(
- folly::to<StreamId>(streamId->first), true /* isBidirectional*/);
+ return MaxStreamsFrame(streamCount->first, true /* isBidirectional*/);
}
MaxStreamsFrame decodeUniMaxStreamsFrame(folly::io::Cursor& cursor) {
- auto streamId = decodeQuicInteger(cursor);
- if (UNLIKELY(!streamId || !isUnidirectionalStream(streamId->first))) {
+ auto streamCount = decodeQuicInteger(cursor);
+ if (UNLIKELY(!streamCount)) {
throw QuicTransportException(
"Invalid Uni-directional streamId",
quic::TransportErrorCode::FRAME_ENCODING_ERROR,
quic::FrameType::MAX_STREAMS_UNI);
}
- return MaxStreamsFrame(
- folly::to<StreamId>(streamId->first), false /* isBidirectional */);
+ return MaxStreamsFrame(streamCount->first, false /* isUnidirectional */);
}
DataBlockedFrame decodeDataBlockedFrame(folly::io::Cursor& cursor) {

diff:
@@ -474,11 +474,11 @@ size_t writeFrame(QuicWriteFrame&& frame, PacketBuilderInterface& builder) {
? FrameType::MAX_STREAMS_BIDI
: FrameType::MAX_STREAMS_UNI;
QuicInteger packetType(static_cast<FrameTypeType>(frameType));
- QuicInteger streamId(maxStreamsFrame.maxStreams);
- auto maxStreamsFrameSize = packetType.getSize() + streamId.getSize();
+ QuicInteger streamCount(maxStreamsFrame.maxStreams);
+ auto maxStreamsFrameSize = packetType.getSize() + streamCount.getSize();
if (packetSpaceCheck(spaceLeft, maxStreamsFrameSize)) {
builder.write(packetType);
- builder.write(streamId);
+ builder.write(streamCount);
builder.appendFrame(std::move(maxStreamsFrame));
return maxStreamsFrameSize;
}

diff:
@@ -63,7 +63,7 @@ TEST_F(QuicPacketRebuilderTest, RebuildPacket) {
TransportErrorCode::FRAME_ENCODING_ERROR,
"The sun is in the sky.",
FrameType::ACK);
- MaxStreamsFrame maxStreamIdFrame(0x1024, true);
+ MaxStreamsFrame maxStreamsFrame(4321, true);
PingFrame pingFrame;
IntervalSet<PacketNum> ackBlocks;
ackBlocks.insert(10, 100);
@@ -83,7 +83,7 @@ TEST_F(QuicPacketRebuilderTest, RebuildPacket) {
// Write them with a regular builder
writeFrame(connCloseFrame, regularBuilder1);
- writeFrame(maxStreamIdFrame, regularBuilder1);
+ writeFrame(maxStreamsFrame, regularBuilder1);
writeFrame(pingFrame, regularBuilder1);
writeAckFrame(ackMeta, regularBuilder1);
writeStreamFrame(streamMeta, regularBuilder1);
@@ -125,7 +125,7 @@ TEST_F(QuicPacketRebuilderTest, RebuildPacket) {
EXPECT_EQ(FrameType::ACK, closeFrame.closingFrameType);
},
[](const MaxStreamsFrame& maxStreamFrame) {
- EXPECT_EQ(0x1024, maxStreamFrame.maxStreams);
+ EXPECT_EQ(4321, maxStreamFrame.maxStreams);
},
[](const PingFrame& ping) { EXPECT_EQ(PingFrame(), ping); },
[](const WriteAckFrame& ack) {

diff:
@@ -945,50 +945,56 @@ TEST_F(QuicWriteCodecTest, NoSpaceForMaxData) {
}
TEST_F(QuicWriteCodecTest, WriteMaxStreamId) {
+ for (uint64_t i = 0; i < 100; i++) {
MockQuicPacketBuilder pktBuilder;
setupCommonExpects(pktBuilder);
- StreamId maxStream = 0x1234;
+ uint64_t maxStream = i;
bool isBidirectional = true;
- MaxStreamsFrame maxStreamIdFrame(maxStream, isBidirectional);
- auto bytesWritten = writeFrame(maxStreamIdFrame, pktBuilder);
+ MaxStreamsFrame maxStreamsFrame(maxStream, isBidirectional);
+ auto bytesWritten = writeFrame(maxStreamsFrame, pktBuilder);
auto builtOut = std::move(pktBuilder).buildPacket();
auto regularPacket = builtOut.first;
- // 1 byte for the type and 2 bytes for the stream id.
- EXPECT_EQ(1 + 2, bytesWritten);
+ auto streamCountSize = i < 64 ? 1 : 2;
+ // 1 byte for the type and up to 2 bytes for the stream count.
+ EXPECT_EQ(1 + streamCountSize, bytesWritten);
auto resultMaxStreamIdFrame =
boost::get<MaxStreamsFrame>(regularPacket.frames[0]);
- EXPECT_EQ(0x1234, resultMaxStreamIdFrame.maxStreams);
+ EXPECT_EQ(i, resultMaxStreamIdFrame.maxStreams);
auto wireBuf = std::move(builtOut.second);
folly::io::Cursor cursor(wireBuf.get());
- auto wireStreamIdFrame = boost::get<MaxStreamsFrame>(parseQuicFrame(cursor));
- EXPECT_EQ(0x1234, wireStreamIdFrame.maxStreams);
+ auto wireStreamsFrame = boost::get<MaxStreamsFrame>(parseQuicFrame(cursor));
+ EXPECT_EQ(i, wireStreamsFrame.maxStreams);
EXPECT_TRUE(cursor.isAtEnd());
}
+}
TEST_F(QuicWriteCodecTest, WriteUniMaxStreamId) {
+ for (uint64_t i = 0; i < 100; i++) {
MockQuicPacketBuilder pktBuilder;
setupCommonExpects(pktBuilder);
- StreamId maxStream = 0x1233;
+ uint64_t maxStream = i;
bool isBidirectional = false;
- MaxStreamsFrame maxStreamIdFrame(maxStream, isBidirectional);
- auto bytesWritten = writeFrame(maxStreamIdFrame, pktBuilder);
+ MaxStreamsFrame maxStreamsFrame(maxStream, isBidirectional);
+ auto bytesWritten = writeFrame(maxStreamsFrame, pktBuilder);
auto builtOut = std::move(pktBuilder).buildPacket();
auto regularPacket = builtOut.first;
- // 1 byte for the type and 2 bytes for the stream id.
- EXPECT_EQ(1 + 2, bytesWritten);
+ auto streamCountSize = i < 64 ? 1 : 2;
+ // 1 byte for the type and up to 2 bytes for the stream count.
+ EXPECT_EQ(1 + streamCountSize, bytesWritten);
auto resultMaxStreamIdFrame =
boost::get<MaxStreamsFrame>(regularPacket.frames[0]);
- EXPECT_EQ(0x1233, resultMaxStreamIdFrame.maxStreams);
+ EXPECT_EQ(i, resultMaxStreamIdFrame.maxStreams);
auto wireBuf = std::move(builtOut.second);
folly::io::Cursor cursor(wireBuf.get());
- auto wireStreamIdFrame = boost::get<MaxStreamsFrame>(parseQuicFrame(cursor));
- EXPECT_EQ(0x1233, wireStreamIdFrame.maxStreams);
+ auto wireStreamsFrame = boost::get<MaxStreamsFrame>(parseQuicFrame(cursor));
+ EXPECT_EQ(i, wireStreamsFrame.maxStreams);
EXPECT_TRUE(cursor.isAtEnd());
}
+}
TEST_F(QuicWriteCodecTest, NoSpaceForMaxStreamId) {
MockQuicPacketBuilder pktBuilder;

original_message:
Interpret varint in MAX_STREAMS_FRAME as count and not as stream-id

Summary: post draft-17 it is interpreted as count not the max stream id

Reviewed By: mjoras

Differential Revision: D15523021

fbshipit-source-id: 779a1c5269eb8dbcdcd169303caa7a65e467339c


diff:
@@ -50,13 +50,6 @@ quic::PacketNum nextAckedPacketLen(
return packetNum - ackBlockLen;
}
-/**
- * Returns whether the given StreamId identifies a unidirectional stream.
- */
-bool isUnidirectionalStream(quic::StreamId stream) {
- return stream & 0b10;
-}
-
// The octet following the version contains the lengths of the two connection ID
// fields that follow it
constexpr size_t kConnIdLengthOctet = 1;

original_message:
Remove unused isUnidirectionalStream() function

Reviewed By: JunqiWang

Differential Revision: D15538072

fbshipit-source-id: 49b26a708431c6409ec495f53ea7304ebdec5fde


diff:
@@ -50,7 +50,7 @@ if [ -z "${BUILD_DIR-}" ] ; then
mkdir -p $BUILD_DIR
fi
-if [[ ! -z $MVFST_FOLLY_USE_JEMALLOC ]]; then
+if [[ -n "${MVFST_FOLLY_USE_JEMALLOC-}" ]]; then
if [[ "$MVFST_FOLLY_USE_JEMALLOC" != "n" ]]; then
unset $MVFST_FOLLY_USE_JEMALLOC
fi
@@ -169,7 +169,7 @@ function setup_folly() {
cd "$FOLLY_BUILD_DIR" || exit
# check for environment variable. If
- if [[ -z $MVFST_FOLLY_USE_JEMALLOC ]]; then
+ if [[ -z "${MVFST_FOLLY_USE_JEMALLOC-}" ]]; then
cmake -DCMAKE_BUILD_TYPE=RelWithDebInfo \
-DCMAKE_PREFIX_PATH="$FOLLY_INSTALL_DIR" \
-DCMAKE_INSTALL_PREFIX="$FOLLY_INSTALL_DIR" \

diff:
@@ -43,6 +43,7 @@ add_library(
target_include_directories(
mvfst_exception PUBLIC
+ $<BUILD_INTERFACE:${LIBFIZZ_INCLUDE_DIR}>
$<BUILD_INTERFACE:${QUIC_FBCODE_ROOT}>
$<INSTALL_INTERFACE:include/>
)
@@ -53,6 +54,11 @@ target_compile_options(
${_QUIC_COMMON_COMPILE_OPTIONS}
)
+add_dependencies(
+ mvfst_exception
+ ${FIZZ_TARGET}
+)
+
target_link_libraries(
mvfst_exception PUBLIC
Folly::folly

original_message:
fix oss build failures

Summary: fix for the unbound variable and include fizz header

Reviewed By: mjoras

Differential Revision: D15565195

fbshipit-source-id: 1b02955a2546d57da71cab40ad0ceccdf4cee577


diff:
@@ -36,7 +36,12 @@ ExternalProject_Get_Property(fizz_project binary_dir)
set(FIZZ_BINARY_DIR ${binary_dir})
# Setup fizz libraries and include dirs
-set(LIBFIZZ_LIBRARY
+set(LIBFIZZ_LIBRARY_LOCATION
"${FIZZ_BINARY_DIR}/${CMAKE_CFG_INTDIR}/lib/${CMAKE_STATIC_LIBRARY_PREFIX}fizz${CMAKE_STATIC_LIBRARY_SUFFIX}"
)
+add_library(mvfst_fizz STATIC IMPORTED)
+set_target_properties(mvfst_fizz PROPERTIES IMPORTED_LOCATION "${LIBFIZZ_LIBRARY_LOCATION}")
+set_target_properties(mvfst_fizz PROPERTIES INTERFACE_LINK_LIBRARIES Folly::folly)
+
+set(LIBFIZZ_LIBRARY mvfst_fizz)
set(LIBFIZZ_INCLUDE_DIR "${source_dir}")

original_message:
fix oss build failure due undefined symbol in folly

Summary: Fix for the issue https://github.com/facebookincubator/mvfst/issues/22

Reviewed By: mingtaoy

Differential Revision: D15709201

fbshipit-source-id: 474a4af4d08f631dbf294210181b13e582c972eb


diff:
@@ -10,6 +10,7 @@ list(APPEND PREFIX_PATH ${CMAKE_PREFIX_PATH} ${CMAKE_INSTALL_PREFIX})
list(APPEND CMAKE_ARGS
-DCMAKE_BUILD_TYPE:STRING=RelWithDebInfo
-DCMAKE_PREFIX_PATH:STRING=${PREFIX_PATH}
+ -DBUILD_TESTS:STRING=OFF
)
if(UNIX AND APPLE)
list(APPEND CMAKE_ARGS

original_message:
speed up mvfst build by not building fizz test target

Summary:
When consuming fizz as an external target, it's wasteful to build test targets
of fizz.
This should fix the travis build timeout of mvfst in oss:
https://travis-ci.com/facebookincubator/mvfst/builds/114688200

Reviewed By: yangchi

Differential Revision: D15712260

fbshipit-source-id: 2c7c1262bb40962c169f5f922e1343e9ab27410e


diff:
@@ -27,6 +27,7 @@ find_package(Boost 1.62
context
)
find_package(folly REQUIRED)
+find_package(Fizz REQUIRED)
find_package(Glog REQUIRED)
find_package(Threads)
@@ -74,7 +75,7 @@ list(APPEND
-Wnon-virtual-dtor
)
-include(BuildFizz)
+SET(LIBFIZZ_LIBRARY fizz::fizz)
if(BUILD_TESTS)
enable_testing()
include(QuicTest)

diff:
@@ -68,9 +68,11 @@ mkdir -p "$MVFST_BUILD_DIR"
if [ -z "${INSTALL_PREFIX-}" ]; then
FOLLY_INSTALL_DIR=$DEPS_DIR
+ FIZZ_INSTALL_DIR=$DEPS_DIR
MVFST_INSTALL_DIR=$BWD
else
FOLLY_INSTALL_DIR=$INSTALL_PREFIX
+ FIZZ_INSTALL_DIR=$INSTALL_PREFIX
MVFST_INSTALL_DIR=$INSTALL_PREFIX
fi
@@ -187,6 +189,28 @@ function setup_folly() {
cd "$BWD" || exit
}
+function setup_fizz() {
+ FIZZ_DIR=$DEPS_DIR/fizz
+ FIZZ_BUILD_DIR=$DEPS_DIR/fizz/build/
+ if [ ! -d "$FIZZ_DIR" ] ; then
+ echo -e "${COLOR_GREEN}[ INFO ] Cloning fizz repo ${COLOR_OFF}"
+ git clone https://github.com/facebookincubator/fizz "$FIZZ_DIR"
+ echo -e "${COLOR_GREEN}[ INFO ] install dependencies ${COLOR_OFF}"
+ fi
+ echo -e "${COLOR_GREEN}Building Fizz ${COLOR_OFF}"
+ mkdir -p "$FIZZ_BUILD_DIR"
+ cd "$FIZZ_BUILD_DIR" || exit
+ cmake -DCMAKE_BUILD_TYPE=RelWithDebInfo \
+ -DBUILD_TESTS=ON \
+ -DCMAKE_PREFIX_PATH="$FIZZ_INSTALL_DIR" \
+ -DCMAKE_INSTALL_PREFIX="$FIZZ_INSTALL_DIR" \
+ "$FIZZ_DIR/fizz"
+ make -j "$nproc"
+ make install
+ echo -e "${COLOR_GREEN}Fizz is installed ${COLOR_OFF}"
+ cd "$BWD" || exit
+}
+
function detect_platform() {
unameOut="$(uname -s)"
case "${unameOut}" in
@@ -199,6 +223,7 @@ function detect_platform() {
detect_platform
setup_folly
+setup_fizz
# build mvfst:
cd "$MVFST_BUILD_DIR" || exit

diff:
@@ -65,7 +65,7 @@ function(quic_add_test)
"${QUIC_FBCODE_ROOT}/quic/common/test/TestMain.cpp"
)
# make sure googletest and fizz are built first
- add_dependencies(${QUIC_TEST_TARGET} googletest ${FIZZ_TARGET})
+ add_dependencies(${QUIC_TEST_TARGET} googletest)
target_compile_options(
${QUIC_TEST_TARGET} PRIVATE
${_QUIC_BASE_COMPILE_OPTIONS}

diff:
@@ -54,11 +54,6 @@ target_compile_options(
${_QUIC_COMMON_COMPILE_OPTIONS}
)
-add_dependencies(
- mvfst_exception
- ${FIZZ_TARGET}
-)
-
target_link_libraries(
mvfst_exception PUBLIC
Folly::folly

diff:
@@ -27,7 +27,6 @@ target_compile_options(
add_dependencies(
mvfst_transport
- ${FIZZ_TARGET}
mvfst_cc_algo
mvfst_codec
mvfst_codec_pktbuilder

diff:
@@ -30,7 +30,6 @@ target_compile_options(
add_dependencies(
mvfst_test_utils
- ${FIZZ_TARGET}
mvfst_client
mvfst_codec_pktbuilder
mvfst_codec_types

diff:
@@ -27,7 +27,6 @@ target_compile_options(
add_dependencies(
mvfst_cc_algo
- ${FIZZ_TARGET}
mvfst_constants
mvfst_exception
mvfst_state_machine

diff:
@@ -25,7 +25,6 @@ target_compile_options(
add_dependencies(
mvfst_handshake
- ${FIZZ_TARGET}
mvfst_constants
mvfst_exception
mvfst_codec_types

diff:
@@ -23,7 +23,6 @@ target_compile_options(
add_dependencies(
mvfst_happyeyeballs
- ${FIZZ_TARGET}
mvfst_state_machine
)

diff:
@@ -31,7 +31,6 @@ target_compile_options(
add_dependencies(
mvfst_server
- ${FIZZ_TARGET}
mvfst_constants
mvfst_codec
mvfst_codec_types

original_message:
Stop consuming fizz via ExternalProject

Summary:
After changing fizz to export test headers and lib, we do not need to pull in
fizz as an ExternalProject anymore. This simplifies and fixes a lot of build
issues

Reviewed By: yangchi

Differential Revision: D15930220

fbshipit-source-id: dcb01c46b8ade545047c20af2fc5245223afff99


diff:
![alt text](logo.png "MVFST")
+[![Build Status](https://api.travis-ci.com/facebookincubator/mvfst.svg?branch=master)](https://travis-ci.com/facebookincubator/mvfst)
+
## Introduction
`mvfst` (Pronounced *move fast*) is a client and server implementation of [IETF QUIC](https://tools.ietf.org/html/draft-ietf-quic-transport-20) protocol in C++ by Facebook. QUIC is a UDP based reliable, multiplexed transport protocol that will become an internet standard. The goal of `mvfst` is to build a performant implementation of the QUIC transport protocol that applications could adapt for use cases on both the internet and the data-center. `mvfst` has been tested at scale on android, iOS apps, as well as servers and has several features to support large scale deployments.

original_message:
Show travis build status on the landing github page

Summary: As in the title

Reviewed By: mjoras

Differential Revision: D15937052

fbshipit-source-id: 819ce5b8fb767fe8578fabfd84e98afffe1efe11


diff:
@@ -75,7 +75,8 @@ list(APPEND
-Wnon-virtual-dtor
)
-SET(LIBFIZZ_LIBRARY fizz::fizz)
+SET(LIBFIZZ_LIBRARY ${FIZZ_LIBRARIES})
+SET(LIBFIZZ_INCLUDE_DIR ${FIZZ_INCLUDE_DIR})
if(BUILD_TESTS)
enable_testing()
include(QuicTest)

change_type: DELETE

diff:
-# 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(ExternalProject)
-list(APPEND PREFIX_PATH ${CMAKE_PREFIX_PATH} ${CMAKE_INSTALL_PREFIX})
-# Construct platform aware cmake args since on OSX
-# it requires path to the openssl libs
-list(APPEND CMAKE_ARGS
- -DCMAKE_BUILD_TYPE:STRING=RelWithDebInfo
- -DCMAKE_PREFIX_PATH:STRING=${PREFIX_PATH}
- -DBUILD_TESTS:STRING=OFF
-)
-if(UNIX AND APPLE)
- list(APPEND CMAKE_ARGS
- -DOPENSSL_ROOT_DIR:STRING=${OPENSSL_ROOT_DIR}
- -DOPENSSL_LIBRARIES:STRING=${OPENSSL_LIBRARIES}
-)
-endif()
-
-# Consume Fizz as an external project
-ExternalProject_Add(
- fizz_project
- GIT_REPOSITORY https://github.com/facebookincubator/fizz
- PREFIX fizz_project
- SOURCE_SUBDIR fizz
- # Disable install step
- INSTALL_COMMAND ""
- CMAKE_CACHE_ARGS ${CMAKE_ARGS}
-)
-set(FIZZ_TARGET fizz_project)
-ExternalProject_Get_Property(fizz_project source_dir)
-set(FIZZ_SOURCE_DIR ${source_dir})
-set(FIZZ_PROJECT ${source_dir})
-ExternalProject_Get_Property(fizz_project binary_dir)
-set(FIZZ_BINARY_DIR ${binary_dir})
-
-# Setup fizz libraries and include dirs
-set(LIBFIZZ_LIBRARY_LOCATION
- "${FIZZ_BINARY_DIR}/${CMAKE_CFG_INTDIR}/lib/${CMAKE_STATIC_LIBRARY_PREFIX}fizz${CMAKE_STATIC_LIBRARY_SUFFIX}"
-)
-add_library(fizz STATIC IMPORTED)
-set_target_properties(fizz PROPERTIES IMPORTED_LOCATION "${LIBFIZZ_LIBRARY_LOCATION}")
-set_target_properties(fizz PROPERTIES INTERFACE_LINK_LIBRARIES Folly::folly)
-
-set(LIBFIZZ_LIBRARY fizz)
-set(LIBFIZZ_INCLUDE_DIR "${source_dir}")

original_message:
remove module to build fizz as an ExternalProject

Summary: This is no longer necessary now

Reviewed By: yangchi

Differential Revision: D15930219

fbshipit-source-id: d277190811c5f5f507783aeddbd8cedb1284e59a


diff:
@@ -1412,6 +1412,11 @@ TEST_F(QuicServerTakeoverTest, TakeoverTest) {
}
struct UDPReader : public folly::AsyncUDPSocket::ReadCallback {
+ UDPReader() {
+ bufPromise_ =
+ std::make_unique<folly::Promise<std::unique_ptr<folly::IOBuf>>>();
+ }
+
~UDPReader() override = default;
void start(EventBase* evb, SocketAddress addr) {
@@ -1439,6 +1444,7 @@ struct UDPReader : public folly::AsyncUDPSocket::ReadCallback {
const folly::SocketAddress&,
size_t len,
bool truncated) noexcept override {
+ std::lock_guard<std::mutex> guard(bufLock_);
if (truncated) {
bufPromise_->setException(std::runtime_error("truncated buf"));
return;
@@ -1450,6 +1456,7 @@ struct UDPReader : public folly::AsyncUDPSocket::ReadCallback {
}
void onReadError(const AsyncSocketException& ex) noexcept override {
+ std::lock_guard<std::mutex> guard(bufLock_);
if (bufPromise_) {
bufPromise_->setException(ex);
}
@@ -1462,13 +1469,17 @@ struct UDPReader : public folly::AsyncUDPSocket::ReadCallback {
}
folly::Future<std::unique_ptr<folly::IOBuf>> readOne() {
+ std::lock_guard<std::mutex> guard(bufLock_);
+ if (!bufPromise_) {
bufPromise_ =
std::make_unique<folly::Promise<std::unique_ptr<folly::IOBuf>>>();
+ }
return bufPromise_->getFuture().ensure([&]() { bufPromise_ = nullptr; });
}
private:
std::unique_ptr<folly::IOBuf> buf_;
+ std::mutex bufLock_;
std::unique_ptr<folly::Promise<std::unique_ptr<folly::IOBuf>>> bufPromise_;
std::unique_ptr<folly::AsyncUDPSocket> client;
EventBase* evb_;

original_message:
fix a race condition in UDPReader in QuicServerTest

Summary:
if OnDataAvailable() gets called before the readOne(), then `bufPromise_` would
be null, and thus the available buffer (buf_) wouldn't get appended to the
bufPromise_.

Reviewed By: sharma95

Differential Revision: D15960463

fbshipit-source-id: b48df104a021a9e89e863d09a0124c6b19228113


diff:
@@ -219,7 +219,10 @@ void TakeoverPacketHandler::processForwardedPacket(
data->trimStart(cursor - data.get());
QUIC_STATS(worker_->getInfoCallback(), onForwardedPacketProcessed);
worker_->handleNetworkData(
- peerAddress, std::move(data), clientPacketReceiveTime);
+ peerAddress,
+ std::move(data),
+ clientPacketReceiveTime,
+ /* isForwardedData */ true);
}
void TakeoverPacketHandler::stop() {

diff:
@@ -125,7 +125,8 @@ void QuicServerWorker::onDataAvailable(
void QuicServerWorker::handleNetworkData(
const folly::SocketAddress& client,
Buf data,
- const TimePoint& packetReceiveTime) noexcept {
+ const TimePoint& packetReceiveTime,
+ bool isForwardedData) noexcept {
try {
if (shutdown_) {
VLOG(4) << "Packet received after shutdown, dropping";
@@ -167,7 +168,8 @@ void QuicServerWorker::handleNetworkData(
return forwardNetworkData(
client,
std::move(routingData),
- NetworkData(std::move(data), packetReceiveTime));
+ NetworkData(std::move(data), packetReceiveTime),
+ isForwardedData);
}
folly::Expected<ParsedLongHeaderInvariant, TransportErrorCode>
@@ -239,7 +241,8 @@ void QuicServerWorker::handleNetworkData(
return forwardNetworkData(
client,
std::move(routingData),
- NetworkData(std::move(data), packetReceiveTime));
+ NetworkData(std::move(data), packetReceiveTime),
+ isForwardedData);
} catch (const std::exception& ex) {
// Drop the packet.
QUIC_STATS(infoCallback_, onPacketDropped, PacketDropReason::PARSE_ERROR);
@@ -275,12 +278,13 @@ void QuicServerWorker::tryHandlingAsHealthCheck(
void QuicServerWorker::forwardNetworkData(
const folly::SocketAddress& client,
RoutingData&& routingData,
- NetworkData&& networkData) {
+ NetworkData&& networkData,
+ bool isForwardedData) {
// if it's not Client initial or ZeroRtt, AND if the connectionId version
// mismatches: foward if pktForwarding is enabled else dropPacket
if (!routingData.isUsingClientConnId &&
!connIdAlgo_->canParse(routingData.destinationConnId)) {
- if (packetForwardingEnabled_) {
+ if (packetForwardingEnabled_ && !isForwardedData) {
VLOG(3) << "Forwarding packet with unknown connId version from client="
<< client << " to another process";
takeoverPktHandler_.forwardPacketToAnotherServer(
@@ -291,7 +295,9 @@ void QuicServerWorker::forwardNetworkData(
VLOG(3) << "Dropping packet due to unknown connectionId version connId="
<< routingData.destinationConnId.hex();
QUIC_STATS(
- infoCallback_, onPacketDropped, PacketDropReason::INVALID_PACKET);
+ infoCallback_,
+ onPacketDropped,
+ PacketDropReason::CONNECTION_NOT_FOUND);
}
return;
}

diff:
@@ -270,7 +270,8 @@ class QuicServerWorker : public folly::AsyncUDPSocket::ReadCallback,
void handleNetworkData(
const folly::SocketAddress& client,
Buf data,
- const TimePoint& receiveTime) noexcept;
+ const TimePoint& receiveTime,
+ bool isForwardedData = false) noexcept;
/**
* Try handling the data as a health check.
@@ -285,7 +286,8 @@ class QuicServerWorker : public folly::AsyncUDPSocket::ReadCallback,
void forwardNetworkData(
const folly::SocketAddress& client,
RoutingData&& routingData,
- NetworkData&& networkData);
+ NetworkData&& networkData,
+ bool isForwardedData = false);
/**
* Return Infocallback ptr for various transport stats (such as packet

original_message:
avoid possibility of loop during packet forwarding

Reviewed By: lnicco

Differential Revision: D17401129

fbshipit-source-id: db255f313311acd462e7bee1a33c45191ef8c3df


diff:
@@ -21,7 +21,8 @@ class QuicReusePortUDPSocketFactory : public QuicUDPSocketFactory {
override {
auto sock = std::make_unique<folly::AsyncUDPSocket>(evb);
sock->setReusePort(true);
+ sock->setReuseAddr(false);
return sock;
}
};
-}
+} // namespace quic

original_message:
disable reuseAddr by default

Summary:
SO_REUSEADDR for UDP has few corner cases (see
https://www.spinics.net/lists/netdev/msg206437.html)
There's no reason to enable this by default

Reviewed By: siyengar, lnicco

Differential Revision: D17401298

fbshipit-source-id: 2dc88052e8df52841162107e4f0ee87cb3536517


diff:
@@ -129,30 +129,7 @@ void happyEyeballsSetUpSocket(
socket.bind(folly::SocketAddress("::", 0));
}
if (transportSettings.turnoffPMTUD) {
- // TODO: Clean this up or move this into AsyncUDPSocket once we have a
- // better idea of how to handle PMTU
-#if defined(IP_MTU_DISCOVER) && defined(IP_PMTUDISC_PROBE)
- if (socket.address().getFamily() == AF_INET) {
- int v4 = IP_PMTUDISC_PROBE;
- folly::netops::setsockopt(
- socket.getNetworkSocket(),
- IPPROTO_IP,
- IP_MTU_DISCOVER,
- &v4,
- sizeof(v4));
- }
-#endif
-#if defined(IPV6_MTU_DISCOVER) && defined(IPV6_PMTUDISC_PROBE)
- if (socket.address().getFamily() == AF_INET6) {
- int v6 = IPV6_PMTUDISC_PROBE;
- folly::netops::setsockopt(
- socket.getNetworkSocket(),
- IPPROTO_IPV6,
- IPV6_MTU_DISCOVER,
- &v6,
- sizeof(v6));
- }
-#endif
+ socket.setDFAndTurnOffPMTU();
} else {
socket.dontFragment(true);
}

diff:
@@ -34,7 +34,7 @@ void QuicServerWorker::bind(const folly::SocketAddress& address) {
DCHECK(!supportedVersions_.empty());
CHECK(socket_);
socket_->bind(address);
- socket_->dontFragment(true);
+ socket_->setDFAndTurnOffPMTU();
}
void QuicServerWorker::setTransportSettingsOverrideFn(

diff:
@@ -24,7 +24,7 @@ class QuicSharedUDPSocketFactory : public QuicUDPSocketFactory {
sock->setFD(
folly::NetworkSocket::fromFd(fd),
folly::AsyncUDPSocket::FDOwnership::SHARED);
- sock->dontFragment(true);
+ sock->setDFAndTurnOffPMTU();
}
return sock;
}

diff:
@@ -67,7 +67,7 @@ class SimpleQuicServerWorkerTest : public Test {
folly::test::MockAsyncUDPSocket* rawSocket_{nullptr};
};
-TEST_F(SimpleQuicServerWorkerTest, DontFragment) {
+TEST_F(SimpleQuicServerWorkerTest, TurnOffPMTU) {
auto sock = std::make_unique<folly::test::MockAsyncUDPSocket>(&eventbase_);
rawSocket_ = sock.get();
DCHECK(sock->getEventBase());
@@ -79,7 +79,7 @@ TEST_F(SimpleQuicServerWorkerTest, DontFragment) {
folly::SocketAddress addr("::1", 0);
// We check versions in bind()
worker_->setSupportedVersions({QuicVersion::MVFST});
- EXPECT_CALL(*rawSocket_, dontFragment(true)).Times(1);
+ EXPECT_CALL(*rawSocket_, setDFAndTurnOffPMTU()).Times(1);
worker_->bind(addr);
}

original_message:
set IP_PMTUDISC_PROBE instead of dontFragment (i.e. PMTUDISC_DO)

Summary:
mvfst will need to handle pmtu in an app specific way. So just set DF bits but
not pmtu.

Reviewed By: siyengar

Differential Revision: D17624134

fbshipit-source-id: 14f445bbb6a971efb8a3d550c84c3d4af53f8517


diff:
@@ -278,6 +278,32 @@ void QuicServer::allowBeingTakenOver(const folly::SocketAddress& addr) {
takeoverHandlerInitialized_ = true;
}
+folly::SocketAddress QuicServer::overrideTakeoverHandlerAddress(
+ const folly::SocketAddress& addr) {
+ // synchronously bind workers to takeover handler port.
+ // This method should not be called from a worker
+ CHECK(!workers_.empty());
+ CHECK(!shutdown_);
+ CHECK(takeoverHandlerInitialized_) << "TakeoverHanders are not initialized. ";
+
+ // this function shouldn't be called from worker's thread
+ for (auto& worker : workers_) {
+ DCHECK(
+ // if the eventbase is not running, it returns true for isInEvbThread()
+ !worker->getEventBase()->isRunning() ||
+ !worker->getEventBase()->isInEventBaseThread());
+ }
+ folly::SocketAddress boundAddress;
+ for (auto& worker : workers_) {
+ worker->getEventBase()->runInEventBaseThreadAndWait([&] {
+ std::lock_guard<std::mutex> guard(startMutex_);
+ CHECK(initialized_);
+ boundAddress = worker->overrideTakeoverHandlerAddress(addr);
+ });
+ }
+ return boundAddress;
+}
+
void QuicServer::pauseRead() {
runOnAllWorkers([&](auto worker) mutable { worker->pauseRead(); });
}

diff:
@@ -227,6 +227,9 @@ class QuicServer : public QuicServerWorker::WorkerCallback,
*/
void allowBeingTakenOver(const folly::SocketAddress& addr);
+ folly::SocketAddress overrideTakeoverHandlerAddress(
+ const folly::SocketAddress& addr);
+
/*
* Setup and initialize the listening socket of the old server from the given
* address to forward misrouted packets belonging to that server during

diff:
@@ -24,12 +24,10 @@ TakeoverHandlerCallback::TakeoverHandlerCallback(
QuicServerWorker* worker,
TakeoverPacketHandler& takeoverPktHandler,
const TransportSettings& transportSettings,
- const folly::SocketAddress& address,
std::unique_ptr<folly::AsyncUDPSocket> socket)
: worker_(worker),
takeoverPktHandler_(takeoverPktHandler),
transportSettings_(transportSettings),
- address_(address),
socket_(std::move(socket)) {}
TakeoverHandlerCallback::~TakeoverHandlerCallback() {
@@ -39,9 +37,9 @@ TakeoverHandlerCallback::~TakeoverHandlerCallback() {
}
}
-void TakeoverHandlerCallback::bind() {
+void TakeoverHandlerCallback::bind(const folly::SocketAddress& addr) {
CHECK(socket_);
- socket_->bind(address_);
+ socket_->bind(addr);
socket_->resumeRead(this);
}
@@ -51,6 +49,11 @@ void TakeoverHandlerCallback::pause() {
}
}
+const folly::SocketAddress& TakeoverHandlerCallback::getAddress() const {
+ CHECK(socket_);
+ return socket_->address();
+}
+
int TakeoverHandlerCallback::getSocketFD() {
CHECK(socket_);
return socket_->getNetworkSocket().toFd();

diff:
@@ -114,7 +114,6 @@ class TakeoverHandlerCallback : public folly::AsyncUDPSocket::ReadCallback,
QuicServerWorker* worker,
TakeoverPacketHandler& takeoverPktHandler,
const TransportSettings& transportSettings,
- const folly::SocketAddress& address,
std::unique_ptr<folly::AsyncUDPSocket> socket);
// prevent copying
@@ -123,12 +122,14 @@ class TakeoverHandlerCallback : public folly::AsyncUDPSocket::ReadCallback,
virtual ~TakeoverHandlerCallback() override;
- void bind();
+ void bind(const folly::SocketAddress& addr);
void pause();
int getSocketFD();
+ const folly::SocketAddress& getAddress() const;
+
// AsyncUDPSocket ReadCallback methods
void getReadBuffer(void** buf, size_t* len) noexcept override;

diff:
@@ -513,12 +513,15 @@ void QuicServerWorker::allowBeingTakenOver(
// We instantiate and bind the TakeoverHandlerCallback to the given address.
// It is reset at shutdownAllConnections (i.e. only when the process dies).
takeoverCB_ = std::make_unique<TakeoverHandlerCallback>(
- this,
- takeoverPktHandler_,
- transportSettings_,
- address,
- std::move(socket));
- takeoverCB_->bind();
+ this, takeoverPktHandler_, transportSettings_, std::move(socket));
+ takeoverCB_->bind(address);
+}
+
+const folly::SocketAddress& QuicServerWorker::overrideTakeoverHandlerAddress(
+ const folly::SocketAddress& address) {
+ CHECK(takeoverCB_);
+ takeoverCB_->bind(address);
+ return takeoverCB_->getAddress();
}
void QuicServerWorker::startPacketForwarding(

diff:
@@ -95,6 +95,13 @@ class QuicServerWorker : public folly::AsyncUDPSocket::ReadCallback,
std::unique_ptr<folly::AsyncUDPSocket> socket,
const folly::SocketAddress& address);
+ /**
+ * Override listening address for takeover packets
+ * Returns const ref to SocketAddress representing the address it is bound to.
+ */
+ const folly::SocketAddress& overrideTakeoverHandlerAddress(
+ const folly::SocketAddress& address);
+
/**
* Setup address that the taken over quic server is listening to forward
* misrouted packets belonging to the old server.

original_message:
retry few times if there's a conflict in takeover handler address

Reviewed By: sharma95

Differential Revision: D17403844

fbshipit-source-id: 3084335be30e2376a6888ba24e21aa63bb040e3e


diff:
@@ -311,7 +311,8 @@ void QuicServer::pauseRead() {
void QuicServer::routeDataToWorker(
const folly::SocketAddress& client,
RoutingData&& routingData,
- NetworkData&& networkData) {
+ NetworkData&& networkData,
+ bool isForwardedData) {
// figure out worker idx
if (!initialized_) {
// drop the packet if we are not initialized. This is a janky memory
@@ -344,7 +345,10 @@ void QuicServer::routeDataToWorker(
if (routingData.isUsingClientConnId && workerPtr_) {
CHECK(workerPtr_->getEventBase()->isInEventBaseThread());
workerPtr_->dispatchPacketData(
- client, std::move(routingData), std::move(networkData));
+ client,
+ std::move(routingData),
+ std::move(networkData),
+ isForwardedData);
return;
}
@@ -358,11 +362,13 @@ void QuicServer::routeDataToWorker(
cl = client,
routingData = std::move(routingData),
w = worker.get(),
- buf = std::move(networkData)]() mutable {
+ buf = std::move(networkData),
+ isForwarded = isForwardedData]() mutable {
if (server->shutdown_) {
return;
}
- w->dispatchPacketData(cl, std::move(routingData), std::move(buf));
+ w->dispatchPacketData(
+ cl, std::move(routingData), std::move(buf), isForwarded);
});
}

diff:
@@ -185,7 +185,7 @@ class QuicServer : public QuicServerWorker::WorkerCallback,
*/
void waitUntilInitialized();
- void handleWorkerError(LocalErrorCode error);
+ void handleWorkerError(LocalErrorCode error) override;
/**
* Routes the given data for the given client to the correct worker that may
@@ -194,7 +194,8 @@ class QuicServer : public QuicServerWorker::WorkerCallback,
void routeDataToWorker(
const folly::SocketAddress& client,
RoutingData&& routingData,
- NetworkData&& networkData);
+ NetworkData&& networkData,
+ bool isForwardedData = false) override;
/**
* Set an EventBaseObserver for server and all its workers. This only works

diff:
@@ -313,7 +313,7 @@ void QuicServerWorker::forwardNetworkData(
return;
}
callback_->routeDataToWorker(
- client, std::move(routingData), std::move(networkData));
+ client, std::move(routingData), std::move(networkData), isForwardedData);
}
void QuicServerWorker::setPacingTimer(
@@ -324,7 +324,8 @@ void QuicServerWorker::setPacingTimer(
void QuicServerWorker::dispatchPacketData(
const folly::SocketAddress& client,
RoutingData&& routingData,
- NetworkData&& networkData) noexcept {
+ NetworkData&& networkData,
+ bool isForwardedData) noexcept {
DCHECK(socket_);
QuicServerTransport::Ptr transport;
bool dropPacket = false;
@@ -447,7 +448,7 @@ void QuicServerWorker::dispatchPacketData(
routingData.destinationConnId);
}
- if (!packetForwardingEnabled_) {
+ if (!packetForwardingEnabled_ || isForwardedData) {
QUIC_STATS(
infoCallback_, onPacketDropped, PacketDropReason::CONNECTION_NOT_FOUND);
return sendResetPacket(

diff:
@@ -38,7 +38,8 @@ class QuicServerWorker : public folly::AsyncUDPSocket::ReadCallback,
virtual void routeDataToWorker(
const folly::SocketAddress& client,
RoutingData&& routingData,
- NetworkData&& networkData) = 0;
+ NetworkData&& networkData,
+ bool isForwardedData) = 0;
};
explicit QuicServerWorker(std::shared_ptr<WorkerCallback> callback);
@@ -246,7 +247,8 @@ class QuicServerWorker : public folly::AsyncUDPSocket::ReadCallback,
void dispatchPacketData(
const folly::SocketAddress& client,
RoutingData&& routingData,
- NetworkData&& networkData) noexcept;
+ NetworkData&& networkData,
+ bool isForwardedData = false) noexcept;
using ConnIdToTransportMap = std::
unordered_map<ConnectionId, QuicServerTransport::Ptr, ConnectionIdHash>;

diff:
@@ -47,30 +47,33 @@ class MockWorkerCallback : public QuicServerWorker::WorkerCallback {
~MockWorkerCallback() = default;
MOCK_METHOD1(handleWorkerError, void(LocalErrorCode));
- MOCK_METHOD3(
+ MOCK_METHOD4(
routeDataToWorkerLong,
void(
const folly::SocketAddress&,
std::unique_ptr<RoutingData>&,
- std::unique_ptr<NetworkData>&));
+ std::unique_ptr<NetworkData>&,
+ bool isForwardedData));
- MOCK_METHOD3(
+ MOCK_METHOD4(
routeDataToWorkerShort,
void(
const folly::SocketAddress&,
std::unique_ptr<RoutingData>&,
- std::unique_ptr<NetworkData>&));
+ std::unique_ptr<NetworkData>&,
+ bool isForwardedData));
void routeDataToWorker(
const folly::SocketAddress& client,
RoutingData&& routingDataIn,
- NetworkData&& networkDataIn) {
+ NetworkData&& networkDataIn,
+ bool isForwardedData = false) {
auto routingData = std::make_unique<RoutingData>(std::move(routingDataIn));
auto networkData = std::make_unique<NetworkData>(std::move(networkDataIn));
if (routingData->headerForm == HeaderForm::Long) {
- routeDataToWorkerLong(client, routingData, networkData);
+ routeDataToWorkerLong(client, routingData, networkData, isForwardedData);
} else {
- routeDataToWorkerShort(client, routingData, networkData);
+ routeDataToWorkerShort(client, routingData, networkData, isForwardedData);
}
}
};

diff:
@@ -114,12 +114,16 @@ class QuicServerWorkerTest : public Test {
auto cb = [&](const folly::SocketAddress& addr,
std::unique_ptr<RoutingData>& routingData,
- std::unique_ptr<NetworkData>& networkData) {
+ std::unique_ptr<NetworkData>& networkData,
+ bool isForwardedData) {
worker_->dispatchPacketData(
- addr, std::move(*routingData.get()), std::move(*networkData.get()));
+ addr,
+ std::move(*routingData.get()),
+ std::move(*networkData.get()),
+ isForwardedData);
};
- EXPECT_CALL(*workerCb_, routeDataToWorkerLong(_, _, _))
+ EXPECT_CALL(*workerCb_, routeDataToWorkerLong(_, _, _, _))
.WillRepeatedly(Invoke(cb));
socketFactory_ = std::make_unique<MockQuicUDPSocketFactory>();
@@ -832,11 +836,13 @@ void QuicServerWorkerTakeoverTest::testNoPacketForwarding(
ConnectionId /* connId */) {
auto cb = [&](const folly::SocketAddress& addr,
std::unique_ptr<RoutingData>& /* routingData */,
- std::unique_ptr<NetworkData>& /* networkData */) {
+ std::unique_ptr<NetworkData>& /* networkData */,
+ bool isForwardedData) {
EXPECT_EQ(addr.getIPAddress(), clientAddr.getIPAddress());
EXPECT_EQ(addr.getPort(), clientAddr.getPort());
+ EXPECT_FALSE(isForwardedData);
};
- EXPECT_CALL(*takeoverWorkerCb_, routeDataToWorkerLong(_, _, _))
+ EXPECT_CALL(*takeoverWorkerCb_, routeDataToWorkerLong(_, _, _, _))
.WillOnce(Invoke(cb));
EXPECT_CALL(*transportInfoCb_, onPacketReceived());
EXPECT_CALL(*transportInfoCb_, onRead(len));
@@ -937,11 +943,15 @@ void QuicServerWorkerTakeoverTest::testPacketForwarding(
auto cb = [&](const folly::SocketAddress& client,
std::unique_ptr<RoutingData>& routingData,
- std::unique_ptr<NetworkData>& networkData) {
+ std::unique_ptr<NetworkData>& networkData,
+ bool isForwardedData) {
takeoverWorker_->dispatchPacketData(
- client, std::move(*routingData.get()), std::move(*networkData.get()));
+ client,
+ std::move(*routingData.get()),
+ std::move(*networkData.get()),
+ isForwardedData);
};
- EXPECT_CALL(*takeoverWorkerCb_, routeDataToWorkerLong(_, _, _))
+ EXPECT_CALL(*takeoverWorkerCb_, routeDataToWorkerLong(_, _, _, _))
.WillOnce(Invoke(cb));
EXPECT_CALL(*transportInfoCb_, onPacketReceived());
EXPECT_CALL(*transportInfoCb_, onRead(len));
@@ -995,15 +1005,17 @@ TEST_F(QuicServerWorkerTakeoverTest, QuicServerTakeoverProcessForwardedPkt) {
// test processing of the forwarded packet
auto cb = [&](const folly::SocketAddress& addr,
std::unique_ptr<RoutingData>& /* routingData */,
- std::unique_ptr<NetworkData>& networkData) {
+ std::unique_ptr<NetworkData>& networkData,
+ bool isForwardedData) {
// verify that it is the original client address
EXPECT_EQ(addr.getIPAddress(), clientAddr.getIPAddress());
EXPECT_EQ(addr.getPort(), clientAddr.getPort());
// the original data should be extracted after processing takeover
// protocol related information
EXPECT_TRUE(eq(*data, *(networkData->data)));
+ EXPECT_TRUE(isForwardedData);
};
- EXPECT_CALL(*takeoverWorkerCb_, routeDataToWorkerLong(_, _, _))
+ EXPECT_CALL(*takeoverWorkerCb_, routeDataToWorkerLong(_, _, _, _))
.WillOnce(Invoke(cb));
takeoverCb->onDataAvailable(client, bufLen, false);
@@ -1011,11 +1023,15 @@ TEST_F(QuicServerWorkerTakeoverTest, QuicServerTakeoverProcessForwardedPkt) {
}));
auto workerCb = [&](const folly::SocketAddress& client,
std::unique_ptr<RoutingData>& routingData,
- std::unique_ptr<NetworkData>& networkData) {
+ std::unique_ptr<NetworkData>& networkData,
+ bool isForwardedData) {
takeoverWorker_->dispatchPacketData(
- client, std::move(*routingData.get()), std::move(*networkData.get()));
+ client,
+ std::move(*routingData.get()),
+ std::move(*networkData.get()),
+ isForwardedData);
};
- EXPECT_CALL(*takeoverWorkerCb_, routeDataToWorkerLong(_, _, _))
+ EXPECT_CALL(*takeoverWorkerCb_, routeDataToWorkerLong(_, _, _, _))
.WillOnce(Invoke(workerCb));
EXPECT_CALL(*transportInfoCb_, onPacketReceived());
EXPECT_CALL(*transportInfoCb_, onRead(len));

original_message:
add more checks for internally forwarded data

Summary:
This adds checks for forwarded data on all the possible branches that lead to
forwarding of packets to another process

Reviewed By: mjoras

Differential Revision: D18416971

fbshipit-source-id: 22dc3fd63de615904a411f90164a138bf0ef56e0


diff:
@@ -298,7 +298,10 @@ folly::SocketAddress QuicServer::overrideTakeoverHandlerAddress(
worker->getEventBase()->runInEventBaseThreadAndWait([&] {
std::lock_guard<std::mutex> guard(startMutex_);
CHECK(initialized_);
- boundAddress = worker->overrideTakeoverHandlerAddress(addr);
+ auto workerEvb = worker->getEventBase();
+ auto localListenSocket = listenerSocketFactory_->make(workerEvb, -1);
+ boundAddress = worker->overrideTakeoverHandlerAddress(
+ std::move(localListenSocket), addr);
});
}
return boundAddress;

diff:
@@ -39,6 +39,20 @@ TakeoverHandlerCallback::~TakeoverHandlerCallback() {
void TakeoverHandlerCallback::bind(const folly::SocketAddress& addr) {
CHECK(socket_);
+ // first reset existing socket if any
+ socket_->bind(addr);
+ socket_->resumeRead(this);
+}
+
+void TakeoverHandlerCallback::rebind(
+ std::unique_ptr<folly::AsyncUDPSocket> socket,
+ const folly::SocketAddress& addr) {
+ if (socket_) {
+ // first reset existing socket if any
+ socket_->pauseRead();
+ socket_.reset();
+ }
+ socket_ = std::move(socket);
socket_->bind(addr);
socket_->resumeRead(this);
}

diff:
@@ -123,6 +123,14 @@ class TakeoverHandlerCallback : public folly::AsyncUDPSocket::ReadCallback,
void bind(const folly::SocketAddress& addr);
+ /**
+ * Rebinds the given socket to the given address
+ * Frees existing socket if any
+ */
+ void rebind(
+ std::unique_ptr<folly::AsyncUDPSocket> socket,
+ const folly::SocketAddress& addr);
+
void pause();
int getSocketFD();

diff:
@@ -520,9 +520,10 @@ void QuicServerWorker::allowBeingTakenOver(
}
const folly::SocketAddress& QuicServerWorker::overrideTakeoverHandlerAddress(
+ std::unique_ptr<folly::AsyncUDPSocket> socket,
const folly::SocketAddress& address) {
CHECK(takeoverCB_);
- takeoverCB_->bind(address);
+ takeoverCB_->rebind(std::move(socket), address);
return takeoverCB_->getAddress();
}

diff:
@@ -102,6 +102,7 @@ class QuicServerWorker : public folly::AsyncUDPSocket::ReadCallback,
* Returns const ref to SocketAddress representing the address it is bound to.
*/
const folly::SocketAddress& overrideTakeoverHandlerAddress(
+ std::unique_ptr<folly::AsyncUDPSocket> socket,
const folly::SocketAddress& address);
/**

diff:
@@ -788,6 +788,21 @@ class QuicServerWorkerTakeoverTest : public Test {
uint16_t clientHostId_{25};
};
+TEST_F(QuicServerWorkerTakeoverTest, QuicServerTakeoverReInitHandler) {
+ auto takeoverSock = std::make_unique<folly::test::MockAsyncUDPSocket>(&evb_);
+ folly::SocketAddress takeoverAddr;
+ EXPECT_CALL(*takeoverSocket_, pauseRead());
+
+ EXPECT_CALL(*takeoverSock, bind(_));
+ EXPECT_CALL(*takeoverSock, resumeRead(_));
+ EXPECT_CALL(*takeoverSock, address()).WillOnce(Invoke([&]() {
+ return takeoverAddr;
+ }));
+ takeoverWorker_->overrideTakeoverHandlerAddress(
+ std::move(takeoverSock), takeoverAddr);
+ takeoverSocket_ = takeoverSock.get();
+}
+
TEST_F(QuicServerWorkerTakeoverTest, QuicServerTakeoverNoForwarding) {
ConnectionId connId = createConnIdForServer(ProcessId::ONE),
clientConnId = getTestConnectionId(clientHostId_);
@@ -1369,6 +1384,24 @@ TEST_F(QuicServerTest, RouteDataFromDifferentThread) {
t.join();
}
+TEST_F(QuicServerTest, OverrideTakeoverAddressTest) {
+ folly::ScopedEventBaseThread evbThread;
+ std::vector<folly::EventBase*> evbs;
+ evbs.emplace_back(evbThread.getEventBase());
+ auto serverAddr = initializeServer(evbs);
+ folly::SocketAddress takeoverAddr("::1", 0);
+ server_->allowBeingTakenOver(takeoverAddr);
+ uint8_t bindAttempt = 0;
+ folly::SocketAddress boundAddr;
+ while (bindAttempt < 5) {
+ boundAddr = server_->overrideTakeoverHandlerAddress(takeoverAddr);
+ bindAttempt++;
+ }
+ EXPECT_TRUE(boundAddr.isInitialized());
+ std::thread t([&] { server_->shutdown(); });
+ t.join();
+}
+
class QuicServerTakeoverTest : public Test {
public:
void SetUp() override {

original_message:
reset existing (takeover) socket when rebinding to different address

Reviewed By: afrind

Differential Revision: D18925452

fbshipit-source-id: dc431cbf899eb8e21a18d421a640e4f0649743cb


diff:
@@ -28,7 +28,7 @@ constexpr uint8_t kWorkerIdSecondByteMask = 0x03;
constexpr uint8_t kShortVersionBitsMask = 0xc0;
/**
- * Sets the short version id bits (0 - 3) into the given ConnectionId
+ * Sets the short version id bits (0 - 1) into the given ConnectionId
*/
void setVersionBitsInConnId(quic::ConnectionId& connId, uint8_t version) {
if (connId.size() < quic::kMinConnectionIdSize) {
@@ -80,7 +80,7 @@ void setHostIdBitsInConnId(quic::ConnectionId& connId, uint16_t hostId) {
}
/**
- * Extract the host id bits [4 - 15] bits from the given ConnectionId
+ * Extract the host id bits [2 - 17] bits from the given ConnectionId
*/
uint16_t getHostIdBitsInConnId(const quic::ConnectionId& connId) {
if (connId.size() < quic::kMinConnectionIdSize) {
@@ -101,7 +101,7 @@ uint16_t getHostIdBitsInConnId(const quic::ConnectionId& connId) {
}
/**
- * Sets the given 8-bit workerId into the given connectionId's 16-23 bits
+ * Sets the given 8-bit workerId into the given connectionId's 18-25 bits
*/
void setWorkerIdBitsInConnId(quic::ConnectionId& connId, uint8_t workerId) {
if (connId.size() < quic::kMinConnectionIdSize) {
@@ -136,7 +136,7 @@ uint8_t getWorkerIdFromConnId(const quic::ConnectionId& connId) {
}
/**
- * Sets the server id bit (at 24th bit) into the given ConnectionId
+ * Sets the server id bit (at 26th bit) into the given ConnectionId
*/
void setProcessIdBitsInConnId(quic::ConnectionId& connId, uint8_t processId) {
if (connId.size() < quic::kMinConnectionIdSize) {

original_message:
update comments in connection-id encoder

Summary: The comments got out-of-sync from the actual code. This fixes it

Reviewed By: yangchi

Differential Revision: D19270100

fbshipit-source-id: b51afcd4c220fc25a4b9a001ce7a18dd74c8f7ef


diff:
@@ -1769,8 +1769,8 @@ TEST_F(QuicServerTest, TestRejectNewConnections) {
// new connections
folly::SocketAddress addr("::1", 0);
server_->start(addr, 2);
- server_->waitUntilInitialized();
server_->rejectNewConnections(true);
+ server_->waitUntilInitialized();
auto testingObserver = std::make_shared<TestingEventBaseObserver>();
server_->setEventBaseObserver(testingObserver);
auto serverAddr = server_->getAddress();
@@ -1808,26 +1808,6 @@ TEST_F(QuicServerTest, TestRejectNewConnections) {
EXPECT_TRUE(testingObserver->observerCalled());
EXPECT_EQ(versionPacket->versions.size(), 1);
EXPECT_EQ(versionPacket->versions.at(0), QuicVersion::MVFST_INVALID);
-
- // Then reset the reject flag and check that we get a valid version instead
- server_->rejectNewConnections(false);
-
- buf = folly::IOBuf::copyBuffer("hello");
- packet = createInitialStream(clientConnId, serverConnId, id, *buf, MVFST1);
- data = std::move(packet);
- reader->getSocket().write(serverAddr, data->clone());
-
- serverData = reader->readOne().get();
-
- packetQueue = bufToQueue(std::move(serverData));
- versionPacket = codec->tryParsingVersionNegotiation(packetQueue);
- ASSERT_TRUE(versionPacket.hasValue());
-
- EXPECT_EQ(versionPacket->destinationConnectionId, clientConnId);
- EXPECT_EQ(versionPacket->sourceConnectionId, serverConnId);
- EXPECT_TRUE(testingObserver->observerCalled());
- EXPECT_GE(versionPacket->versions.size(), 1);
- EXPECT_NE(versionPacket->versions.at(0), QuicVersion::MVFST_INVALID);
}
TEST_F(QuicServerTest, NetworkTestHealthCheck) {

original_message:
Remove a flaky test block in QuicServer Test

Summary:
rejectNewConnections() runs asynchronously. As such, it is not guaranteed to complete before the next line where client writes packets. As a result,
server sometimes rejects the connection

Reviewed By: lnicco

Differential Revision: D19356730

fbshipit-source-id: 6e2e6571937737f30da1a1ffdcefdb759a0e9a73


diff:
@@ -39,8 +39,8 @@ QuicClientTransport::QuicClientTransport(
DCHECK(handshakeFactory);
// TODO(T53612743) Only enforce that the initial destination connection id
// is at least kMinInitialDestinationConnIdLength.
- // All subsequent connection ids should be in between
- // [kMinConnectionIdSize, kMaxConnectionIdSize]
+ // All subsequent destination connection ids should be in between
+ // [kMinSelfConnectionIdSize, kMaxConnectionIdSize]
DCHECK(
connectionIdSize == 0 ||
(connectionIdSize >= kMinInitialDestinationConnIdLength &&

diff:
@@ -500,7 +500,7 @@ NewConnectionIdFrame decodeNewConnectionIdFrame(folly::io::Cursor& cursor) {
quic::TransportErrorCode::FRAME_ENCODING_ERROR,
quic::FrameType::NEW_CONNECTION_ID);
}
- if (connIdLen < kMinConnectionIdSize || connIdLen > kMaxConnectionIdSize) {
+ if (connIdLen > kMaxConnectionIdSize) {
throw QuicTransportException(
"ConnectionId invalid length",
quic::TransportErrorCode::FRAME_ENCODING_ERROR,

diff:
@@ -31,7 +31,7 @@ constexpr uint8_t kShortVersionBitsMask = 0xc0;
* Sets the short version id bits (0 - 1) into the given ConnectionId
*/
void setVersionBitsInConnId(quic::ConnectionId& connId, uint8_t version) {
- if (connId.size() < quic::kMinConnectionIdSize) {
+ if (connId.size() < quic::kMinSelfConnectionIdSize) {
throw quic::QuicInternalException(
"ConnectionId is too small for version",
quic::LocalErrorCode::INTERNAL_ERROR);
@@ -45,7 +45,7 @@ void setVersionBitsInConnId(quic::ConnectionId& connId, uint8_t version) {
* Extract the version id bits (0 - 1) from the given ConnectionId
*/
uint8_t getVersionBitsFromConnId(const quic::ConnectionId& connId) {
- if (connId.size() < quic::kMinConnectionIdSize) {
+ if (connId.size() < quic::kMinSelfConnectionIdSize) {
throw quic::QuicInternalException(
"ConnectionId is too small for version",
quic::LocalErrorCode::INTERNAL_ERROR);
@@ -59,7 +59,7 @@ uint8_t getVersionBitsFromConnId(const quic::ConnectionId& connId) {
* Sets the host id bits [2 - 17] bits into the given ConnectionId
*/
void setHostIdBitsInConnId(quic::ConnectionId& connId, uint16_t hostId) {
- if (connId.size() < quic::kMinConnectionIdSize) {
+ if (connId.size() < quic::kMinSelfConnectionIdSize) {
throw quic::QuicInternalException(
"ConnectionId is too small for hostid",
quic::LocalErrorCode::INTERNAL_ERROR);
@@ -83,7 +83,7 @@ void setHostIdBitsInConnId(quic::ConnectionId& connId, uint16_t hostId) {
* Extract the host id bits [2 - 17] bits from the given ConnectionId
*/
uint16_t getHostIdBitsInConnId(const quic::ConnectionId& connId) {
- if (connId.size() < quic::kMinConnectionIdSize) {
+ if (connId.size() < quic::kMinSelfConnectionIdSize) {
throw quic::QuicInternalException(
"ConnectionId is too small for hostid",
quic::LocalErrorCode::INTERNAL_ERROR);
@@ -104,7 +104,7 @@ uint16_t getHostIdBitsInConnId(const quic::ConnectionId& connId) {
* Sets the given 8-bit workerId into the given connectionId's 18-25 bits
*/
void setWorkerIdBitsInConnId(quic::ConnectionId& connId, uint8_t workerId) {
- if (connId.size() < quic::kMinConnectionIdSize) {
+ if (connId.size() < quic::kMinSelfConnectionIdSize) {
throw quic::QuicInternalException(
"ConnectionId is too small for workerid",
quic::LocalErrorCode::INTERNAL_ERROR);
@@ -123,7 +123,7 @@ void setWorkerIdBitsInConnId(quic::ConnectionId& connId, uint8_t workerId) {
* Extracts the 'workerId' bits from the given ConnectionId
*/
uint8_t getWorkerIdFromConnId(const quic::ConnectionId& connId) {
- if (connId.size() < quic::kMinConnectionIdSize) {
+ if (connId.size() < quic::kMinSelfConnectionIdSize) {
throw quic::QuicInternalException(
"ConnectionId is too small for workerid",
quic::LocalErrorCode::INTERNAL_ERROR);
@@ -139,7 +139,7 @@ uint8_t getWorkerIdFromConnId(const quic::ConnectionId& connId) {
* Sets the server id bit (at 26th bit) into the given ConnectionId
*/
void setProcessIdBitsInConnId(quic::ConnectionId& connId, uint8_t processId) {
- if (connId.size() < quic::kMinConnectionIdSize) {
+ if (connId.size() < quic::kMinSelfConnectionIdSize) {
throw quic::QuicInternalException(
"ConnectionId is too small for processid",
quic::LocalErrorCode::INTERNAL_ERROR);
@@ -153,7 +153,7 @@ void setProcessIdBitsInConnId(quic::ConnectionId& connId, uint8_t processId) {
* Extract the server id bit (at 26th bit) from the given ConnectionId
*/
uint8_t getProcessIdBitsFromConnId(const quic::ConnectionId& connId) {
- if (connId.size() < quic::kMinConnectionIdSize) {
+ if (connId.size() < quic::kMinSelfConnectionIdSize) {
throw quic::QuicInternalException(
"ConnectionId is too small for processid",
quic::LocalErrorCode::INTERNAL_ERROR);
@@ -167,7 +167,7 @@ uint8_t getProcessIdBitsFromConnId(const quic::ConnectionId& connId) {
namespace quic {
bool DefaultConnectionIdAlgo::canParse(const ConnectionId& id) const {
- if (id.size() < kMinConnectionIdSize) {
+ if (id.size() < kMinSelfConnectionIdSize) {
return false;
}
return getVersionBitsFromConnId(id) == kShortVersionId;

diff:
@@ -32,9 +32,7 @@ ConnectionId::ConnectionId(const std::vector<uint8_t>& connidIn) {
static_assert(
std::numeric_limits<uint8_t>::max() > kMaxConnectionIdSize,
"Max connection size is too big");
- if (connidIn.size() != 0 &&
- (connidIn.size() < kMinConnectionIdSize ||
- connidIn.size() > kMaxConnectionIdSize)) {
+ if (connidIn.size() > kMaxConnectionIdSize) {
// We can't throw a transport error here because of the dependency. This is
// sad because this will cause an internal error downstream.
throw std::runtime_error("ConnectionId invalid size");
@@ -51,7 +49,7 @@ ConnectionId::ConnectionId(folly::io::Cursor& cursor, size_t len) {
connidLen = 0;
return;
}
- if (len < kMinConnectionIdSize || len > kMaxConnectionIdSize) {
+ if (len > kMaxConnectionIdSize) {
// We can't throw a transport error here because of the dependency. This is
// sad because this will cause an internal error downstream.
throw std::runtime_error("ConnectionId invalid size");

diff:
@@ -21,7 +21,9 @@ namespace quic {
constexpr uint8_t kStatelessResetTokenLength = 16;
using StatelessResetToken = std::array<uint8_t, kStatelessResetTokenLength>;
-constexpr size_t kMinConnectionIdSize = 4;
+// min connId size for one chosen by 'mvfst' as a peer
+constexpr size_t kMinSelfConnectionIdSize = 4;
+// max size of a connId as specified in the draft
constexpr size_t kMaxConnectionIdSize = 20;
// Minimum required length (in bytes) for the destination connection-id

diff:
@@ -59,10 +59,10 @@ TEST(ConnectionIdTest, ConnIdSize) {
}
EXPECT_THROW(ConnectionId{testconnid}, std::runtime_error);
testconnid.clear();
- for (size_t i = 0; i < kMinConnectionIdSize - 1; ++i) {
+ for (size_t i = 0; i < kMinSelfConnectionIdSize - 1; ++i) {
testconnid.push_back(0);
}
- EXPECT_THROW(ConnectionId{testconnid}, std::runtime_error);
+ EXPECT_NO_THROW(ConnectionId{testconnid});
}
struct ConnectionIdLengthParams {

diff:
@@ -236,7 +236,9 @@ void QuicServerWorker::handleNetworkData(
return;
}
- if (parsedLongHeader->invariant.dstConnId.size() < kMinConnectionIdSize) {
+ if (!isUsingClientConnId &&
+ parsedLongHeader->invariant.dstConnId.size() <
+ kMinSelfConnectionIdSize) {
// drop packet if connId is present but is not valid.
VLOG(3) << "Dropping packet due to invalid connectionId";
QUIC_STATS(

diff:
@@ -592,7 +592,10 @@ TEST_F(QuicServerWorkerTest, ConnectionIdTooShort) {
PacketNum num = 1;
QuicVersion version = QuicVersion::MVFST;
LongHeader header(LongHeader::Types::Initial, connId, connId, num, version);
- EXPECT_CALL(*transportInfoCb_, onPacketDropped(_));
+ EXPECT_CALL(*transportInfoCb_, onPacketDropped(_)).Times(0);
+ EXPECT_CALL(*transportInfoCb_, onPacketProcessed()).Times(1);
+ EXPECT_CALL(*transportInfoCb_, onPacketSent()).Times(1);
+ EXPECT_CALL(*transportInfoCb_, onWrite(_)).Times(1);
RegularQuicPacketBuilder builder(
kDefaultUDPSendPacketLen, std::move(header), 0 /* largestAcked */);

original_message:
remove constraints of 4-bytes minimum len for connection-id

Summary:
New QUIC draft no longer has this limitation for connection-id, and allows
connid of len 0 -> 20.

This diff removes the constraints. I still kept the requirement for
*server-chosen* conn-id

Reviewed By: mjoras, lnicco

Differential Revision: D19507366

fbshipit-source-id: 4c73f45617f40b29d47d2d86b7598f6c95588d0a


diff:
@@ -184,11 +184,11 @@ void QuicServer::bindWorkersToSocket(
const folly::SocketAddress& address,
const std::vector<folly::EventBase*>& evbs) {
auto numWorkers = evbs.size();
+ CHECK(!initialized_);
+ boundAddress_ = address;
for (size_t i = 0; i < numWorkers; ++i) {
auto workerEvb = evbs[i];
- workerEvb->runImmediatelyOrRunInEventBaseThreadAndWait(
- [address,
- self = this->shared_from_this(),
+ workerEvb->runInEventBaseThreadAndWait([self = this->shared_from_this(),
workerEvb,
numWorkers,
processId = processId_,
@@ -207,21 +207,27 @@ void QuicServer::bindWorkersToSocket(
}
// dup the takenover socket on only one worker and bind the rest
if (takeoverOverFd >= 0) {
- VLOG(4) << "Setting dup()'ed fd for address=" << address
- << " on workerId=" << (int)worker->getWorkerId();
workerSocket->setFD(
folly::NetworkSocket::fromFd(::dup(takeoverOverFd)),
// set ownership to OWNS to allow ::close()'ing of of the fd
// when this server goes away
folly::AsyncUDPSocket::FDOwnership::OWNS);
worker->setSocket(std::move(workerSocket));
+ if (idx == 0) {
+ self->boundAddress_ = worker->getAddress();
+ }
+ VLOG(4) << "Set up dup()'ed fd for address=" << self->boundAddress_
+ << " on workerId=" << (int)worker->getWorkerId();
} else {
- VLOG(4) << "No valid takenover fd found for address=" << address
- << ". binding on worker=" << worker
+ VLOG(4) << "No valid takenover fd found for address="
+ << self->boundAddress_ << ". binding on worker=" << worker
<< " workerId=" << (int)worker->getWorkerId()
<< " processId=" << (int)processId;
worker->setSocket(std::move(workerSocket));
- worker->bind(address);
+ worker->bind(self->boundAddress_);
+ if (idx == 0) {
+ self->boundAddress_ = worker->getAddress();
+ }
}
if (idx == (numWorkers - 1)) {
VLOG(4) << "Initialized all workers in the eventbase";
@@ -614,7 +620,7 @@ void QuicServer::addTransportFactory(
const folly::SocketAddress& QuicServer::getAddress() const {
CHECK(initialized_) << "Quic server is not initialized. "
<< "Consider calling waitUntilInitialized() before this ";
- return workers_[0]->getAddress();
+ return boundAddress_;
}
void QuicServer::setListeningFDs(const std::vector<int>& fds) {

diff:
@@ -367,6 +367,8 @@ class QuicServer : public QuicServerWorker::WorkerCallback,
std::unique_ptr<ConnectionIdAlgo> connIdAlgo_;
// Used to override certain transport parameters, given the client address
TransportSettingsOverrideFn transportSettingsOverrideFn_;
+ // address that the server is bound to
+ folly::SocketAddress boundAddress_;
};
} // namespace quic

original_message:
make sure all workers bind to the same address when using

Summary:
This is a bug where each worker can potentially bind to different address if no
address is supplied

Reviewed By: mjoras

Differential Revision: D19533888

fbshipit-source-id: 6f1258f09564d44ebb9843789d441c1fdecbdacc


diff:
@@ -199,7 +199,14 @@ void TakeoverPacketHandler::processForwardedPacket(
return;
}
uint16_t addrLen = cursor.readBE<uint16_t>();
+ if (addrLen > kMaxBufSizeForTakeoverEncapsulation) {
+ VLOG(2) << "Buffer size for takeover encapsulation: " << addrLen
+ << " exceeds the max limit: "
+ << kMaxBufSizeForTakeoverEncapsulation;
+ return;
+ }
struct sockaddr* sockaddr = nullptr;
+ uint8_t sockaddrBuf[addrLen];
std::pair<const uint8_t*, size_t> addrData = cursor.peek();
if (addrData.second >= addrLen) {
// the address is contiguous in the queue
@@ -207,8 +214,6 @@ void TakeoverPacketHandler::processForwardedPacket(
cursor.skip(addrLen);
} else {
// the address is not contiguous, copy it to a local buffer
- uint8_t* sockaddrBuf =
- new uint8_t[std::min(addrLen, kMaxBufSizeForTakeoverEncapsulation)];
if (!cursor.canAdvance(addrLen)) {
VLOG(4) << "Cannot extract peerAddress address of length=" << addrLen
<< " from the forwarded packet. Dropping the packet.";

original_message:
drop packet if encap length exceeds max allowed length

Reviewed By: yangchi

Differential Revision: D19822133

fbshipit-source-id: 2738058bfed00ef176f9bcc908fced1062646e1f


message: add install script for installing mvfst

diff:
+#!/bin/bash -eu
+
+# 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.
+
+## Run this script to (re)install mvfst and its dependencies (fizz
+## and folly). You must first compile all the dependencies before running this. This
+## Usually this is done by first running `build_helper.sh`.
+
+set -e
+start_dir=$(pwd)
+trap 'cd $start_dir' EXIT
+
+# Must execute from the directory containing this script
+cd "$(dirname "$0")"
+
+cd _build
+# Uninstall is expected to fail the first time
+sudo make uninstall || true
+sudo make install
+
+# Make sure the libraries are available
+# Linux only
+sudo /sbin/ldconfig || true

original_message:
add install script for installing mvfst

Reviewed By: mjoras

Differential Revision: D19914554

fbshipit-source-id: e1ee1c26849378cb6b27a4d4a889e86b5e1f06ad


diff:
@@ -136,7 +136,9 @@ void QuicServer::initializeWorkers(
const std::vector<folly::EventBase*>& evbs,
bool useDefaultTransport) {
CHECK(workers_.empty());
- for (auto& workerEvb : evbs) {
+ // iterate in the order of insertion in vector
+ for (size_t i = 0; i < evbs.size(); ++i) {
+ auto workerEvb = evbs[i];
auto worker = newWorkerWithoutSocket();
if (useDefaultTransport) {
CHECK(transportFactory_) << "Transport factory is not set";
@@ -162,7 +164,7 @@ void QuicServer::initializeWorkers(
}
worker->setConnectionIdAlgo(connIdAlgoFactory_->make());
worker->setCongestionControllerFactory(ccFactory_);
- worker->setWorkerId(workers_.size());
+ worker->setWorkerId(i);
worker->setTransportSettingsOverrideFn(transportSettingsOverrideFn_);
workers_.push_back(std::move(worker));
evbToWorkers_.emplace(workerEvb, workers_.back().get());
@@ -639,10 +641,11 @@ int QuicServer::getListeningSocketFD() const {
std::vector<int> QuicServer::getAllListeningSocketFDs() const noexcept {
CHECK(initialized_) << "Quic server is not initialized. "
<< "Consider calling waitUntilInitialized() before this ";
- std::vector<int> sockets;
+ std::vector<int> sockets(workers_.size());
for (const auto& worker : workers_) {
if (worker->getFD() != -1) {
- sockets.push_back(worker->getFD());
+ CHECK_LT(worker->getWorkerId(), workers_.size());
+ sockets.at(worker->getWorkerId()) = worker->getFD();
}
}
return sockets;

original_message:
keep worker-id and socket-id in sync with the given order of worker threads

Reviewed By: dddmello, lnicco

Differential Revision: D20240116

fbshipit-source-id: a85049d6cbc3a20c8cc86d70d6715f31384895be


diff:
*
*/
+#include <folly/Format.h>
#include <folly/io/Cursor.h>
#include <folly/io/SocketOptionMap.h>
#include <folly/system/ThreadId.h>
@@ -113,9 +114,11 @@ void QuicServerWorker::start() {
evb_, transportSettings_.pacingTimerTickInterval);
}
socket_->resumeRead(this);
- VLOG(10) << "Registered read on worker=" << this
- << " thread=" << folly::getCurrentThreadID()
- << " processId=" << (int)processId_;
+ VLOG(10) << folly::format(
+ "Registered read on worker={}, thread={}, processId={}",
+ this,
+ folly::getCurrentThreadID(),
+ (int)processId_);
}
void QuicServerWorker::pauseRead() {
@@ -195,9 +198,11 @@ void QuicServerWorker::onDataAvailable(
// TODO: we can get better receive time accuracy than this, with
// SO_TIMESTAMP or SIOCGSTAMP.
auto packetReceiveTime = Clock::now();
- VLOG(10) << "Worker=" << this
- << " Received data on thread=" << folly::getCurrentThreadID()
- << " processId=" << (int)processId_;
+ VLOG(10) << folly::format(
+ "Worker={}, Received data on thread={}, processId={}",
+ this,
+ folly::getCurrentThreadID(),
+ (int)processId_);
// Move readBuffer_ first so that we can get rid
// of it immediately so that if we return early,
// we've flushed it.
@@ -395,16 +400,19 @@ void QuicServerWorker::forwardNetworkData(
if (!routingData.isUsingClientConnId &&
!connIdAlgo_->canParse(routingData.destinationConnId)) {
if (packetForwardingEnabled_ && !isForwardedData) {
- VLOG(3) << "Forwarding packet with unknown connId version from client="
- << client << " to another process";
+ VLOG(3) << folly::format(
+ "Forwarding packet with unknown connId version from client={} to another process, routingInfo={}",
+ client.describe(),
+ logRoutingInfo(routingData.destinationConnId));
auto recvTime = networkData.receiveTimePoint;
takeoverPktHandler_.forwardPacketToAnotherServer(
client, std::move(networkData).moveAllData(), recvTime);
QUIC_STATS(statsCallback_, onPacketForwarded);
return;
} else {
- VLOG(3) << "Dropping packet due to unknown connectionId version connId="
- << routingData.destinationConnId.hex();
+ VLOG(3) << folly::format(
+ "Dropping packet due to unknown connectionId version, routingInfo={}",
+ logRoutingInfo(routingData.destinationConnId));
QUIC_STATS(
statsCallback_,
onPacketDropped,
@@ -436,12 +444,12 @@ void QuicServerWorker::dispatchPacketData(
<< routingData.destinationConnId.hex() << " " << *transport;
} else if (routingData.headerForm != HeaderForm::Long) {
// Drop the packet if the header form is not long
- VLOG(3) << "Dropping non-long header packet with no connid match CID="
- << routingData.destinationConnId << " headerForm="
- << static_cast<typename std::underlying_type<HeaderForm>::type>(
- routingData.headerForm)
- << ", workerId=" << (uint32_t)workerId_
- << ", thread=" << folly::getCurrentThreadID();
+ VLOG(3) << folly::format(
+ "Dropping non-long header packet with no connid match"
+ " headerForm={}, routingInfo={}",
+ static_cast<typename std::underlying_type<HeaderForm>::type>(
+ routingData.headerForm),
+ logRoutingInfo(routingData.destinationConnId));
// Try forwarding the packet to the old server (if it is enabled)
dropPacket = true;
}
@@ -459,14 +467,16 @@ void QuicServerWorker::dispatchPacketData(
// the case, where the new server gets packets sent to the old one due
// to network reordering
if (!routingData.isInitial) {
- VLOG(3) << "Dropping packet from client=" << client
- << ", workerId=" << (uint32_t)workerId_
- << ", thread=" << folly::getCurrentThreadID();
+ VLOG(3) << folly::format(
+ "Dropping packet from client={}, routingInfo={}",
+ client.describe(),
+ logRoutingInfo(routingData.destinationConnId));
dropPacket = true;
} else {
- VLOG(4) << "Creating new connection for client=" << client
- << ", workerId=" << (uint32_t)workerId_
- << ", thread=" << folly::getCurrentThreadID();
+ VLOG(4) << folly::format(
+ "Creating new connection for client={}, routingInfo={}",
+ client.describe(),
+ logRoutingInfo(routingData.destinationConnId));
// This could be a new connection, add it in the map
// verify that the initial packet is at least min initial bytes
@@ -523,8 +533,10 @@ void QuicServerWorker::dispatchPacketData(
auto result = sourceAddressMap_.emplace(std::make_pair(
std::make_pair(client, routingData.destinationConnId), trans));
if (!result.second) {
- LOG(ERROR) << "Routing entry already exists for client=" << client
- << ", dest CID=" << routingData.destinationConnId.hex();
+ LOG(ERROR) << folly::format(
+ "Routing entry already exists for client={}, routingInfo={}",
+ client.describe(),
+ logRoutingInfo(routingData.destinationConnId));
dropPacket = true;
}
transport = trans;
@@ -551,10 +563,8 @@ void QuicServerWorker::dispatchPacketData(
return;
}
if (!connIdAlgo_->canParse(routingData.destinationConnId)) {
- VLOG(3) << "Dropping packet with bad DCID, CID="
- << routingData.destinationConnId.hex()
- << ", workerId=" << (uint32_t)workerId_
- << ", hostId=" << (uint32_t)hostId_;
+ VLOG(3) << "Dropping packet with bad DCID, routingInfo="
+ << logRoutingInfo(routingData.destinationConnId);
QUIC_STATS(statsCallback_, onPacketDropped, PacketDropReason::PARSE_ERROR);
// TODO do we need to reset?
return;
@@ -562,22 +572,18 @@ void QuicServerWorker::dispatchPacketData(
auto connIdParam =
connIdAlgo_->parseConnectionId(routingData.destinationConnId);
if (connIdParam.hasError()) {
- VLOG(3) << "Dropping packet due to DCID parsing error="
- << connIdParam.error().what()
- << ", errorCode=" << connIdParam.error().errorCode()
- << ", DCID=" << routingData.destinationConnId.hex()
- << ", workerId=" << (uint32_t)workerId_
- << ", hostId=" << (uint32_t)hostId_;
+ VLOG(3) << folly::format(
+ "Dropping packet due to DCID parsing error={}, , errorCode={}, routingInfo={}",
+ connIdParam.error().what(),
+ folly::to<std::string>(connIdParam.error().errorCode()),
+ logRoutingInfo(routingData.destinationConnId));
QUIC_STATS(statsCallback_, onPacketDropped, PacketDropReason::PARSE_ERROR);
// TODO do we need to reset?
return;
}
if (connIdParam->hostId != hostId_) {
- VLOG(3) << "Dropping packet routed to wrong host, CID="
- << routingData.destinationConnId.hex()
- << ", workerId=" << (uint32_t)workerId_
- << ", hostId=" << (uint32_t)hostId_
- << ", received hostId=" << (uint32_t)connIdParam->hostId;
+ VLOG(3) << "Dropping packet routed to wrong host, routingInfo="
+ << logRoutingInfo(routingData.destinationConnId);
QUIC_STATS(
statsCallback_,
onPacketDropped,
@@ -618,9 +624,10 @@ void QuicServerWorker::dispatchPacketData(
// Optimistically route to another server
// if the packet type is not Initial and if there is not any connection
// associated with the given packet
- VLOG(4) << "Forwarding packet from client=" << client
- << " to another process, workerId=" << (uint32_t)workerId_
- << ", processId_=" << (uint32_t) static_cast<uint8_t>(processId_);
+ VLOG(4) << folly::format(
+ "Forwarding packet from client={} to another process, routingInfo={}",
+ client.describe(),
+ logRoutingInfo(routingData.destinationConnId));
auto recvTime = networkData.receiveTimePoint;
takeoverPktHandler_.forwardPacketToAnotherServer(
client, std::move(networkData).moveAllData(), recvTime);
@@ -840,8 +847,9 @@ void QuicServerWorker::onConnectionUnbound(
}
for (auto& connId : connectionIdData) {
- VLOG(4) << "Removing from connectionIdMap_ for CID=" << connId.connId
- << ", workerId=" << (uint32_t)workerId_;
+ VLOG(4) << folly::format(
+ "Removing CID from connectionIdMap_, routingInfo={}",
+ logRoutingInfo(connId.connId));
auto it = connectionIdMap_.find(connId.connId);
// This should be nullptr in most cases. In order to investigate if
// an incorrect server transport is removed, this will be set to the value
@@ -926,4 +934,43 @@ bool QuicServerWorker::rejectConnectionId(const ConnectionId& candidate) const
noexcept {
return connectionIdMap_.find(candidate) != connectionIdMap_.end();
}
+
+std::string QuicServerWorker::logRoutingInfo(const ConnectionId& connId) const {
+ folly::StringPiece base =
+ "CID={}, workerId={}, processId={}, hostId={}, threadId={}, ";
+ if (!connIdAlgo_->canParse(connId)) {
+ return folly::format(
+ base,
+ connId.hex(),
+ (uint32_t)workerId_,
+ (uint32_t)processId_,
+ (uint32_t)hostId_,
+ folly::getCurrentThreadID())
+ .str();
+ }
+ auto connIdParam = connIdAlgo_->parseConnectionId(connId);
+ if (connIdParam.hasError()) {
+ return folly::format(
+ base,
+ connId.hex(),
+ (uint32_t)workerId_,
+ (uint32_t)processId_,
+ (uint32_t)hostId_,
+ folly::getCurrentThreadID())
+ .str();
+ }
+ std::string extended = base.toString() +
+ "workerId in packet={}, processId in packet={}, hostId in packet={}, ";
+ return folly::format(
+ extended,
+ connId.hex(),
+ (uint32_t)workerId_,
+ (uint32_t)processId_,
+ (uint32_t)hostId_,
+ folly::getCurrentThreadID(),
+ (uint32_t)connIdParam->workerId,
+ (uint32_t)connIdParam->processId,
+ (uint32_t)connIdParam->hostId)
+ .str();
+}
} // namespace quic

diff:
@@ -361,6 +361,11 @@ class QuicServerWorker : public folly::AsyncUDPSocket::ReadCallback,
bool isInitial,
LongHeaderInvariant& invariant);
+ /**
+ * Helper method to extract and log routing info from the given (dest) connId
+ */
+ std::string logRoutingInfo(const ConnectionId& connId) const;
+
std::unique_ptr<folly::AsyncUDPSocket> socket_;
folly::SocketOptionMap* socketOptions_{nullptr};
std::shared_ptr<WorkerCallback> callback_;

original_message:
cleanup logging of routing info in QuicServerWorker

Summary: Extract out logging of all relavent routing info into a separate method

Reviewed By: sharmafb

Differential Revision: D21217339

fbshipit-source-id: 7982a4d9a3f3549a470e543e0a14aac1c6dec5f0


diff:
@@ -420,8 +420,49 @@ void QuicServerWorker::forwardNetworkData(
}
return;
}
+
+ bool userSpacePktRoute;
+ if (routingData.isUsingClientConnId) {
+ // no need to route further if it's an initial packet or zero-rtt packet
+ userSpacePktRoute = false;
+ } else {
+ // try matching the workerId in the destination Connection Id
+ auto connIdParam =
+ connIdAlgo_->parseConnectionId(routingData.destinationConnId);
+ if (UNLIKELY(connIdParam.hasError())) {
+ // not enough information to keep this packet
+ VLOG(3) << folly::format(
+ "Dropping packet due to DCID parsing error={}, , errorCode={}, routingInfo={}",
+ connIdParam.error().what(),
+ folly::to<std::string>(connIdParam.error().errorCode()),
+ logRoutingInfo(routingData.destinationConnId));
+ QUIC_STATS(
+ statsCallback_, onPacketDropped, PacketDropReason::PARSE_ERROR);
+ return;
+ }
+ if (LIKELY(connIdParam->workerId == workerId_)) {
+ userSpacePktRoute = false;
+ } else {
+ VLOG(3) << folly::format(
+ "worker-id mismatch in received packet, routingInfo={}",
+ logRoutingInfo(routingData.destinationConnId));
+ userSpacePktRoute = true;
+ }
+ }
+
+ if (userSpacePktRoute) {
callback_->routeDataToWorker(
- client, std::move(routingData), std::move(networkData), isForwardedData);
+ client,
+ std::move(routingData),
+ std::move(networkData),
+ isForwardedData);
+ } else {
+ dispatchPacketData(
+ client,
+ std::move(routingData),
+ std::move(networkData),
+ isForwardedData);
+ }
}
void QuicServerWorker::setPacingTimer(

diff:
@@ -153,21 +153,6 @@ class QuicServerWorkerTest : public Test {
worker_->setCongestionControllerFactory(
std::make_shared<DefaultCongestionControllerFactory>());
transportInfoCb_ = (MockQuicStats*)worker_->getTransportStatsCallback();
-
- auto cb = [&](const folly::SocketAddress& addr,
- std::unique_ptr<RoutingData>& routingData,
- std::unique_ptr<NetworkData>& networkData,
- bool isForwardedData) {
- worker_->dispatchPacketData(
- addr,
- std::move(*routingData.get()),
- std::move(*networkData.get()),
- isForwardedData);
- };
-
- EXPECT_CALL(*workerCb_, routeDataToWorkerLong(_, _, _, _))
- .WillRepeatedly(Invoke(cb));
-
socketFactory_ = std::make_unique<MockQuicUDPSocketFactory>();
EXPECT_CALL(*socketFactory_, _make(_, _)).WillRepeatedly(Return(nullptr));
worker_->setNewConnectionSocketFactory(socketFactory_.get());
@@ -1055,16 +1040,6 @@ void QuicServerWorkerTakeoverTest::testNoPacketForwarding(
Buf /* data */,
size_t len,
ConnectionId /* connId */) {
- auto cb = [&](const folly::SocketAddress& addr,
- std::unique_ptr<RoutingData>& /* routingData */,
- std::unique_ptr<NetworkData>& /* networkData */,
- bool isForwardedData) {
- EXPECT_EQ(addr.getIPAddress(), clientAddr.getIPAddress());
- EXPECT_EQ(addr.getPort(), clientAddr.getPort());
- EXPECT_FALSE(isForwardedData);
- };
- EXPECT_CALL(*takeoverWorkerCb_, routeDataToWorkerLong(_, _, _, _))
- .WillOnce(Invoke(cb));
EXPECT_CALL(*transportInfoCb_, onPacketReceived());
EXPECT_CALL(*transportInfoCb_, onRead(len));
EXPECT_CALL(*transportInfoCb_, onPacketForwarded()).Times(0);
@@ -1163,19 +1138,6 @@ void QuicServerWorkerTakeoverTest::testPacketForwarding(
return data->computeChainDataLength();
}));
takeoverWorker_->startPacketForwarding(folly::SocketAddress("0", 0));
-
- auto cb = [&](const folly::SocketAddress& client,
- std::unique_ptr<RoutingData>& routingData,
- std::unique_ptr<NetworkData>& networkData,
- bool isForwardedData) {
- takeoverWorker_->dispatchPacketData(
- client,
- std::move(*routingData.get()),
- std::move(*networkData.get()),
- isForwardedData);
- };
- EXPECT_CALL(*takeoverWorkerCb_, routeDataToWorkerLong(_, _, _, _))
- .WillOnce(Invoke(cb));
EXPECT_CALL(*transportInfoCb_, onPacketReceived());
EXPECT_CALL(*transportInfoCb_, onRead(len));
EXPECT_CALL(*transportInfoCb_, onPacketForwarded()).Times(1);
@@ -1226,40 +1188,11 @@ TEST_F(QuicServerWorkerTakeoverTest, QuicServerTakeoverProcessForwardedPkt) {
size_t bufLen =
std::min(workerBufLen, writtenData->computeChainDataLength());
memcpy(workerBuf, writtenData->buffer(), bufLen);
-
- // test processing of the forwarded packet
- auto cb = [&](const folly::SocketAddress& addr,
- std::unique_ptr<RoutingData>& /* routingData */,
- std::unique_ptr<NetworkData>& networkData,
- bool isForwardedData) {
- // verify that it is the original client address
- EXPECT_EQ(addr.getIPAddress(), clientAddr.getIPAddress());
- EXPECT_EQ(addr.getPort(), clientAddr.getPort());
- // the original data should be extracted after processing takeover
- // protocol related information
- EXPECT_EQ(networkData->packets.size(), 1);
- EXPECT_TRUE(eq(*data, *(networkData->packets[0])));
- EXPECT_TRUE(isForwardedData);
- };
- EXPECT_CALL(*takeoverWorkerCb_, routeDataToWorkerLong(_, _, _, _))
- .WillOnce(Invoke(cb));
-
takeoverCb->onDataAvailable(
client, bufLen, false, OnDataAvailableParams());
return bufLen;
}));
- auto workerCb = [&](const folly::SocketAddress& client,
- std::unique_ptr<RoutingData>& routingData,
- std::unique_ptr<NetworkData>& networkData,
- bool isForwardedData) {
- takeoverWorker_->dispatchPacketData(
- client,
- std::move(*routingData.get()),
- std::move(*networkData.get()),
- isForwardedData);
- };
- EXPECT_CALL(*takeoverWorkerCb_, routeDataToWorkerLong(_, _, _, _))
- .WillOnce(Invoke(workerCb));
+
EXPECT_CALL(*transportInfoCb_, onPacketReceived());
EXPECT_CALL(*transportInfoCb_, onRead(len));
EXPECT_CALL(*transportInfoCb_, onPacketForwarded()).Times(1);
@@ -1334,9 +1267,8 @@ class QuicServerTest : public Test {
if (stats) {
CHECK_EQ(evbs.size(), 1);
EXPECT_CALL(*transportStatsFactory_, make())
- .WillRepeatedly(Invoke([stats]() {
- return std::unique_ptr<MockQuicStats>(stats);
- }));
+ .WillRepeatedly(Invoke(
+ [stats]() { return std::unique_ptr<MockQuicStats>(stats); }));
} else {
EXPECT_CALL(*transportStatsFactory_, make()).WillRepeatedly(Invoke([&]() {
auto mockInfoCb = std::make_unique<NiceMock<MockQuicStats>>();

original_message:
avoid unnecessary userspace routing of packets

Summary:
for initial or zero-rtt packets, or in the case of short-header packets, if the
worker-id already matches, we do not need to do any userspace routing of
packets.

Reviewed By: mjoras

Differential Revision: D21219335

fbshipit-source-id: 988614530960a621a02c0375f4a507b333f66e69


diff:
@@ -414,11 +414,13 @@ void QuicServerWorker::forwardNetworkData(
"Dropping packet due to unknown connectionId version, routingInfo={}",
logRoutingInfo(routingData.destinationConnId));
QUIC_STATS(
- statsCallback_,
- onPacketDropped,
- PacketDropReason::CONNECTION_NOT_FOUND);
+ statsCallback_, onPacketDropped, PacketDropReason::PARSE_ERROR);
+ return sendResetPacket(
+ routingData.headerForm,
+ client,
+ networkData,
+ routingData.destinationConnId);
}
- return;
}
bool userSpacePktRoute;
@@ -438,7 +440,11 @@ void QuicServerWorker::forwardNetworkData(
logRoutingInfo(routingData.destinationConnId));
QUIC_STATS(
statsCallback_, onPacketDropped, PacketDropReason::PARSE_ERROR);
- return;
+ return sendResetPacket(
+ routingData.headerForm,
+ client,
+ networkData,
+ routingData.destinationConnId);
}
if (LIKELY(connIdParam->workerId == workerId_)) {
userSpacePktRoute = false;
@@ -601,14 +607,20 @@ void QuicServerWorker::dispatchPacketData(
statsCallback_,
onPacketDropped,
PacketDropReason::CANNOT_MAKE_TRANSPORT);
+ // TODO reevaluate sending stateless reset here. At this state, the peer
+ // will not have received the token yet. So these reset packets will just
+ // be dropped by the peer.
return;
}
if (!connIdAlgo_->canParse(routingData.destinationConnId)) {
VLOG(3) << "Dropping packet with bad DCID, routingInfo="
<< logRoutingInfo(routingData.destinationConnId);
QUIC_STATS(statsCallback_, onPacketDropped, PacketDropReason::PARSE_ERROR);
- // TODO do we need to reset?
- return;
+ return sendResetPacket(
+ routingData.headerForm,
+ client,
+ networkData,
+ routingData.destinationConnId);
}
auto connIdParam =
connIdAlgo_->parseConnectionId(routingData.destinationConnId);
@@ -619,8 +631,11 @@ void QuicServerWorker::dispatchPacketData(
folly::to<std::string>(connIdParam.error().errorCode()),
logRoutingInfo(routingData.destinationConnId));
QUIC_STATS(statsCallback_, onPacketDropped, PacketDropReason::PARSE_ERROR);
- // TODO do we need to reset?
- return;
+ return sendResetPacket(
+ routingData.headerForm,
+ client,
+ networkData,
+ routingData.destinationConnId);
}
if (connIdParam->hostId != hostId_) {
VLOG(3) << "Dropping packet routed to wrong host, routingInfo="

original_message:
send reset instead of silently dropping packets due to misrouting

Summary:
There were some TODOs left in the QuicServerWorker where we silently drop
packets.

Reviewed By: mjoras, yangchi

Differential Revision: D21235150

fbshipit-source-id: eb546ea6d792915cf3155370758e837318530a38


diff:
@@ -658,7 +658,7 @@ void QuicServerWorker::dispatchPacketData(
return;
}
if (connIdParam->hostId != hostId_) {
- VLOG(3) << "Dropping packet routed to wrong host, routingInfo="
+ VLOG_EVERY_N(2, 100) << "Dropping packet routed to wrong host, routingInfo="
<< logRoutingInfo(routingData.destinationConnId);
QUIC_STATS(
statsCallback_,

original_message:
log misrouting of packets at host level with increased verbosity

Reviewed By: lnicco

Differential Revision: D21988330

fbshipit-source-id: 33978dd66bf10f929994dd147ec531691b67a1ee


diff:
@@ -42,6 +42,7 @@ std::unique_ptr<QuicClientConnectionState> undoAllClientStateForRetry(
newConn->originalVersion = conn->originalVersion;
newConn->originalPeerAddress = conn->originalPeerAddress;
newConn->peerAddress = conn->peerAddress;
+ newConn->udpSendPacketLen = conn->udpSendPacketLen;
newConn->peerMaxPacketSize = conn->peerMaxPacketSize;
newConn->currentPMTU = conn->currentPMTU;
newConn->supportedVersions = conn->supportedVersions;
@@ -152,10 +153,14 @@ void processServerInitialParams(
}
conn.peerAckDelayExponent =
ackDelayExponent.value_or(kDefaultAckDelayExponent);
+
+ // TODO: udpSendPacketLen should also be limited by PMTU
+ if (conn.transportSettings.canIgnorePathMTU) {
if (*packetSize > kDefaultMaxUDPPayload) {
*packetSize = kDefaultUDPSendPacketLen;
}
updateUdpSendPacketLen(conn, *packetSize);
+ }
// Currently no-op for a client; it doesn't issue connection ids
// to the server.
@@ -233,7 +238,7 @@ void updateTransportParamsFromCachedEarlyParams(
const CachedServerTransportParameters& transportParams) {
conn.peerIdleTimeout = std::chrono::milliseconds(transportParams.idleTimeout);
if (conn.transportSettings.canIgnorePathMTU) {
- conn.peerMaxPacketSize = transportParams.maxRecvPacketSize;
+ updateUdpSendPacketLen(conn, transportParams.maxRecvPacketSize);
}
conn.flowControlState.peerAdvertisedMaxOffset =
transportParams.initialMaxData;

diff:
@@ -176,10 +176,13 @@ void processClientInitialParams(
}
conn.peerAckDelayExponent =
ackDelayExponent.value_or(kDefaultAckDelayExponent);
+ // TODO: udpSendPacketLen should also be limited by PMTU
+ if (conn.transportSettings.canIgnorePathMTU) {
if (*packetSize > kDefaultMaxUDPPayload) {
*packetSize = kDefaultUDPSendPacketLen;
}
updateUdpSendPacketLen(conn, *packetSize);
+ }
conn.peerActiveConnectionIdLimit =
activeConnectionIdLimit.value_or(kDefaultActiveConnectionIdLimit);

original_message:
Respect canIgnorePMTU transport setting

Summary: Mitigate a regression caused by ignoring this setting.

Reviewed By: mjoras, lnicco

Differential Revision: D23267566

fbshipit-source-id: dc93116c0dab21c9070a4245c4e4eb9dcd9d2de1