Summary:
This implements a basic keepalive timer at the transport layer.
The basic idea is to leverage the same conditions as the idle timer. The keepalive timer will fire ~15% before the idle timer and trigger a PING frame. This has the effect of keeping the connection open if the PING is acknowledged.
Reviewed By: jbeshay
Differential Revision: D34256174
fbshipit-source-id: a8cee579f5b09bdfa23bcdc7cf4c70de299de3bd
Summary:
Add an experimental setting in the TokenlessPacer to use an adjustable burstSize in the to compensate for delays in pacer loop timer with a limit of 5 loop intervals.
This practically restores some concept of tokens in the pacer. This should help with pacer timer delays observed in relatively high BDP links.
To help quantify how often the delays happen, the QuicStats pacer_lagged counter is updated every time the pacer is delayed by more than 10% when the connection is not application limited.
In a test configuration across a 150 Mbps link with 150 ms round-trip delay, the experimental setting achieves ~25% higher bandwidth.
Reviewed By: bschlinker, mjoras
Differential Revision: D33035430
fbshipit-source-id: 4aa02821d609facf6c830647459319d44c6fc3bd
Summary:
Maine change is `MockConnectionCallback` -> `MockConnectionSetupCallback` + `MockConnectionCallbackNew`.
Everything else is changing tests to use the two new classes.
Differential Revision: D33076321
fbshipit-source-id: a938b63ce59f07f549b3e725caad8785348db7ed
Summary:
- Wrap the TokenLength in a QuicInteger
- Validate token written by server equals token parsed by client codec
Reviewed By: mjoras
Differential Revision: D33134186
fbshipit-source-id: fc003979d9bacdb9ab9bb563a300cedd7a99d58a
Summary: When there is not enough space in the QUIC packet to write a datagram frame, we should not dequeue the datagram
Reviewed By: mjoras
Differential Revision: D33109603
fbshipit-source-id: 937a5a0ffe55c7f88e39faf224e7ad06ca599708
Summary:
Add experimental CUBIC's hystart delay bounds that ensure CUBIC does exit Hystart too early. Current limits of 2,8 us are too short.
On a 100 Mbps link with 150 ms round-trip delay, this improved CUBIC's throughput from ~14 Mbps to ~80 Mbps.
Reviewed By: mjoras
Differential Revision: D33035403
fbshipit-source-id: bdeb081111f8053bc7fb01002316d991d8bac2e0
Summary:
Fixing four issues in open source build.
First, two missing changes to `CMakeLists.cpp` changes
- `handshake/TokenGenerator.cpp`, added in D31673160 (7233c55d29)
- `state/AckEvent.cpp`, added in D31221302 (7a916869ff)
The two two issues are caused by inconsistencies between our build env and open source build env, including gtest versions. We should investigate getting open source gtest on same version.
---
**[D31216724 (20c17a882b)] We cannot use lambdas with `testing::ResultOf` for older versions of gtest and/or older compilers.**
```
gmock-matchers.h:2310:29: error: no type named 'result_type' in '(lambda at /data/sandcastle/temp/fbcode_builder_getdeps/shipit/mvfst/quic/api/test/QuicTransportTest.cpp:711:19)'
```
In particular, the `decltype(f(arg))` logic in the following code from gtest is going to fail on the lambda, since a lambda has no type. I believe it would need to be a `declval(decltype(f(arg)))` for the lambda to work.
```
template <typename Functor>
struct CallableTraits {
typedef Functor StorageType;
static void CheckIsValid(Functor /* functor */) {}
template <typename T>
static auto Invoke(Functor f, const T& arg) -> decltype(f(arg)) {
return f(arg);
}
};
```
See this for details: https://stackoverflow.com/questions/4846540/c11-lambda-in-decltype
-----
**[D32772165 (e784fafb10)] `EXPECT_CALL` does not work with the formatting used for older versions of gtest and/or older compilers (again, unclear).**
Specifically
```
EXPECT_CALL(handshakeFinishedCallback, onHandshakeFinished);
```
must instead be
```
EXPECT_CALL(handshakeFinishedCallback, onHandshakeFinished());
```
Reviewed By: kvtsoy
Differential Revision: D33026131
fbshipit-source-id: 886a6165f2e217cadbe479320195abbd4895eb7c
Summary:
- Issuing NewTokenFrames to clients, allowing them to verify their address in subsequent connections by including the token.
- add NewTokenFrame struct in the union type QuicSimpleFrame.
- Issued only once when the crypto handshake is complete.
- Testing includes validating token serialization & deserialization and asserting that the NewTokenFrame is only issued once on handshake completeness.
Reviewed By: mjoras
Differential Revision: D31673160
fbshipit-source-id: 9401ab1a4b878d8b4380d55afa531ec768f5f4cd
Summary:
This implements a global (per process) limit on unfinished handshakes from unverified source addresses.
This limits the ability of an attacker to create connection state without also allocating connection state themselves. By default the limit is 1024.
Reviewed By: kvtsoy
Differential Revision: D32772165
fbshipit-source-id: 6c195169377a9f687c54bc9782cc58fe085e1275
Summary:
Introduces `QuicTypedTransportTest`, a `TYPED_TEST` that will enable us to test various implementations of `QuicTransportBase` with only one set of tests.
Today, we write separate tests in `QuicClientTransportTest` and `QuicServerTransportTest` for the same functionality. Due to this overhead, I've noticed that in some cases we don't have proper test coverage, and this leads to bugs (see the off by one issue with ACK byte events).
`QuicTypedTransportTest` is an effort to fix this by making it easier to write tests that all transports should be able to support. `QuicTypedTransportTestBase` serves as a shim layer between the test and the specific test transport implementation. In following diffs, I extend the functionality of `QuicTypedTransportTest` further to make it easier to test cases where packets arrive from the remote (peer), including stream and reset packets.
Reviewed By: mjoras
Differential Revision: D32560835
fbshipit-source-id: e08387364f614c37ed2126dbfb91b30230d116a0
Summary:
- Skip ACK-only response to initial crypto data from client
- Enabled through experimental transport settings
Reviewed By: mjoras
Differential Revision: D31863653
fbshipit-source-id: b290db0399dd7a3be41078c4a839b484da864f2f
Summary: Making it possible to reuse the foundation of `QuicServerTransportTest` elsewhere. Will be building a test that uses `TYPED_TEST` to test against both transports.
Reviewed By: lnicco
Differential Revision: D31886976
fbshipit-source-id: a158d94e1a75866e21f505f21f0f7401326e76f4
Summary: rename test local variables to be self documenting
Reviewed By: mjoras
Differential Revision: D32750782
fbshipit-source-id: 94ff5bbd34dbc804cd0229d8abd0ffd9891a44fc
Summary:
- Change the knob param handler to receive a Transport rather than a ConnectionState.
- Add a new transport knob that can set the priority threshold and target utilization factor for automatically controlling background mode based upon active streams.
Reviewed By: mjoras
Differential Revision: D31847477
fbshipit-source-id: 4cfeff04b9fe60eb25ab484f460e252ef6970646
Summary:
Experiment with a less aggressive variant of BBR that could be used for background requests in order to improve responsiveness of foreground requests.
BBR's agressiveness can be controlled through a new transport knob CC_AGRESSIVENESS_KNOB(0xccab) which accepts values between 25-100. The value indicates the percentage of the available BW the congestion controller should try to use, leaving headroom for other flows.
Aggressiveness of BBR is reduced by adjusting BBR's startup gain during the STARTUP phase. During the ProbeBW phase, BBR uses a longer pacing gain progression with values that intentionally underutilize the available BW.
Reviewed By: bschlinker, mjoras
Differential Revision: D31219364
fbshipit-source-id: c67ffcf03d27fd4d41be2d58f00f4f960c7646c7
Summary: As in title. This isn't actually very risky to have on, and is actually confusing if you try to use the APIs without the flag since nothing will happen.
Reviewed By: shodoco
Differential Revision: D30585491
fbshipit-source-id: d79598ab5c723675d63cb71f011b9107acda6d7d
Summary: It's useful at the end of a connection to know if we tried DSR.
Reviewed By: jbeshay
Differential Revision: D30545282
fbshipit-source-id: bbb2f3408f7a2d5666676c9e2583bf8fd9f18911
Summary:
- Removed packetNum field from CipherUnavailable struct.
- Removed all instances referring to the field and fixed tests accordingly.
Reviewed By: mjoras
Differential Revision: D29968168
fbshipit-source-id: 9802b8cd66f43f2a8d54340f2d00639ee4679aaf
Summary:
clear DSR buffers, release flow control, calling release() on the
packetization request sender. For now the stream will own the sender until
stream itself is dead. We need to change this ownership model later to be able
to reset the pointer when we reset the stream.
Reviewed By: mjoras
Differential Revision: D27901663
fbshipit-source-id: d9d12ef95ae59c6f0fe7ac1b1589d8527b1bc48d
Summary:
This diff hooks the DSR write function into QuicServerTransport's
write path.
Reviewed By: mjoras
Differential Revision: D27890711
fbshipit-source-id: ac4452373c0baafe091f93fb54fccf87be604a9c
Summary:
will use it for other tests as well
(Note: this ignores all push blocking failures!)
Reviewed By: lnicco
Differential Revision: D27024118
fbshipit-source-id: 27b0182525787aa2969f789b1e4eb8deca296e69
Summary:
as title. This also moves a FizzCryptoTestFactory from FizzCryptoFactoryTest to TestUtils so that it can be used in other test code
This change has an unfortunate side-effect that cryptoFactory_ in both client and server will be moved from stack to heap.
Reviewed By: mjoras
Differential Revision: D27264488
fbshipit-source-id: febc307fb02cb136d58fe70bee648d35431acff0
Summary: This seems to cause some issues with CGNAT type networks where the client is actually using v6 or v4.
Reviewed By: yangchi
Differential Revision: D27772485
fbshipit-source-id: ac118441caad38301f2a22e657cefb398a5210da
Summary:
On receiving a QUIC packet, if the packet has no frames we should end the connection with PROTOCOL_VIOLATION.
This fixes the error reported by h3spec test `/QUIC servers/MUST send PROTOCOL_VIOLATION on no frames [Transport 12.4]/`
This change adds the check right after a packet is successfully parsed for both the client and server.
Reviewed By: mjoras
Differential Revision: D27483874
fbshipit-source-id: 9b648709e6985f151ba0ffc973aa05c28683fbe9
Summary:
As before we will now aggressively send probes on all spaces with probes available when the PTO timer fires.
This time with more unit tests and some bug fixes.
Reviewed By: yangchi
Differential Revision: D27338523
fbshipit-source-id: 8a9ccb90ed691e996fab4afa2f132c0f99044fbc
Summary: As in title. There's a bug here somewhere with empty write loops we need to find.
Reviewed By: yangchi
Differential Revision: D27279100
fbshipit-source-id: e1d26fbf8d6df1590d464a6504a8b940b46794e0
Summary:
Previously we would only send probes for the first space which had one available, i.e. Initial before Handshake before AppData. Since we only have one PTO timer this can lead to situations where we perpetually probe with only Initials, which can significantly delay the handshake if we should have probed with Handshakes.
With this diff we will keep the single PTO timer but aggressively write more probes from all spaces if they are available.
Additionally this refactors some counters into EnumArrays
Reviewed By: yangchi
Differential Revision: D27235199
fbshipit-source-id: ef3614a833bf0f02f5806846a1335fa7ac2a4dc8
Summary:
Before the change, there's no good way to recreate Cubic CC instance with custom CC factory, because Cubic is created by default.
On client side this requires calling setCongestionControl() or setTransportSettings() after calling setCongestionControllerFactory(), which is normally the case.
Reviewed By: yangchi
Differential Revision: D26401996
fbshipit-source-id: dfda39be835c67b9db42f726b3ac64c7b3d37c2f
Summary:
Having the lines `using namespace testing;` and `using namespace folly;` or `using folly::Optional;` causes a build error with the new googletest version because of `testing::Optional` being a new symbol from googletest
## How I made this diff:
From the list of files
[rquitt@devbig013.ftw5 ~/fbsource/fbcode/scripts/rquitt] grep -r "'Optional' is ambiguous" | sed 's/stderr: //' | cut -d ':' -f 2 | sort -u
admarket/adreview/video_understanding/server/test/VusMothershipManagerTest.cpp
admarket/adreview/video_understanding/server/test/VusTaskManagerTest.cpp
archive_service/blockio/SdcLogicalBlockWriterTest.cpp
...
Open all the files in Vim and use a command like: `:%s/[^y/:]Optional/folly::Optional/`
Also removed all the `using folly::Optional;` lines to discourage further use of ambiguous `Optional` before googletest change is landed.
The alternative to this diff would be to not export `testing::Optional` from googletest.
Reviewed By: igorsugak
Differential Revision: D26358560
fbshipit-source-id: ae8695b3525bf333758b012adcfe944383777625
Summary: this param is passed to transport then ignored
Reviewed By: avasylev
Differential Revision: D26133327
fbshipit-source-id: 459dd0132185513215ba034f213d4137d7b56ba1
Summary:
Given the large number of callbacks that are being triggered from the Observer
this change makes it possible to enable through a simple config, just the
subset of callbacks that a consumer is interested in receiving.
Observer and Socket Lifecycle callbacks are enabled by default, they are not
configurable.
Reviewed By: bschlinker
Differential Revision: D25879382
fbshipit-source-id: abe79ed92e958ddc96475c347f8ec7204400cdfa
Summary:
We were using the LifecycleObserver and InstrumentationObserver classes
separately, to generate and receive callbacks.
This change migrates both these to use the unified Observer callback class and
adjusts the unit tests.
Reviewed By: bschlinker
Differential Revision: D25845845
fbshipit-source-id: c489400f5d70bccadbcc1d957136c5ade36b65ff
Summary:
I think this should just work without the trailing `_E`. It was added
when we mixed up our own union based variant and boost::variant. Some compiler
flags didn't like that. Now we no longer have mixed up cases, this should be
fine
Reviewed By: lnicco
Differential Revision: D25589393
fbshipit-source-id: 6430dc20f8e81af0329d89e6990c16826da168b8
Summary: Adds another knob param to enforce udp payload size. This is basically a "canIgnorePathMTU" knob that client has.
Reviewed By: mjoras
Differential Revision: D24586165
fbshipit-source-id: befb265a24fae8f450f323cf2d652a8b448e698c
Summary: Blackhole detection has quite a lot false positives, we want to have a way to run d6d without it.
Reviewed By: mjoras
Differential Revision: D24584357
fbshipit-source-id: ab27655ec38d62fd6deffe41cb156de0c981cf6d
