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:
Comparator in test byte event callback class wasn't accounting for the
type field.
Reviewed By: mjoras, lnicco
Differential Revision: D32226068
fbshipit-source-id: 62c67b08f91ff3eee4096fa9dc777fc7000a5854
Summary:
The StreamManager monitors can now have a reference to a new observer interface QuicStreamPrioritiesObserver. If the reference is set, the observer is notified whenever streams are created, removed, or change priorities
QuicTransportBase implements this observer interface. It uses the event notifications to control a connection's background mode based upon the new background mode parameters: priority level threshold, and target utilization factor. When these parameters are set, and all active streams have a priority lower than the threshold, the connection's congestion controller is set to use only the defined utilization factor of the available bandwidth.
Reviewed By: bschlinker
Differential Revision: D31562505
fbshipit-source-id: 9c74fa834301f745d97e851741b911795f756023
Summary:
For each `QuicServerTransport` created by `QuicServerWorker`, we are able to set the `cmsg`s to be sent whenever there's a write.
With this we can set things like TOS/SO_MARK per connection
Reviewed By: bschlinker
Differential Revision: D29313608
fbshipit-source-id: d60c97f65681086ae1079b6b95beade95158ec59
Summary: This probably never made much sense as a policy. The application can reasonably expect to be able to set a read callback after the stream first becomes readable.
Reviewed By: afrind
Differential Revision: D29563483
fbshipit-source-id: 215a6aed130108248a0b4cbc36c02c8da70bf721
Summary: When multiple observers attached and one is removed, removal code can end up calling `observerDetach` on the wrong observer. Simplified and fixed removal logic and added new tests.
Reviewed By: yangchi
Differential Revision: D27033221
fbshipit-source-id: d200fd2243a678890758b2652b61d16887f073dd
Summary: This was missed when peekError was added
Reviewed By: mjoras, lnicco
Differential Revision: D27973063
fbshipit-source-id: dbf1112a4ce6822401ba5125de853fe922acd85e
Summary:
As per design, we are not supposed to honor duplicate byte event registrations. Duplicate is defined by a registration for the same <stream ID, stream offset, callback*> tuple twice.
Currently, there is a bug where-in we can register the same byte event a second time *if* there is another byte event for a higher offset that is registered in-between, for the same stream.
Example Sequence that triggers the bug:
1. Registration for Stream ID =10, offset = 50, callback = A. This will be successful.
2. Registration for Stream ID = 10, offset = 55, callback = <doesn't matter>. This will be successful.
3. Registration for Stream ID = 10, offset = 50, callback = A. This is a duplicate of #1. This *should not* be honored, but it was being honored (this was the bug).
This commit fixes the bug, #3 will return INVALID_OPERATION.
Reviewed By: bschlinker, lnicco
Differential Revision: D27946237
fbshipit-source-id: f9751ac90159284f86a285e5e0ebb23b83041d2a
Summary:
Diff D27846813 (98b5e2aa8f) introduced a fix for a corner case in the ByteEventRegistration logic where-in it was possible to register a byte event for the same stream ID, offset and ByteEvent type as long as they were not consecutive registrations. If there was a different byte event registration for the same stream ID in-between, then duplicates were being allowed.
However, that fix causes an unexpected crash in HQSession::dropConnectionSync().
This commit reverts the fix for duplicate byte event registrations until we are able to find the root cause for the crash and come up with a better fix. Note that we are still able to catch duplicate registrations as long as they are consecutive registrations for the same <stream ID, offset, callback>. With this fix reverted, we will not be able to catch non-consecutive duplicate registrations.
Reviewed By: bschlinker
Differential Revision: D27903179
fbshipit-source-id: b4da3128d5ba14fc764e1768fd27982553968997
Summary:
When registerByteEvent is called for a particular stream ID and offset, it is
possible that the byte event has already occurred in the past. In this case, we
schedule a lambda to be executed in the *next* iteration of the event loop, to
deliver the byte event to the callback.
There are 2 bugs in the current implementation of runEvbAsync which executes
the scheduled lambda.
1. It is possible that multiple recipients register for the same byte event on
the same stream ID and offset. If one of the recipients is already delivered
the byte event, then there is a possibility that the lambda can redeliver the
same notification a second time, which is not desirable.
2. There is a race condition where-in between the time the lambda was scheduled to
the time the lambda was excecuted, it is possible that the byte event gets
delivered when we process callbacks on receiving network data. In this
situation, there is a bug in the lambda where-in if there is another byte event
registered for any other offset for the same stream ID (regardless of the
callback), we will falsely deliver the byte event that was requested, resulting
in double delivery of the same byte event.
This commit fixes both bugs by checking that the requested byte event still
exists in the ByteEventDetail deque (comparing the offset as well as callback),
before erasing and delivering it.
Reviewed By: bschlinker
Differential Revision: D27846813
fbshipit-source-id: 099b79af2580fba5561ec33c4be45a8cf84c39e8
Summary:
This diff adds error callback API during the time PeekCallback() is applied, following D9724616, D988728, D15312250, design doc https://fb.quip.com/8P9WAaF43VOn
The peekError() API implementation is added into unidirectional stream dispatcher, class HQUnidirStreamDispatcher.
Reviewed By: mjoras
Differential Revision: D27514260
fbshipit-source-id: b65cc0e46a5d520f46f9dc218c9a65be99e6df55
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
Summary:
Previously we would only send probes for the first space which had one available, i.e. Initial before Handshake before AppData. Since we only have one PTO timer this can lead to situations where we perpetually probe with only Initials, which can significantly delay the handshake if we should have probed with Handshakes.
With this diff we will keep the single PTO timer but aggressively write more probes from all spaces if they are available.
Additionally this refactors some counters into EnumArrays
Reviewed By: yangchi
Differential Revision: D27235199
fbshipit-source-id: ef3614a833bf0f02f5806846a1335fa7ac2a4dc8
Summary:
Add an onByteEventRegistered callback that will be invoked when the
registration for a ByteEvent callback is successful.
This makes it easy for recipients of the callback to track the successful
registration AND callback recipt in a single place (the recipient class). For
example, if all successfully registered ByteEvents have been received, the
recipient can destroy itself safely, knowing that no more ByteEvents will be
sent to it.
Reviewed By: bschlinker
Differential Revision: D27100624
fbshipit-source-id: dbfeff1f4cf2367587fdb73cbd334165b3b159de
Summary: If we idle timeout when there are non control streams open, there could be some sort of issue especially with protocols like HTTP/3.
Reviewed By: yangchi
Differential Revision: D27017861
fbshipit-source-id: 319b839a641fa417a5026adf607c7bd0070cb66c
Summary: Extend the Observer to record events for all knob frame received.
Reviewed By: bschlinker
Differential Revision: D26325312
fbshipit-source-id: 810589d08be3614c8c091cccb7bb9fbdb5c65402
Summary: It's legit to set this to nullptr before the callback is removed.
Reviewed By: yangchi, lnicco
Differential Revision: D26582589
fbshipit-source-id: 0119e06e6e5347a9080dc5a814551e7b44d64c74
Summary: Adding QuiConnectionStats to client transport. Moving getConnectionsStats() logic from server worker into transport base class.
Reviewed By: lnicco
Differential Revision: D26316635
fbshipit-source-id: a384eee5d1bc7b23d908e0b03fafcc4ee962b0b9
Summary: This function is useful for when an application wants to explicitly reset all its non-control streams in one go.
Reviewed By: yangchi
Differential Revision: D26490209
fbshipit-source-id: 747449938e4dcd58bee028835f34776e5314cdf9
Summary: this param is passed to transport then ignored
Reviewed By: avasylev
Differential Revision: D26133327
fbshipit-source-id: 459dd0132185513215ba034f213d4137d7b56ba1
Summary:
Given the large number of callbacks that are being triggered from the Observer
this change makes it possible to enable through a simple config, just the
subset of callbacks that a consumer is interested in receiving.
Observer and Socket Lifecycle callbacks are enabled by default, they are not
configurable.
Reviewed By: bschlinker
Differential Revision: D25879382
fbshipit-source-id: abe79ed92e958ddc96475c347f8ec7204400cdfa
Summary:
We were using the LifecycleObserver and InstrumentationObserver classes
separately, to generate and receive callbacks.
This change migrates both these to use the unified Observer callback class and
adjusts the unit tests.
Reviewed By: bschlinker
Differential Revision: D25845845
fbshipit-source-id: c489400f5d70bccadbcc1d957136c5ade36b65ff
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: Inform LifecycleObservers when an EventBase is attached or detached from a QuicSocket.
Reviewed By: bschlinker
Differential Revision: D24294254
fbshipit-source-id: a606ad5ae4d4cbe3095e6ae32d6c74713325ad08
Summary:
Giving more access to the current state of the connection by exposing the socket.
Pull Request resolved: https://github.com/facebookincubator/mvfst/pull/185
Reviewed By: yangchi
Differential Revision: D23924861
Pulled By: bschlinker
fbshipit-source-id: 16866a93e58b6544e25d474a51ca17fab9ffdc55
Summary:
`InstrumentationObserver` is owned by `QuicSocket` now. However, to capture packet loss signal, we need it in `QuicConnectionState`. So, I refactored the code a little bit to make this easy. Changes in this code:
(a) Moved the definition of `InstrumentationObserver` and `LifeCycleObserver` to
a separate header `Observer.h`.
(b) Moved the vector of `InstrumentationObserver`s from `QuicSocket` to `QuicConnectionState`.
(c) Added a callback in `conn->pendingCallbacks` when a packet loss is detected
Reviewed By: bschlinker
Differential Revision: D23018569
fbshipit-source-id: e70d954839bdb70679ecd52d2bd1a6a6841f6778
Summary: This was probably a premature optimization and introduces complexity for dubious gain. Additionally a sequence of losses could potentially cause multiple updates to be delayed.
Reviewed By: yangchi
Differential Revision: D23628058
fbshipit-source-id: d6cf70baec8c34f0209ea791dadc724795fe0c21
Summary:
This is following a similar pattern than what was done for the client side.
Pull Request resolved: https://github.com/facebookincubator/mvfst/pull/160
Reviewed By: yangchi
Differential Revision: D23560951
Pulled By: xttjsn
fbshipit-source-id: 351417cbfa3230112fff4c4de59b307f88389cf6
Summary: ^ This might increase readability. e.g. "cbs->offset" is more readable than "cbs->first".
Reviewed By: bschlinker, yangchi
Differential Revision: D23361655
fbshipit-source-id: 811eb4439c2c717059ec5f0d15255ec5ede9a2b8
Summary: Add missing type parameter to two instances of `folly::Optional` introduced in D21653651 (98e0e4dc5d). It seems to build fine without the type parameters using buck on a devserver, but fails using CMake on an ubuntu machine.
Reviewed By: udippant
Differential Revision: D22632231
fbshipit-source-id: dec8ad2e11ce90b59dec4d7a9b9e1139b8cc1f41
Summary:
Adds support for timestamping on TX (TX byte events). This allows the application to determine when a byte that it previously wrote to the transport was put onto the wire.
Callbacks are processed within a new function `QuicTransportBase::processCallbacksAfterWriteData`, which is invoked by `writeSocketDataAndCatch`.
Reviewed By: mjoras
Differential Revision: D22008855
fbshipit-source-id: 99c1697cb74bb2387dbad231611be58f9392c99f
Summary:
Introduces framework for handling callbacks when a `ByteEvent` occurs.
- Adds `ByteEventCallback`, a new class that can be used to notify a callback implementation about delivery (ACK) of a byte and is used in the subsequent diff for notifications about TX of a byte.
- Adds `ByteEvent` and `ByteEventCancellation`, both of which are used to communicate ancillary information when a ByteEvent happens to the callback implementation. This makes it easier to add additional fields (such as kernel timestamps, information about ACK delays, etc) in the future, without requiring the function signatures of all callback implementations to be updated.
- For the moment, `DeliveryCallback` inherits from `ByteEventCallback` so that existing implementations do not need to be adjusted; `DeliveryCallback` implements `ByteEventCallback`'s methods and transforms the received `ByteEvents` into the expected callback. I will deprecate `DeliveryCallback` in a subsequent diff.
- Refactors existing logic for handling delivery callbacks to be generic and capable of handling different types of byte events. This allows the same logic for registering and cancelling byte events to be reused for ACK and TX timestamps; custom logic is only required to decide when to trigger the callback.
Reviewed By: mjoras
Differential Revision: D21877803
fbshipit-source-id: 74f344aa9f83ddee2a3d0056c97c62da4a581580
Summary:
Adds `QuicSocket::InstrumentationObserver`, an observer that can be registered to receive various transport events.
The ultimate goal of this class is to provide an interface similar to what we have through TCP tracepoints. This means we need to be able to register multiple callbacks.
- Initially, the first event exposed through the callback is app rate limited. In the future, we will expose retransmissions (which are loss + TLP), loss events (confirmed), spurious retransmits, RTT measurements, and raw ACK / send operations to enable throughput and goodput measurements.
- Multiple callbacks can be registered, but a `folly::small_vector` is used to minimize memory overhead in the common case of between 0 and 2 callbacks registered.
- We currently have a few different callback classes to support instrumentation, including `QuicTransportStatsCallback` and `QLogger`. However, neither of these meet our needs:
- We only support installing a single transport stats callback and QLogger callback, and they're both specialized to specific use cases. TransportStats is about understanding in aggregation how often an event (like CWND limited) is occurring, and QLogger is about logging a specific event, instead of notifying a callback about an event and allowing it to decide how to proceed.
- Ideally, we can find a way to create a callback class that handles all three cases; we can start strategizing around that as we extend `InstrumentationObserver` and identify overlap.
Differential Revision: D21923745
fbshipit-source-id: 9fb4337d55ba3e96a89dccf035f2f6978761583e
Summary:
Adds `QuicSocket::LifecycleObserver`, an observer that is notified when a socket is closed and destroyed.
- Can be used by instrumentation that ties its lifetime to that of the socket.
- Multiple observer can be registered, but a `folly::small_vector` is used to minimize memory overhead in the common case of 0 - 2 being registered.
- `folly::AsyncSocket` has a matching interface being added (D21613750), so instrumentation can follow the same paradigm for both QUIC and TCP.
- In the future, will extend to also be triggered when a transport becomes connected, similar to what we have for `AsyncSocket`.
Reviewed By: mjoras
Differential Revision: D21653651
fbshipit-source-id: 0aa374cc0dd9b9c11d81b49597326bd53264531d
Summary: Without this option the read callbacks come in an somewhat random order based on stream ID. This enforces ordering by stream ID, which is useful for some applications.
Reviewed By: yangchi
Differential Revision: D22442837
fbshipit-source-id: cd97de9760deff7b19841f46e23da2004df31492
Summary:
Callbacks notifying that max stream limit has been increased and new streams can be created.
This get triggered every time limit gets bumped, doesn't track if limit was previously exhausted or not.
Reviewed By: mjoras
Differential Revision: D22339814
fbshipit-source-id: 6bcd0b52adb61e1ac440d11559dc8544ad7aa1ac
Summary: Updating signature of writeChain to stop returning IOBuf, as implementation never actually returns back buffer and always writes full buffer.
Reviewed By: mjoras
Differential Revision: D21740607
fbshipit-source-id: f473ed8f3c6c6cbe2dd5db8f1247c912f3e77d0b
Summary:
as title
(Note: this ignores all push blocking failures!)
Reviewed By: mjoras
Differential Revision: D21303116
fbshipit-source-id: 5e76e02a618a5f79867adae0a78e0174630ce039
