Summary:
As title. This changes the NewTokenFrame (for writing) to hold the toke as an IOBuf instead of a string. This makes it consistent with the read side.
In the same change, the qlogger now hexlifies this buffer when writing the token to the qlog.
Reviewed By: hanidamlaj, mjoras
Differential Revision: D46955172
fbshipit-source-id: 8f5f575ad2f0a4ec3b4c01cb67defa1f4425cd74
Summary:
Right now small packets will cause us to subtract from the packet limit during write loops. This is generally okay when using just non-DSR data. For DSR though there is an issue where if we only write a single small packet from the non-DSR path (like an ACK) we will discount it from the amount of packets the DSR path can write.
The existing workaround for this is to only discount the non-DSR path write if the number of bytes written is less than half a packet's worth. This heuristic is incomplete though and doesn't consider the cases like the following:
1. Packet limit is 5
2. Normal path writes 4 packets of size 1232, 1232, 1232, 20.
3. The DSR path only gets one packet.
The reason for this is because the normal path also wrote full packets, but ended with a short packet. The account for this we just have to discount in terms of full packet's written. In the above example, the DSR path would get the chance to write two packets. Thus we would exceed the packet limit by one packet, but it would get us closer to the desired amount of data on the wire.
Reviewed By: kvtsoy
Differential Revision: D46633105
fbshipit-source-id: 00d92499ab73fc746ea5fdb6ff31e10f06b98666
Summary:
The idea here is to allow a way to do incremental stream priorities while switching between streams as aggressively.
Achieving this is somewhat tricky. The easiest place to track this is to make it so the iterator in QuicPriorityQueue no-op next() until a counter reaches an increment.
This is especially impacftul for DSR, where round robining per packet is almost pathologically bad both for CPU impact but also spurious losses and low bandwidth estimates.
Thoughts?
(Note: this ignores all push blocking failures!)
Reviewed By: kvtsoy
Differential Revision: D46268308
fbshipit-source-id: cd5b924141365f61f8a3363bc9cb38a62e5c94cf
Summary:
This has been hardcoded to SRTT/25 for a long time. However, especially when using DSR this might not be the most appropriate since it can start to get close to real world SRTTs.
Control it via a knob, and also add the behavior such that setting it to 0 effectively disables the time limit.
Reviewed By: jbeshay
Differential Revision: D46084438
fbshipit-source-id: 7dc619fdff1bd5c3f8654c4936200e0340ef94f2
Summary: Allows the default stream priority (which also means scheduling policy) to be set via TransportSettings.
Reviewed By: jbeshay, hanidamlaj
Differential Revision: D45881729
fbshipit-source-id: fcb72022cd6eac2f1dafc55173d5a46a72a95dbc
Summary:
Due to the fact that store DSR and non DSR streams in separate priority queues, we end up having to make a decision on which to write first.
Longer term we should merge them into a single priority queue, but in the meantime to mitigate potential starvation, randomly flip flop between which we write first in a given write loop.
Reviewed By: jbeshay, shodoco
Differential Revision: D45701230
fbshipit-source-id: 894179e457a4d6f7364767108f9290ff267eb977
Summary:
To get reliable packet destruction events, created a `OutstandingPacketWrapper` wrapper that wraps the current `OutstandingPacket` class, so callback functions can be added to the wrapper instead of the underlying object itself.
#### Why do we need this wrapper?
`std::deque::erase` does not guarantee that appropriate object destructors will be called (only that the number of destructions = number of objects erased). This is a real problem as packet destruction events are then no longer reliable (OutstandingPacket object is wrong!). The wrapper class handles this condition by detecting it in the move assignment constructor (called during erase) and calls the appropriate packet destruction callback before packets are moved. If we did the same fix in the old OutstandingPacket, we have to make sure the callback is called before all the fields of OutstandingPacket are moved - this is not scaleable. Hence a wrapper with the underlying object and a destruction callback function.
I also disabled copy construction for OutstandingPacket (otherwise we will get duplicate OnPacketDestroyed callbacks and cannot track packets reliably). Removing packet copies also improves performance. Some code changes (in tests mostly) are mostly in service of this particular change.
Reviewed By: bschlinker
Differential Revision: D43896148
fbshipit-source-id: c295d3c4dba2368aa66f06df5fc82b473a03fb4d
Summary:
Fixing dependency loop introduced by D37799050 (96abc8160d)
Running `autodeps` yields the following patch:
```
--- a/xplat/quic/state/TARGETS
+++ b/xplat/quic/state/TARGETS
@@ -43,8 +43,8 @@
exported_deps = [
"//folly:random",
"//quic:constants",
+ "//quic/codec:codec",
"//quic/codec:types",
- "//quic/common:circular_deque",
"//quic/common:interval_set",
],
)
```
If this patch is applied, there is a circular dependency loop between `//quic/codec:codec` and `//quic/state:ack_states` by way of `//quic/codec:types`; this loop was introduced by D37799050 (96abc8160d).
Fixed by separating out headers files and targets. In parallel, renamed structures used for writing ACK frames (which were the reason this loop occurred) to make their role clear.
Differential Revision: D42281359
fbshipit-source-id: 8514c99f3fe72ff1d942d7f303e4a209838c7623
Summary:
We don't need to carry these states after the handshake is confirmed, so make them pointers instead. This will facilitate adding a structure to the AckState for tracking duplicate packets.
(Note: this ignores all push blocking failures!)
Reviewed By: hanidamlaj
Differential Revision: D41626895
fbshipit-source-id: d8ac960b3672b9bb9adaaececa53a1203ec801e0
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
