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:
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:
These cases were missed due to the fact that writeFrame was returning something different.
Also, revert the change in the codec for receive timestamps that was using value_or and suppressing something too large being encoded.
Reviewed By: jbeshay, kvtsoy
Differential Revision: D72904909
fbshipit-source-id: 47e415e7b12208c8a2917325ed42f51c63992687
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: These aren't controlled by the peer, they would represent internal programming bugs and qualify as invariants. Trying to handle these as connection-ending rather than process-ending requires excessive plumbing for dubious value.
Reviewed By: jbeshay, kvtsoy
Differential Revision: D71744341
fbshipit-source-id: 6a9768bc2f26c2dcc07b722f43779b712b713951
Summary:
Write the new ACK_EXTENDED frame. When any of the ACK_EXTENDED features are enabled locally and supported by the peer, this frame type will take precedence over ACK_ECN and ACK_RECEIVETIMESTAMPS. See first diff in the stack for the features.
Frame format:
```
ACK_EXTENDED Frame {
Type (i) = 0xB1
// Fields of the existing ACK (type=0x02) frame:
Largest Acknowledged (i),
ACK Delay (i),
ACK Range Count (i),
First ACK Range (i),
ACK Range (..) ...,
Extended Ack Features (i),
// Optional ECN counts (if bit 0 is set in Features)
[ECN Counts (..)],
// Optional Receive Timestamps (if bit 1 is set in Features)
[Receive Timestamps (..)]
}
// Fields from the existing ACK_ECN frame
ECN Counts {
ECT0 Count (i),
ECT1 Count (i),
ECN-CE Count (i),
}
// Fields from the existing ACK_RECEIVE_TIMESTAMPS frame
Receive Timestamps {
Timestamp Range Count (i),
Timestamp Ranges (..) ...,
}
Timestamp Range {
Gap (i),
Timestamp Delta Count (i),
Timestamp Delta (i) ...,
}
```
Reviewed By: sharmafb
Differential Revision: D68931148
fbshipit-source-id: 0fb4bac23e121f82a11602daabc1ec7084db43dd
Summary:
This replaces three separate functions for writing ACK, ACK_ECN, and ACK_RECEIVE_TIMESTAMPS with one function that can write all three.
Previously, we have two paths:
1. Default path which writes the base ACK frame and optionally adds the ECN counts if the frame type is ACK_ECN.
2. If ACK_RECEIVE_TIMESTAMPS are supported, another path would take the ART config and write that frame with the ART fields.
Path #2 does not support ECN marks.
This change consolidates ACK writing into a single path which takes ART config and frame type. It decides which fields to include based upon the type and config passed. This change does not add any new frame types but it prepares for one that can carry both ECN counts and ART fields.
Differential Revision: D68931147
fbshipit-source-id: 47b425b30f00b6c76574bc768d0ec249c60a0aa7
Summary: This is a step in the process of moving away from `folly::IOBuf` in our codebase
Reviewed By: jbeshay
Differential Revision: D68731820
fbshipit-source-id: b8e6f86cceb99d5fa5450f18ad5116d05ae9e13c
Summary: This will make it easier to distinguish between the `finalSize` and the `reliableSize` when we implement reliable resets
Reviewed By: hanidamlaj
Differential Revision: D64836474
fbshipit-source-id: d811d64a2538d4d1acba1ea10a7790d5905f02a4
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
Summary:
The idea here is to make it so we can swap out the type we are using for optionality. In the near term we are going to try swapping towards one that more aggressively tries to save size.
For now there is no functional change and this is just a big aliasing diff.
Reviewed By: sharmafb
Differential Revision: D57633896
fbshipit-source-id: 6eae5953d47395b390016e59cf9d639f3b6c8cfe
Summary:
Read echoed ECN counts from incoming ACK_ECN frames.
Write ECN counts from AckState into outgoing ACK_ECN frames.
This also logs both the ECN counts in the read/writter packets in qlog.
Reviewed By: kvtsoy
Differential Revision: D54967248
fbshipit-source-id: 68b910865515271abfd1fa61fc43ce1cb12f30a7
Summary:
This diff changes `WriteAckFrameState::ReceivedPacket` so that it stores a complete `ReceivedUdpPacket::Timings` object, instead of just one field extracted from that object. This lets us have access to both user space and socket timestamps in our ACK RX timestamp handling code. See D48785086 for a similar change.
--
This diff is part of a larger stack focused on the following:
- **Cleaning up client and server UDP packet receive paths while improving testability.** We currently have multiple receive paths for client and server. Capabilities vary significantly and there are few tests. For instance:
- The server receive path supports socket RX timestamps, abet incorrectly in that it does not store timestamp per packet. In comparison, the client receive path does not currently support socket RX timestamps, although the code in `QuicClientTransport::recvmsg` and `QuicClientTransport::recvmmsg` makes reference to socket RX timestamps, making it confusing to understand the capabilities available when tracing through the code. This complicates the tests in `QuicTypedTransportTests`, as we have to disable test logic that depends on socket RX timestamps for client tests.
- The client currently has three receive paths, and none of them are well tested.
- **Modularize and abstract components in the receive path.** This will make it easier to mock/fake the UDP socket and network layers.
- `QuicClientTransport` and `QuicServerTransport` currently contain UDP socket handling logic that operates over lower layer primitives such `cmsg` and `io_vec` (see `QuicClientTransport::recvmmsg` and `...::recvmsg` as examples).
- Because this UDP socket handling logic is inside of the mvfst transport implementations, it is difficult to test this logic in isolation and mock/fake the underlying socket and network layers. For instance, injecting a user space network emulator that operates at the socket layer would require faking `folly::AsyncUDPSocket`, which is non-trivial given that `AsyncUDPSocket` does not abstract away intricacies arising from the aforementioned lower layer primitives.
- By shifting this logic into an intermediate layer between the transport and the underlying UDP socket, it will be easier to mock out the UDP socket layer when testing functionality at higher layers, and inject fake components when we want to emulate the network between a mvfst client and server. It will also be easier for us to have unit tests focused on testing interactions between the UDP socket implementation and this intermediate layer.
- **Improving receive path timestamping.** We only record a single timestamp per `NetworkData` at the moment, but (1) it is possible for a `NetworkData` to have multiple packets, each with their own timestamps, and (2) we should be able to record both userspace and socket timestamps.
Reviewed By: silver23arrow
Differential Revision: D48795108
fbshipit-source-id: 70471d2654a09cbf25e711af583c18084eb90ca0
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: There's no point in doing this for the DSR backend, as we don't use the frame metadata.
Reviewed By: hanidamlaj
Differential Revision: D46787595
fbshipit-source-id: 027fdc0dfe2e45b8b82d506446c8be9090cef939
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: We shouldn't have config in the codec types. Instead solve the plumbing problem more explicitly, and only define the config in one place.
Reviewed By: jbeshay
Differential Revision: D45881730
fbshipit-source-id: fab6c967a38172f16e57a8978b10460fd196902e
Summary:
Padding frames by definition have no semantics. There also can be quite a large number of them, which makes their storage expensive.
Optimize for this by effectively coalescing multiple contiguous padding frames into a single frame.
(Note: this ignores all push blocking failures!)
Reviewed By: kvtsoy
Differential Revision: D44636497
fbshipit-source-id: 4c1cd88123c4fdf40ec9c553885f46669f7326da
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:
- 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:
- Use varint reordering threshold (this is still being discussed, using varint until a decision is made)
- Track server's minAckDelay on client
- Track ack frequency frame sequence number
- Improve frame parsing error logs
- Add some unit tests
Reviewed By: hanidamlaj
Differential Revision: D38289108
fbshipit-source-id: 274e45115022ffd4e15b60dc57f77f1cce69bd82
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:
- 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:
These are either no longer relevant, are unlikely to be done, or are spculative enough that they don't deserve code space.
Hope here is to make our search for TODOs higher signal.
Reviewed By: lnicco
Differential Revision: D29769792
fbshipit-source-id: 7cfa62cdc15e72d8b7b0cd5dbb5913ea3ca3dc5a
Summary: This diff is the encode and decode support of Datagram frame.
Reviewed By: mjoras, yangchi
Differential Revision: D20983883
fbshipit-source-id: 1a72a87e6ce3601b71fececca872a9d20bf7820e
Summary:
For now the SendInstruction only contains stream information. One
SendInstruction is intended for one QUIC packet. The packet that eventually
will be built by backend also can only have one frame: A Stream Frame. Thus the
data length is skipped.
Reviewed By: mjoras
Differential Revision: D25985035
fbshipit-source-id: 94ef638565899893eff5b429d210234ff3f7f5cd
Summary: As in title. This doesn't actually send any frames, but implements basic support for the transport parameter and responding to the frames.
Reviewed By: yangchi
Differential Revision: D26134787
fbshipit-source-id: 2c48e01084034317c8f36f89c69d172e3cb42278
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:
This introduces a new extension frame, the KnobFrame, and an implementation.
The idea with Knobs is that they are arbitrary key-values within a namespace that can be transmitted to the peer at any time. They provide an alternative to transport settings, by eschewing the standard transport setting space entirely.
The idea is simple, each knob has a "knobspace", "id", and value. The knobspace is a namespace in which the knob has semantic meaning (e.g. 0xfaceb00). The id and value are just that, arbitrary identifiers and values.
On receiving a knob it is the application's reponsibility to deal with it.
Reviewed By: mjoras
Differential Revision: D23601468
fbshipit-source-id: 63e5e4a7bdb76e11e8c952f1234f512a629ef348
Summary:
The problem with Ping being a simple frame:
(1) All SimpleFrames are in the same scheduler. So sending ping means we may
also send other frames which can be problematic if we send in Initial or
Handshake space
(2) Ping isn't retranmisttable. But other Simple frames are. So we are
certainly setting this wrong when we send pure Ping packet today.
That being said, there are cases where we need to treat Ping as retransmittable.
One is when it comes to update ack state: If peer sends us Ping, we may want to
Ack early rather than late. so it makes sense to treat Ping as retransmittable.
Another place is insertion into OutstandingPackets list. When our API user sends
Ping, then also add a Ping timeout. Without adding pure Ping packets into OP list,
we won't be able to track the acks to our Pings.
Reviewed By: mjoras
Differential Revision: D21763935
fbshipit-source-id: a04e97b50cf4dd4e3974320a4d2cc16eda48eef9
Summary:
Becuase when we clone an existing packet, the logic inside the current
writetStreamFrameHeader is no longer correct.
Reviewed By: mjoras
Differential Revision: D21383828
fbshipit-source-id: 8e6bbb048eefd97ca7cf17b89edc2f395f274a73
Summary: We need to internally represent them as uint64_t, in addition to parsing them as var ints.
Reviewed By: yangchi
Differential Revision: D21261466
fbshipit-source-id: 4ff7b9350cbf1dc0b58cb7ce00674430afd3ba53
Summary:
Other frames have been using the BufQueue version of frame writing.
Change this for Crypto streams as well
Reviewed By: mjoras
Differential Revision: D20947921
fbshipit-source-id: 1cfa0f3806e8d74e8c8a4864498e1c06b55d5292
