Summary: Add backpressure based on CCA's writable bytes (cwnd), which can be useful for the app to use as backpressure. This also takes into account stream bytes currently buffered.
Differential Revision: D52098629
fbshipit-source-id: c672e73bab01eb301a67c9b8a95225780000cbd6
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:
This diff changes `QuicAsyncUDPSocketWrapper` so that it is an abstraction layer that inherits from `QuicAsyncUDPSocketType`, instead of simply being a container with aliases.
- Key changes in `QuicAsyncUDPSocketWrapper.h`, the rest of the updates switch us from using `QuicAsyncUDPSocketType` to `QuicAsyncUDPSocketWrapper`.
- It's difficult to mock the UDP socket today given that we expose the entire `folly::AsyncUDPSocket` type to the higher layers of the QUIC stack. This complicates testing and emulation because any mock / fake has to implement low level primitives like `recvmmsg`, and because the `folly::AsyncUDPSocket` interface can change over time.
- Pure virtual functions will be defined in `QuicAsyncUDPSocketWrapper` in a follow up diff to start creating an interface between the higher layers of the mvfst QUIC stack and the UDP socket, and this interface will abstract away lower layer details such as `cmsgs` and `io_vec`, and instead focus on populating higher layer structures such as `NetworkData` and `ReceivedPacket` (D48714615). This will make it easier for us to mock or fake the UDP socket.
This diff relies on changes to `folly::MockAsyncUDPSocket` introduced in D48717389.
--
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: jbeshay, hanidamlaj
Differential Revision: D48717388
fbshipit-source-id: 4f34182a69ab1e619e454da19e357a6a2ee2b9ab
Summary: Update flow control settings names to reflect that these are indeed flow control
Reviewed By: jbeshay
Differential Revision: D48137685
fbshipit-source-id: a48372e21cdd529480e25785a9bd5de456427ef3
Summary:
The pacingTimerTickInterval transport setting conflates two options: the minimum interval a pacer can use, and the resolution of the underlying timer. This means that a higher value leads to lower timer resolution and less pacing accuracy.
This change adds a separate parameter for the timer resolution to ensure that a larger pacing tick does not degrade the pacer accuracy.
Reviewed By: mjoras
Differential Revision: D46564066
fbshipit-source-id: 0d0e54077b80da03e6e6c9baaab49a4c969966b6
Summary:
This has been a default behavior for us for a long time. Basically, we will always include ACK frames in a burst if there's new packets to ACK.
This is overall probably fine and is flexible to peer behavior such that the peer can get away with basically never sending anything ACK-eliciting.
However it is not without its issues. In particular, for DSR it means that we write an excessive amount of pure ACK packets since they cannot be coalesced with a DSR burst.
Make this behavior optional, such that we only include ACK frames in non-probing schedulers when we are only writing stream data.
Reviewed By: kvtsoy
Differential Revision: D39479149
fbshipit-source-id: 6e92d45311a3fb23750c93483b94e97dd3fdce26
Summary:
The write loop functions for DSR or non-DSR are segmented today. As such, so are the schedulers. Mirroring this, we also currently store the DSR and non-DSR streams in separate write queues. This makes it impossible to effectively balance between the two without potential priority inversions or starvation.
Combining them into a single queue eliminates this possibility, but is not entirely straightforward.
The main difficulty comes from the schedulers. The `StreamFrameScheduler` for non-DSR data essentially loops over the control stream queue and the normal write queue looking for the next stream to write to a given packet. When the queues are segmented things are nice and easy. When they are combined, we have to deal with the potential that the non-DSR scheduler will hit a stream with only DSR data. Simply bailing isn't quite correct, since it will just cause an empty write loop. To fix that we need check, after we are finished writing a packet, if the next scheduled stream only has DSR data. If it does, we need to ensure `hasPendingData()` returns false.
The same needs to be done in reverse for the DSR stream scheduler.
The last major compication is that we need another loop which wraps the two individual write loop functions, and calls both functions until the packet limit is exhausted or there's no more data to write. This is to handle the case where there are, for example, two active streams with the same incremental priority, and one is DSR and the other is not. In this case each write loop we want to write `packetLimit` packets, flip flopping between DSR and non DSR packets.
This kind of round robining is pathologically bad for DSR, and a future diff will experiment with changing the round robin behavior such that we write a minimum number of packets per stream before moving on to the next stream.
This change also contains some other refactors, such as eliminating `updateLossStreams` from the stream manager.
(Note: this ignores all push blocking failures!)
Reviewed By: kvtsoy
Differential Revision: D46249067
fbshipit-source-id: 56a37c02fef51908c1336266ed40ac6d99bd14d4
Summary:
## Context
Context: see summary for D43854603.
## In this diff
In this diff, make way for easily adding new elements to Priority struct.
Reviewed By: afrind
Differential Revision: D43882907
fbshipit-source-id: f74f6ec41162a970dcd666bfa5192676f968d740
Summary: Currently, we can enter a deadlock situation where a client rx's a PathChallenge frame, but the packet containing its respective PathResposne frame is lost/delayed. Clients should re-tx the PathResponse frame upon PTO/loss.
Reviewed By: jbeshay
Differential Revision: D43718357
fbshipit-source-id: 26b4bc64dbf48417558eda02744f321f1cb73148
Summary: We always use an SRTT/4 ACKing heuristic for the ACK delay. This has generally been a useful heuristic, but when using ACK_FREQUENCY we should defer to using the delay by the peer.
Reviewed By: jbeshay
Differential Revision: D40272530
fbshipit-source-id: c7a44e53233c97d0c96bc6f936a7318cf4676aba
Summary: We were previously allowing this, but I don't think it's actually a useful semantic. The application should commit to writing the EOF via writeBufferMeta.
Reviewed By: shodoco
Differential Revision: D40245316
fbshipit-source-id: 22492755f45a04223246306b91bfda10be02f8eb
Summary:
The current `close` observer event marks when `closeImpl` is called. However, the actual close of the socket may be delayed for some time while the socket drains. I've changed the existing event to `closeStarted` and added a new event `closing` that marks the close of the underlying `AsyncUDPSocket`.
Adding the `closeStarted` event required two other changes which are difficult to separate from this diff:
- When a transport is destroyed, `QuicTransportBase` was calling `closeImpl` and also closing the UDP socket. However, because the `folly::ObserverContainer` used to store observers is maintained in classes that derive from `QuicTransportBase`, the observers are gone by the time the UDP socket is closed in the base class destructor. Thus, the UDP socket should be closed by the derived classes in their respective destructors. This requirement is inline with the existing code: `closeImpl` is called by all derived classes in their destructors. Made this change and added `DCHECK` statements in the `QuicTransportBase` destructor to ensure that derived classes cleanup after themselves.
- Writing tests with draining enabled and disabled required being able to set the transport settings. However, all of the existing `QuicTypedTransportTest` test cases were designed to operate after the connection was accepted (for the server impls) or established (for client impls), and transport settings cannot be updated at this state. Resolving this required adding new test classes in which the accept/connect operation is delayed until requested by the test.
Reviewed By: mjoras
Differential Revision: D39249604
fbshipit-source-id: 0ebf8b719c4d3b01d4f9509cf2b9a4fc72c2e737
Summary: - adding an API to the QuicSocket to enable users to synchronously query the socket for the maximum amount of data that should be written.
Reviewed By: mjoras
Differential Revision: D37390086
fbshipit-source-id: 5dd64e28b7f841ba3e28a4bc2a79c1b1e5a95047
Summary:
These functions are used to determine when the TX/delivery callbacks are delivered. If we don't consider the writeBufMeta lengths then these callbacks will never be delivered for DSR streams, causing the stream to never destruct.
(Note: this ignores all push blocking failures!)
Reviewed By: kvtsoy
Differential Revision: D36552031
fbshipit-source-id: fe6c0de1afc7c93d158294d965db7701639499fb
Summary: The code has changed since this was made an inlined vector. We now see higher cost from copy operations due to this rather than being able to move the data around.
Reviewed By: bschlinker
Differential Revision: D36151209
fbshipit-source-id: 0b10558c6bd8ebfea9bb960aac36a6c4044fc95f
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: Extend `PacketsWrittenEvent` to include the CWND and number of writable bytes from the congestion controller after the write completed. If no congestion controller is installed, these values are left blank.
Differential Revision: D32588533
fbshipit-source-id: 4b8361937fcab36daa17e5f6dc4386762987d2b4
Summary:
QE result post: https://fb.workplace.com/groups/646964542156536/permalink/1989360141250296/
We are going ahead with shipping this, with a default value of 32 for paddingModulo.
Leaving the transport knob settings still in place in case vips want to selectively disable the feature (not sure if that is the proper way to config that but I think so it is?)
Reviewed By: mjoras
Differential Revision: D34596608
fbshipit-source-id: 5603bb391113c29830f43a67e73c0f50154dcca1
Summary: Report all bytes written in observer packets written events.
Reviewed By: jbeshay
Differential Revision: D33086404
fbshipit-source-id: 1456cf23a420abd025f047f190cb2b8a9868826e
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:
This is a bug that could prevent us from writing data if we ran out of connection flow control while we had lost data.
The last attempt missed a mistake in the scheduling of sequential priority streams.
Reviewed By: kvtsoy
Differential Revision: D33030784
fbshipit-source-id: e1b82234346a604875a9ffe9ab7bc5fb398450ed
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: This is causing empty write loops for some reason.
Reviewed By: jbeshay, lnicco
Differential Revision: D32990291
fbshipit-source-id: 2a183d591de54c7fe0ca54aea828a697a4cd9af0
Summary: This is an edge case we weren't handling properly. This can lead to situations where we run out of conn flow control, the peer hasn't received all data, and then we never write the data to trigger a flow control update.
Reviewed By: afrind
Differential Revision: D32887140
fbshipit-source-id: df5dfad8e0775ef43e5ca6ab98b8ca6b5987ce31
Summary:
Currently the `packetsWritten` observer event is only triggered when ACK-eliciting packets are written to the network. This change makes it so that the event is triggered even when non ACK-eliciting packets are written. In addition, this change increases the information provided in the observer event to reduce the processing that must be performed by observers.
More specifically, this diff makes the following changes:
- It creates two new structs — `WriteEvent` and `PacketsWrittenEvent` — and makes `PacketsWrittenEvent` and `AppLimitedEvent` inherit from `WriteEvent`, which contains the base set of fields that appear in both derived events. The base set of fields includes `writeCount` and the reference to the list of `OutstandingPackets`.
- A `PacketsWrittenEvent` is generated after each write loop during which packets are written.
- The `PacketsWrittenEvent` records the total number of packets written (the sum of ACK-eliciting and non ACK-eliciting packets written) and the total number of ACK-eliciting packets written during the last loop. By exposing this information, observers can determine whether there is value in inspecting the list of `OutstandingPackets` before doing so.
- In the future, I'll extend this event to also report the number of stream bytes written.
- It adopts a new builder pattern for observer events to make it easier to create instances of these structures:
- We use this approach instead of aggregate initialization (specifically designated initializers via aggregate initialization), because designated initializers (C++20 feature, although supported by some compilers long before) is not yet supported on all platforms for which we need this code to compile.
- I intend to change the other observer events to use this pattern as the number of arguments in some of their constructors makes them increasingly difficult to deal with.
Reviewed By: mjoras
Differential Revision: D31216724
fbshipit-source-id: 42ceb922d46ab0e0dc356f8726c2d0467a219587
Summary: Provide observers with visibility into stream events triggered by local and peer (e.g., new stream opened locally or by peer).
Reviewed By: mjoras
Differential Revision: D31886978
fbshipit-source-id: 7556fef0f336bd0f190b4474f1a7b0120aae6ef1
Summary: As titled. We currently allow `QuicSocket::Observer::close` to be called multiple times. Only call it once.
Reviewed By: lnicco
Differential Revision: D31886995
fbshipit-source-id: 61d959e8575b08c34ef896b87da0cf8ada482144
Summary: Do not call onAppRateLimited() before connection is ready
Reviewed By: bschlinker
Differential Revision: D30563194
fbshipit-source-id: dd8c2da656041f9219a645935b6c4c3c85b27816
Summary: This solves test flake that resulted from using EXPECT_NEAR with varying timing on devservers and CI situations
Reviewed By: yangchi
Differential Revision: D29574796
fbshipit-source-id: 804935e8e0148fcccec642a1894e8863bee4465b
Summary:
Add a new QuicSocket API that sets the maximum pacing rate in Bytes per second to be used if pacing is enabled. This setting is passed down to the Pacer which enforces it.
The change also includes:
- Refactoring the setPacingRate function of the Pacer to make it more consistent with the other refreshPacingRate function.
- A new flag for testing the MAX_PACING_RATE using tperf.
Reviewed By: mjoras
Differential Revision: D29276789
fbshipit-source-id: 818d86707084b2697f7417b4a47e62cbbce65c73
Summary: Remove a check that only allowed getting and setting the receive window on writable streams. This had prevented a receiver from getting/setting the receive window of a unidirectional stream initiated by the peer.
Reviewed By: mjoras
Differential Revision: D29366097
fbshipit-source-id: 3bc5cba826663b3499c19fe26bdd9a070682e533
Summary:
Another bug around offset when DSR is used. When frontend sends reset,
it should use writeBufMeta.offset for reset offset if some bytes have been sent
via the DSR path.
Reviewed By: mjoras
Differential Revision: D28791467
fbshipit-source-id: d563964504a5d55ba584f5c90a0883c6900084b4
Summary:
Previously we were using a single flag appDataSentEvents to enable 3 callbacks
(startWritingFromAppLimited, packetsWritten and appRateLimited). This commit
creates separate flags to enable each of these callbacks, we have use-cases
where Observers might want only packetsWritten events and not the others.
Also, this commit refactors the logic to deliver these 3 callbacks to all
observers into a separate method so they can be unit tested easily.
Reviewed By: bschlinker
Differential Revision: D27925011
fbshipit-source-id: 7f7436dfc3d50c3abcb8ec121b221d20e30b0c4b
Summary:
The CHECK is too harsh today. It blocks any write if BufferMeta has
been written to a Quic stream. Writing a pure EOM should be allowed. If caller
wants to write just an EOM, it should be free to choose either writeChain or
writeBufMeta API.
(Note: this ignores all push blocking failures!)
Reviewed By: mjoras
Differential Revision: D27684033
fbshipit-source-id: 475c65c7eac110ca6eb2a8c2abaf1eb00628a492
Summary:
This assigns a DSRPacketizationRequestSender to a QUIC stream, and let
it own it.
Reviewed By: mjoras
Differential Revision: D27668523
fbshipit-source-id: 4beba6ddf247801368c3e1c24a0a4956490d45cd
Summary:
As before we will now aggressively send probes on all spaces with probes available when the PTO timer fires.
This time with more unit tests and some bug fixes.
Reviewed By: yangchi
Differential Revision: D27338523
fbshipit-source-id: 8a9ccb90ed691e996fab4afa2f132c0f99044fbc
Summary:
This commit demonstrates the use of ByteEvents in maps. Intentionally didn't define the custom comparator and hash function inside the quic library because we want consumers of ByteEvents to define equality of 2 ByteEvents in their own way.
This will help recipients of ByteEventCallback keep track of pending,
received and cancelled ByteEvents easily.
In addition, the behavior of registerByteEventCallback was modified to NOT allow duplicate registrations. A pair of registrations is considered duplicate if they both have the same stream ID, stream offset, ByteEvent type and recipient callback pointer. In such cases, registerByteEventCallback returns an INVALID_OPERATION error.
This is critical to make sure that ByteEvents work well in maps, which may be used by recipients.
Reviewed By: bschlinker
Differential Revision: D27100623
fbshipit-source-id: 084a4765fa6c98fdc1b98414fbd30582cf1e5139
Summary: As in title. There's a bug here somewhere with empty write loops we need to find.
Reviewed By: yangchi
Differential Revision: D27279100
fbshipit-source-id: e1d26fbf8d6df1590d464a6504a8b940b46794e0
