Summary:
This is a major change that adds support for handling path probes in the QuicServerTransport. To that end, this change:
- Incorporates the new QuicPathManager into the read and write paths of the QuicTransportBaseLite.
- Separates the connection migration logic from path probing to enable handling probe-only packets that could fail or succeed without impacting the active path.
- Tracks the path information in the outstanding packets and when updating the connection state for probing packets.
- Makes minor changes for the client transport to be able to accommodate the previous changes.
This change does NOT include:
- Client logic to initiate or handle path probes
- Server logic to rotate CIDs on migration.
Reviewed By: mjoras
Differential Revision: D79122987
fbshipit-source-id: 8db01b007b7312f51d7431cb10abb35b43d42794
Summary: In this commit, I'm moving the incrementing of `inflightBytes` out of the cc. In subsequent commits, I'll move the decrementing out as well.
Reviewed By: jbeshay
Differential Revision: D80984885
fbshipit-source-id: 9fca504060d800fcfdf2b5074a7cbac314eb89fe
Summary:
This commit converts IntervalSet to use CHECKs instead of throwing exceptions and provides safe tryInsert methods that return quic::Expected for error handling.
**Core Problem Solved:**
IntervalSet was throwing `std::invalid_argument` exceptions in two scenarios:
1. When constructing an Interval with `start > end`
2. When interval bounds exceed the maximum allowed value
This change eliminates exceptions in favor of CHECKs (for internal validation) and Expected-based error handling (for caller validation).
**Implementation Details:**
**1. IntervalSet Core Changes:**
- Replaced `throw std::invalid_argument` with `CHECK_LE` in Interval constructor
- Replaced `throw std::invalid_argument` with `CHECK_LE` in `insert(start, end)`
- Added `IntervalSetError` enum for error classification
- Added `folly::Expected` include
**2. Safe API Layer:**
- Added `tryInsert(interval)` method returning `Expected<Unit, IntervalSetError>`
- Added `tryInsert(start, end)` method with pre-validation
- Added `tryInsert(point)` method
- Added static `Interval::tryCreate()` method for safe interval construction
**3. Updated Code:**
- **QuicWriteCodec.cpp**: Updated `fillFrameWithPacketReceiveTimestamps` to use `tryInsert`
- Returns `QuicError` if interval validation fails
- Maintains existing error handling patterns
- **QuicTransportFunctions.cpp**: Updated `implicitAckCryptoStream` to use `tryInsert`
- Logs errors and continues processing other packets
- Robust error handling for crypto stream implicit acks
Reviewed By: kvtsoy
Differential Revision: D76792362
fbshipit-source-id: 5bd7c22e69a91d60cc41c603a1f2380893f4c8a0
Summary:
Meta:
Special handling for MVFST_PRIMING connections on QUIC Server
* Do not write any packets back to the client
* Close connection early: as soon as data for the client is ready on a bidirectional stream -- using that as a proxy for "HTTP request stream"
* Do not bump the "connection closed with zero bytes written" counter to not add noise to current monitoring
Reviewed By: kvtsoy
Differential Revision: D79491915
fbshipit-source-id: c914968f37816d725790a12f3c1c136777c94d00
Summary:
* After a write, the stream buffers may be empty (the write completes sending the stream) or nearly empty. We call this situation (before the write) "imminent stream completion".
* If it's the nearly-empty case, another write will then have to be scheduled to complete sending the remaining stream data. Instead of waiting for this next round, we can just send everything in the current write. The advantage is that, absent buffer bloat, the stream will reach the client sooner. This does violate the burst size from the pacer. However, after the streams complete, the connection will be app-limited and the pacer state will reset. Thus this violation won't have persistent effects.
* This diff creates a knob to specify a "minimum stream buffer threshold". If the writer determines that the stream buffers will be left below this threshold after a write (imminent stream completion), it writes everything. (Note: It will not allow thresholds greater than the minimum burst size to have an effect).
* In these imminent stream completion situations, we can also hasten the stream completion by allowing for some elasticity in the CC's cwnd. Thus another knob is added to allow the writer to use excess cwnd during these situations. The knob controls the allowable excess as a percentage of the congestion controller's writable bytes, which is cwnd - inflight. Thus if the knob is set to X, the writable bytes will be (cwnd - inflight) * (1 +X/100).
* Add limit for pacer_min_burst_packets.
* Rename TransportKnobParamId::MAX_BATCH_PACKETS to MAX_WRITE_CONN_DATA_PKT_LIM since it now controls only the latter.
Reviewed By: jbeshay
Differential Revision: D77837617
fbshipit-source-id: 24afe90b4daaa74d08a8ea84c35110ee284f189b
Summary:
Meta:
The priming data vector vector ownerhip was not moved, that made us save duplicate packets.
For example: the onPrimingDataAvailable callbacks is currently invoked twice
* 1 - after "writing" the initial
* 2 - after "writing" the first flight of 0-RTT packets
If primingData is not moved correctly the callbacks would look like this:
1. onPrimingDataAvailable([initial])
2. onPrimingDataAvailable([initial, 0RTT packets])
so we would end up with a duplicate initial. This is why the tests were originally written with relaxed checks "CHECK_GE(packets.size(), 2)" instead of "CHECK_EQ(packets.size(),2)"
Reviewed By: mjoras
Differential Revision: D78054738
fbshipit-source-id: 5779d6b0513473c9d37416ca5686726144bd3a37
Summary: Continuing the theme, this time with inplaceEncrypt. We have to catch the exceptions from Fizz for the bridge version.
Reviewed By: kvtsoy
Differential Revision: D75886829
fbshipit-source-id: f74d5ce73e242e20e4c62552748970b2d5d7cb79
Summary: Some old MVFST clients did not support key updates. This change prevents the server from initiating key updates to clients using the MVFST quic version until the client has initiated a key update. This ensures that key updates are triggered for clients that support them.
Reviewed By: mjoras
Differential Revision: D75022528
fbshipit-source-id: 7af2d65becbfefee347cbe98ccdfc01323cd153c
Summary:
This is an API break, but it should mostly be a manageable one. We want to be able to compile mvfst internally without exceptions, and folly::Optional is one dependency that makes this challenging. Additionally, we already have an imported secondary optional type for performance/struct size reasons, tiny-optional.
This second optional interface is mostly compatible in an API sense (including the use of std::nullopt) with std::optional. Thus our approach is to remove the dependency on folly::Optional, and offer a quic::Optional instead.
The next diff will properly vendor tiny-optional so that quic::Optional is an independent version of it.
Reviewed By: sharmafb, kvtsoy
Differential Revision: D74133131
fbshipit-source-id: 715f8bb5043ba3bb876cacfe54236887e0686b30
Summary:
When the transport is in priming mode it saves all packets instead of writing them on the wire, and feeds them to a callback for the caller to get the data.
Meta:
Priming mode is used to get all packets before 1-RTT cipher is available, in order for them to get replayed later.
Reviewed By: kvtsoy
Differential Revision: D71290230
fbshipit-source-id: 230650cb1e5901069dda4ef850c9c724bf33b6be
Summary:
- Fail the connection if the peer acks future packets.
- Skip a packet number randomly and fail if the peer acks it
- Account for the skipped packet in the reorder distance when detecting losses
- Account for the skipped packet when implicitly acking crypto streams
Reviewed By: mjoras, kvtsoy
Differential Revision: D73120636
fbshipit-source-id: 661e4b418f9f015d6de627b67e1cf4d1fd81bb27
Summary:
Previously,
* `RawBuf` was a typealias for `std::unique_ptr<folly::IOBuf>`
* `Buf` was a typealias for `folly::IOBuf`
In this diff,
* `Buf` is a typealias for `folly::IOBuf`
* `BufPtr` is a typealias for `std::unique_ptr<folly::IOBuf>`
Reviewed By: hanidamlaj
Differential Revision: D73206576
fbshipit-source-id: 454bf6ccfce3d6571e5e931889263ed98cc24af3
Summary: Continuing the theme. This removes it from client and server handshakes.
Reviewed By: kvtsoy
Differential Revision: D73335422
fbshipit-source-id: 262bad17c1ebd2bcef623b1185e38e6a63ec714b
Summary: Previously, we didn't support all combinations of (batching mode, data path type).
Reviewed By: mjoras
Differential Revision: D73280506
fbshipit-source-id: 752a1eda4f58a19f86410bff514415017ffdb383
Summary: More in the theme of returning Expected instead of throwing. For the folly case, we keep the try/catches in there and translate to Expected. For Libev, we convert directly to Expected.
Reviewed By: kvtsoy
Differential Revision: D73217128
fbshipit-source-id: d00a978f24e3b29a77a8ac99a19765ae49f64df8
Summary: This introduces a more generic typealias so that we can, for instance, write `BufHelpers::createCombined` instead of `folly::IOBuf::createCombined`.
Reviewed By: jbeshay
Differential Revision: D73127508
fbshipit-source-id: d585790904efc8e9f92d79cbf766bafe0e84a69f
Summary:
This adds the new priority queue implementation and a TransportSetting that controls whether it should be used or not. The default is still the old priority queue, so this diff should not introduce any functional changes in production code.
One key difference is that with the new queue, streams with new data that become connection flow control blocked are *removed* from the queue, and added back once more flow control comes. I think this will make the scheduler slightly more efficient at writing low-priority loss streams when there's high-pri data and no connection flow control, since it doesn't need to skip over those streams when building the packet.
If this diff regresses build size, D72476484 should get it back.
Reviewed By: mjoras
Differential Revision: D72476486
fbshipit-source-id: 9665cf3f66dcdbfd57d2199d5c832529a68cfac0
Summary: Continuing the theme, removing it from QuicInteger which ends up being in a lot of the write paths.
Reviewed By: kvtsoy
Differential Revision: D72757026
fbshipit-source-id: 99a6ab2caea8fb495b1cb466172611002968e527
Summary: As in title, this is more of a theme on adding an Expected return.
Reviewed By: kvtsoy
Differential Revision: D72579218
fbshipit-source-id: 25735535368838f1a4315667cd7e9e9b5df1c485
Summary: I started with the QuicStreamManager, but it turns out that the path from the manager up to the close path touches a LOT, and so this is a big diff. The strategy is basically the same everywhere, add a folly::Expected and check it on every function and enforce that with [[nodiscard]]
Reviewed By: kvtsoy
Differential Revision: D72347215
fbshipit-source-id: 452868b541754d2ecab646d6c3cbd6aacf317d7f
Summary: Cache the negotiated config for what ACK type to write and which fields to use once the peer transport parameters are available. This avoids computing the config with every ack frame being written.
Reviewed By: sharmafb
Differential Revision: D70004436
fbshipit-source-id: 79354f5137c77353c3a97d4c41782a700622e986
Summary: This was added to not bundle ACKs with stream frames. Don't special case those and also disabling opportunistic ACKing for all write reasons.
Reviewed By: jbeshay
Differential Revision: D66514392
fbshipit-source-id: f2657a5c06ea8ae37b8c8eacd04c5a3b8ac75390
Summary: In `writeCryptoAndAckDataToSocket`, add an additional `writeProbingDataToSocket` call at the end that is limited to the number of CRYPTO frame-containing packets just written, gated by the new `TransportSetttings` field `immediatelyRetransmitInitialPackets`.
Reviewed By: mjoras
Differential Revision: D64485616
fbshipit-source-id: f0927a3796767700fd46673195e1cd4e1bbbcbeb
Summary: This kinda makes sense to just use as a default.
Reviewed By: kvtsoy, sharmafb
Differential Revision: D64066392
fbshipit-source-id: 0915f163c0483af6bec014bde61e82b6ee2ac6cb
Summary: As in title. Without this the keepalive option is less effective since a single PING packet loss can cause issues.
Reviewed By: kvtsoy
Differential Revision: D63397494
fbshipit-source-id: 7ef6b6f54189609e3a96409ac9c035c475dc0569
Summary: This is my second attempt at D61871891. This time, I ran `xplat/cross_plat_devx/somerge_maps/compute_merge_maps.py`, which generated quic/somerge_defs.bzl
Reviewed By: kvtsoy
Differential Revision: D61975459
fbshipit-source-id: bec62acb2b400f4a102574e8c882927f41b9330e
Summary: `PacketEvent` is a very inaccurate and misleading name. We're basically using this as an identifier for cloned packets, so `ClonedPacketIdentifier` is a much better.
Reviewed By: kvtsoy
Differential Revision: D61871891
fbshipit-source-id: f9c626d900c8b7ab7e231c9bad4c1629384ebb77
Summary:
**Context**
The `BufAccessor` is used to access a contiguous section of memory. Right now, it works with a `Buf` under the hood.
**Overall plan**
The plan is to change the `BufAccessor` to use a `uint8_t*` instead. Since we're using section of contiguous memory, there's no need to use a chained buffer abstraction here. This'll move us closer to deprecating the usage `folly::IOBuf`.
**What this diff is doing**
Most use cases of the `BufAccessor` look like the following:
```
auto buf = bufAccessor.obtain();
// Do something with buf, like calling trimEnd
bufAccessor.release(buf)
```
I'm adding APIs to the `BufAccessor` so that there's no need to `obtain()` and `release()` the `Buf`. We'd instead just call an API on the `BufAccessor`, which would call that same API on the underlying `folly::IOBuf`. Later on, we'll change the `BufAccessor` to use a `uint8_t*` under the hood.
I'm currently leaving in the `obtain()`, `release()`, and `buf()` APIs because Fizz and the AsyncUDPSocket expect `folly::IOBuf` as inputs in many of their APIs. Once those callsites are migrated off `folly::IOBuf`, we can remove these APIs.
Reviewed By: mjoras
Differential Revision: D60973166
fbshipit-source-id: 52aa3541d0c4878c7ee8525d70ac280508b61e24
Summary:
Timely reaction to congestion requires relaying any CE marks to the sender as soon as possible.
This change schedules an ack to be sent whenever incoming packets are received with CE marks. This will only happen when the readEcnOnIngress option is enabled.
Reviewed By: mjoras
Differential Revision: D58423959
fbshipit-source-id: 30f8cf8b11d0446985c2d87d7df67c24c0d5afdf
Summary: Useful to track so we can optimize it.
Reviewed By: kvtsoy
Differential Revision: D58196435
fbshipit-source-id: c15c409f998430fd0c6acde0539d0345123a9e15
Summary: We have a lot of optionals that are either integral values or std::chrono::microseconds. These end up wasting memory, where we can instead store sentinel values to encode whether the value is there or not. This reduces the effective range of the type by one value, but that is an acceptable tradeoff.
Reviewed By: kvtsoy
Differential Revision: D57684368
fbshipit-source-id: b406b86011f9b8169b6e5e925265f4829928cc63
