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
Summary:
This is a pretty confusing API behavior. This means that when reading from stream A, stream B could potentially be removed if it doesn't have a read callback set. This effectively means you cannot safely call read from onNew*Stream.
There's no real reason the check for a closed stream here, as the streams will get cleaned up from the onNetworkData or read looper calls.
Reviewed By: yangchi
Differential Revision: D20848090
fbshipit-source-id: 38eba9e5a24b6ddf5bf05ac75787dda9c001c1c6
Summary:
Right now we use un-sanitized error message on onConnectionError only.
This also uses it in app callbacks
Reviewed By: lnicco
Differential Revision: D20843327
fbshipit-source-id: c81896d41b712a7165ac6f6b381d3687ecca2a3a
Summary: It's not a great practice to leak the excpetion string via a conn close, but it is useful for the app to be able to report what the exception string was.
Reviewed By: yangchi
Differential Revision: D20628591
fbshipit-source-id: bf6eb5f33f516cec0034caed53da998643fcc120
Summary:
Currnetly there isn't a way for apps to unregister a pending
WriteCallbacks for a stream. resetStream() does that if the transport isn't in
Closed state. This diff adds such support even if transport is already in
Closed state. This solves the problem where app has a class that's both stream
ReadCallback and stream WriteCallback and readError would kill the callback
object itself. The new API gives such class a chance to remove itself from the
transport.
Reviewed By: mjoras
Differential Revision: D20545067
fbshipit-source-id: 81d9f025310769aadef062711a49adc47a0639d0
Summary: Previously we would end up writing a non-application close when the application calls close(folly::none). This isn't correct, as those cases should be an application error with no error.
Reviewed By: afrind
Differential Revision: D20518529
fbshipit-source-id: fe069cccc32bd550fb3ec599694110955816993f
Summary: Resetting a stream has the semantics of abandoning all writes on that stream. As such, the application resetting the stream is an implicit signal that it does not care about further writes. It is reasonable then for an application to not expect further write callbacks to trigger.
Reviewed By: lnicco
Differential Revision: D20462859
fbshipit-source-id: b6701e6a262d618c5cd93fd1531095a134f6554e
Summary: When we don't use NiceMock we end up with a ton of spam in failing tests for every callback that we didn't EXPECT. This makes failed test output extremely noisy.
Reviewed By: sharmafb
Differential Revision: D19977113
fbshipit-source-id: 1a083fba13308cd3f2859da364c8106e349775bb
