Summary: Continuing the theme, removing them from the CryptoFactory and translating to Expected.
Reviewed By: kvtsoy, jbeshay
Differential Revision: D74676120
fbshipit-source-id: 715b497e68a4e3004811038cba479c443d5398fd
Summary: This primarily involved making the constructors private and changing the callers of the factory functions. The crashing factory is only expected to be used by tests.
Reviewed By: kvtsoy
Differential Revision: D74347638
fbshipit-source-id: 4c0dd7fabaa233c8a3460c359462a22642d26f5b
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:
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: 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: 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:
The purpose of this is so that we can import this header from the WebTransport implementation and use the `ByteEventCallback` directly instead of creating a wrapper, thereby saving an allocation.
There's no functional change in this commit, it's just moving things around.
Relevant files:
* quic/api/QuicCallbacks.h
* quic/api/QuicSocketLite.h
Reviewed By: hanidamlaj
Differential Revision: D70000563
fbshipit-source-id: 9523cc788f50b4ba218be33e84f7d5b4f44a73c2
Summary:
All according to plan: https://fburl.com/gdoc/pebccgi1
Changing function definitions to return errors while still throwing.
Reviewed By: sharmafb
Differential Revision: D69567329
fbshipit-source-id: 5d40ee32fe185d5674785632a9a13e4cef996988
Summary: These aren't used anywhere and we're going to start refactoring the priority implementation.
Reviewed By: jbeshay, hanidamlaj
Differential Revision: D68657305
fbshipit-source-id: e4a3a5a991bac99afef1362adb2663a50766d2c7
Summary:
All the functionality is a duplicated from `updateWritableStreams`.
Btw the only callsites were inside UT.
Reviewed By: kvtsoy
Differential Revision: D68351972
fbshipit-source-id: 7e17f38ffcffecea23a64f5d2d5c1dec7a8c43f5
Summary: There's no need to loop through all the streams if we're just resetting one.
Reviewed By: hanidamlaj
Differential Revision: D67304631
fbshipit-source-id: 4817459ca7d1c1fd906b7640a0089c1c52e6e485
Summary:
With normal resets, we transition from `StreamSendState::ResetSent` to `StreamSendState::Closed` when we get an ACK for a RESET_STREAM frame.
With reliable resets, this is going to be a little more complicated. We can't automatically transition from `StreamSendState::ResetSent` to `StreamSendState::Closed` when we get an ACK for a RESET_STREAM_AT frame because it's possible that the peer hasn't yet received all data until the reliable reset offset.
My idea is the following: Keep track of the `minReliableSizeAcked`, which is the lowest value of the reliable size in any RESET_STREAM_AT frame that was ACKed by the peer. We set it to 0 if a RESET_STREAM frame was ACKed by the peer.
Then, we can transition from `StreamSendState::ResetSent` to `StreamSendState::Closed` if any one of the following two events happen:
* We get an ACK for a RESET_STREAM_AT or a RESET_STREAM frame, and all data until the `minReliableSizeAcked` has been ACKed by the peer.
* We get an ACK for stream data, and this puts us in a state where all data until the `minReliableSizeAcked` has been ACKed by the peer.
Note: This diff doesn't have any functional change. The only change is that we're keeping track of the `minReliableSizeAcked`, but aren't using it anywhere.
Reviewed By: mjoras
Differential Revision: D66781199
fbshipit-source-id: 2aa5138a18f70e9801e59e747460558ba706939c
Summary:
setReadCallback didn't function properly during shutdown
1) it was completely ignored when state_ == CLOSING
2) cancelAllAppCallbacks made a copy of readCallbacks_
This is problematic for application constructs that use groups of streams -- eg: HTTP WebTransport. When one stream (the WebTransport session) is reset during shutdown, it needs to clean up any dependent streams as well, including preventing them from getting error callbacks.
The fix is to use only the streamId's from readCallbacksCopy for iteration, but look up the actual callback value from readCallbacks_.
Note: there's an as yet unhandled corner case which is that a readError callback installs a new stream read callback, but it seems far fetched enough I'm not including a fix here.
Reviewed By: sharmafb
Differential Revision: D48159644
fbshipit-source-id: c9d522a6b200538193969d60d242a505831e4cd0
Summary:
Concatenate adjacent namespaces + format
This is a codemod. It was automatically generated and will be landed once it is approved and tests are passing in sandcastle.
You have been added as a reviewer by Sentinel or Butterfly.
Autodiff project: nc
Autodiff partition: fbcode.quic
Autodiff bookmark: ad.nc.fbcode.quic
Reviewed By: hanidamlaj
Differential Revision: D65365244
fbshipit-source-id: 0bbaa7684d03caf8fc8eff3439a0865940398220
Summary: If the peer sends higher stream limits in the settings, these callbacks may be invoked before the app has a chance to set the connection callback.
Reviewed By: sharmafb
Differential Revision: D64214648
fbshipit-source-id: 6a8a9b8d4d9e02a2baad672d69a43ba61daba918
Summary:
fixes the arg passed to the ::onBidirectionalStreamsAvailable callback
#facebook:
i accidentally introduced a bad operator precedence bug in a refactor; this fixes that and the unit test
luckily HQSession doesn't consume the argument passed to the callback
Reviewed By: knekritz
Differential Revision: D62765762
fbshipit-source-id: 7ae206d5036fc744485736f92fd366ce99103d58
Summary: Constructing an `fbstring` from an `IOBuf` and then converting that to `std::string` is inefficient: if the `IOBuf` is chained, that can end up copying the data twice. `to<std::string>()` does exactly one copy.
Reviewed By: mjoras
Differential Revision: D58379802
fbshipit-source-id: a1094dd63ae00fc123e0d695a800a017004562e9
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: We can throw in `writeSocketDataAndCatch()` and that would result in transport closure with drain (with drain keeping the socket alive), but the congestion controller would be reset in close: https://fburl.com/code/zxwfye5u and `maybeStopWriteLooperAndArmSocketWritableEvent()` trips over it later when executed in scope exit.
Reviewed By: mjoras
Differential Revision: D57728369
fbshipit-source-id: 51a4719ae97fab0e90e3ae093a3f56be5a096aff
Summary:
The existing batch writers do not handle failed writes to the AsyncUDPSocket. A packet that fails to be written is detected as a packet loss later when feedback is received from the peer. This negatively impacts the congestion controller because of the fake loss signal, and artificially inflates the number of retransmitted packets/bytes.
This change adds a new batch writer (SinglePacketBackpressuretBatchWriter) that retains the buffers when a write fails. For subsequent writes, the writer retries the same buffer. No new packets are scheduled until the retried buffer succeeds.
Notes:
- To make sure that retry writes are scheduled, the write callback is installed on the socket when a buffer needs to be retried.
- The retries are for an already scheduled packet. The connection state reflects the timing of the first attempt. This could still have an impact on rtt samples, etc. but it this is a milder impact compared to fake losses/retranmissions.
- Any changes outside of the batch writer only impact the new batch writer. Existing batch writers do not use the fields and are not affected by the changes in this diff.
Reviewed By: kvtsoy
Differential Revision: D57597576
fbshipit-source-id: 9476d71ce52e383c5946466f64bb5eecd4f5d549
Summary: Use writable events on the socket (disabled by default)
Reviewed By: jbeshay
Differential Revision: D56305786
fbshipit-source-id: f04dea326587268c96915f7a39338ff21dee4aec
Summary: Adds new field `tosValue` to ReceivedUdpPacket so it is accessible in the rest of the read path.
Reviewed By: kvtsoy
Differential Revision: D54912161
fbshipit-source-id: ea4714fa2374d38e915fc850387e1094d1fb8adf
Summary: ReceivedUdpPacket has attached metadata (currently timings and later in the stack, tos values). Rather than passing the metadata separately in the read path for the client and the server, this propagates the ReceivedUdpPacket further up so this metadata is easier to use in the rest of the stack.
Reviewed By: mjoras
Differential Revision: D54912162
fbshipit-source-id: c980d5b276704f5bba780ac16a380bbb4e5bedad
Summary: I want to have visibility in to the current count of streams. There are already callback methods onNew/onClosed, unfortunately, the latter is not called when connections are closed, so I had to fix that to prevent the metric from uncontrolled growth. I am still not positive it's correct and was thinking of hooking into QuicStreamState destructor instead.
Reviewed By: kvtsoy
Differential Revision: D56195487
fbshipit-source-id: 901bce9473327a8f7ef1bc0f70af10899784d681
Summary: I'm putting this up to get some initial feedback. I'm writing a function to get the peer transport parameters so that we can get them in the HQSession and set the QUIC fingerprint.
Reviewed By: hanidamlaj
Differential Revision: D54399572
fbshipit-source-id: 594294c26a5b34bc99a8d286a66be9cdbe7fff02
Summary:
This moves all the loop and timer callback operations back to the callbacks instead of relying the QuicEventBase to perform them. I.e. it's now Callback::cancelCallback() instead of QuicEventBase::cancelCallback(&callback).
To simplify the design, the lifetime of LoopCallbackWrapper and TimerCallbackWrapper has been changed. Previously these wrappers lasted for one iteration of the loop or the timer callback. In the new design the wrapper is created the first time the callback is scheduled and is only destroyed when the callback is destroyed. This significantly simplifies the lifetime management of the wrapper and the code overall.
While transitioning LibevQuicEventBase to this new design, this change also consolidates the QuicTimer functionality in the LibevQuicEventBase itself.
Reviewed By: hanidamlaj, mjoras
Differential Revision: D52217879
fbshipit-source-id: da7b95d592650ddc112813fdc3b9f5010d32e7fb
Summary:
This is the major transition that updates mvfst code to use the new interfaces. The new Folly implementations of the interfaces maintain all the existing behavior of folly types so this should not introduce any functional change. The core changes are:
- Update the BatchWriters to use the new interfaces.
- Update the FunctionLooper to use the new interfaces.
- Change QuicServerTransport to take the folly types and wrap them in the new types for use in the QuicTransportBase.
The rest of the diff is for updating all the existing uses of the QuicTrasnport to initialize the necessary types and pass them to the QUIC transport instead of directly passing folly types.
Reviewed By: mjoras
Differential Revision: D51413481
fbshipit-source-id: 5ed607e12b9a52b96148ad9b4f8f43899655d936
Summary: Move the callback to before the stream is destroyed.
Reviewed By: jbeshay
Differential Revision: D52127833
fbshipit-source-id: 1a9dbcf9ad92dbdb4e15409a418307dc6852b091
Summary: This is potentially useful to an application to know when the underlying stream state has been freed by the transport.
Reviewed By: jbeshay
Differential Revision: D52048888
fbshipit-source-id: e7b2d33c3702ce8aa348459a37094198d16af60f
Summary:
This diff renames `ReceivedPacket` to `ReceivedUdpPacket` to clarify that it maps to a UDP packet and not a QUIC packet. A single UDP packet can contain multiple QUIC packets due to coalescing.
--
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: D48788809
fbshipit-source-id: 3793c30212d545e226f3e5337289bc2601dfa553
