Summary:
Store timestamps/packet numbers of recently received packets in AckState.
- The maximum number of packets stored is controlled by kMaxReceivedPktsTimestampsStored.
- The packet number of entries in the deque is guarenteed to increase
monotonically because an entry is only added for a received packet
if the packet number is greater than the packet number of the last
element in the deque (e.g., entries are not added for packets that
arrive out of order relative to previously received packets).
Reviewed By: bschlinker
Differential Revision: D37799023
fbshipit-source-id: 3b6bf2ba8ea15219a87bbdc2724fe23eebe66b70
Summary: PacketDropReason was converted to a Better_Enum
Reviewed By: jbeshay
Differential Revision: D40350056
fbshipit-source-id: a6af9ccf0fc7c4358a0481de5cca6f69d1beb438
Summary: This changes the experimental behavior of BBR sending ACK_FREQUENCY frames. Now instead of using the static ack eliciting threshold, early in the connection BBR will set the threshold to 2 which has been empirically shown to be a good "initial" value.
Reviewed By: jbeshay
Differential Revision: D40438721
fbshipit-source-id: 184b8025581b6152eea97f9b557b6343d980322d
Summary:
This introduces the ability for BBR to control ACK_FREQUENCY. It also introduces a knob which will let us experiment with different policies.
This code makes BBR issue a new ACK_FREQUENCY every round trip. This may be excessive, so BBR will track the last frame sent, and only send a new one if the new value is sufficiently different in terms of milliseconds.
Note that the only dynamic part of the frame currently will be the max ack delay, which is in terms of fractional RTTs. It is not clear yet how we should vary the other parameters dynamically.
Reviewed By: kvtsoy
Differential Revision: D39910417
fbshipit-source-id: 73cfdac40ca52185c33322017a893e5a02ee58e8
Summary:
The `QuicServerTransportTestBase` test fixture accepts the connection before beginning to run any of the test code. However, if a test wants to be able to modify the transport settings, it needs to be able to modify the transport before the connection has been accepted and the transport parameters have been encoded (e.g., before `transportParametersEncoded` is set to true).
This diff adds a `startTransport` function to `QuicServerTransportTestBase` that does the work of accepting the connection, and then adds a new test fixture `QuicServerTransportAfterStartTestBase` — named after similar existing class `QuicClientTransportAfterStartTestBase` — which calls `startTransport` during setup. All tests previously using `QuicServerTransportTestBase` are switched to `QuicServerTransportAfterStartTestBase`.
For clients, we already had `QuicClientTransportAfterStartTestBase`. A `startTransport` function is also added to `QuicClientTransportTestBase` and the same is done. I need to investigate the client side a bit further as we also have a `start` function there, but there are many layers on `QuicClientTransportTestBase` and that will take some time to unwind.
Differential Revision: D39274871
fbshipit-source-id: 6de47d463637d2e93b808de680d52cc8820d9d47
Summary:
- continually issuing new connection ids to peer as old connections ids are retired through RETIRE_CONN_ID frames
- add logic to parse and act on receiving RETIRE_CONN_ID frame
Reviewed By: mjoras
Differential Revision: D38443561
fbshipit-source-id: 82fb679f482fd69c7b3a3385693d2e5575e92703
Summary: Add the new IMMEDIATE_ACK frame from the ack frequency draft.
Reviewed By: mjoras
Differential Revision: D38523438
fbshipit-source-id: 79f4a26160ccf4333fb79897ab4ace2ed262fa01
Summary:
From [RFC9000](https://datatracker.ietf.org/doc/html/rfc9000)
```
Note that it is not possible to send the following frames in 0-RTT
packets for various reasons: ACK, CRYPTO, HANDSHAKE_DONE, NEW_TOKEN,
PATH_RESPONSE, and RETIRE_CONNECTION_ID. A server MAY treat receipt
of these frames in 0-RTT packets as a connection error of type
PROTOCOL_VIOLATION.
```
Reviewed By: jbeshay, mjoras
Differential Revision: D38642380
fbshipit-source-id: a1a7167c06fa68037758ce5395c3798479c26b42
Summary: Handle MAX_PACING_RATE_KNOB_SEQUENCED transport knob with which a client can suggest a limit on the pacing rate used by the server, if it has pacing enabled. This knob also include a sequence number with which the server can use to determine out-of-order frames.
Reviewed By: bschlinker
Differential Revision: D37062350
fbshipit-source-id: 981f0c9df99cf30de6c676784409a43d45f62ff2
Summary:
Add a test for dropping datagram on the floor after receiving conn
close
Reviewed By: hanidamlaj
Differential Revision: D37532252
fbshipit-source-id: d98d34a6bf692cf719864fa01120676b63bbbc1d
Summary: - unit test to verify that the writableBytesLimit is bypassed upon receiving a valid new token
Reviewed By: mjoras
Differential Revision: D36645447
fbshipit-source-id: 2c4163414c1d04f51cd473b07e32fcd78fbe1bda
Summary:
- PTOs should not be subject to congestion control limits
- Quickly recover from PTOs being writableBytesLimited by calling onPTOAlarm() as soon as we become unblocked
Reviewed By: mjoras
Differential Revision: D35480409
fbshipit-source-id: 51500db6fff17a7badefea8bda7f63141e97f746
Summary:
This diff is part of larger change to switch to using `folly::ObserverContainer` (introduced in D27062840).
This diff:
- Changes `LegacyObserver` to inherit from the new `Observer` class by adding a compatibility layer. This compatibility layer enables existing observers to continue to be supported.
- Changes generation of observer events so that they are routed through `ObserverContainer::invokeInterfaceMethod`
- Temporarily removes some of the reentrancy protection that previously existed, as it was not being applied consistently — some events had reentrancy protection, some did not. This will be reintroduced in the next diff.
- Improves some unit tests for observers during the transition process.
Differential Revision: D35268271
fbshipit-source-id: 5731c8a9aa8da8a2da1dd23d093e5f2e1a692653
Summary:
This diff is part of larger change to switch to using `folly::ObserverContainer` (introduced in D27062840).
This diff:
- Renames `quic::Observer` to `quic::LegacyObserver`.
- Switches to using `quic::SocketObserverInterface` instead of `quic::Observer` when referencing structures used for the observer interface. For instance, `quic::Observer::WriteEvent` changes to `quic::SocketObserverInterface::WriteEvent`.
Differential Revision: D35000152
fbshipit-source-id: f387231fd58e8e763c3d7ed0a9c6fcec3b2324e2
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
