Summary: - transport settings to enable the following behaviour: invoking stopSending() now drops buffered ingress data and closes the ingress path
Reviewed By: mjoras
Differential Revision: D40678864
fbshipit-source-id: 7dd5c7843909d5ac8cc309292b31d1d87b97a2f5
Summary: - as title, we should just no-op ::StopSending when ingress is already closed
Reviewed By: jbeshay, mjoras
Differential Revision: D40990383
fbshipit-source-id: e0cd64facf78f510eabe8198e93a643c6ebfb89e
Summary: Enable more tests for Windows
Reviewed By: mjoras
Differential Revision: D39595126
fbshipit-source-id: 702d243ff7fbb5ffdb18af2dd1d13c184599ef75
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: As titled, enables `QuicSocket` to support observers that are managed pointers. The underlying support for this was added to `folly::ObserverContainer` in D35455208.
Differential Revision: D36921806
fbshipit-source-id: 0ed710977d08866ae32ed7231101c6de163aecf3
Summary: Implement group streams receiver api in transport
Reviewed By: mjoras
Differential Revision: D36419901
fbshipit-source-id: 98bfefa1a4205fde8764f2e4300f51156667e024
Summary:
Current behavior is to create streams "lazily", that is pre-allocate stream ids that are lower than the stream id received in stream frame, even though we have not seen the lower streams yet. The `onNew(Bi|Uni)directionalStream()` callbacks are also raised implicitly, that is on lower stream ids pre-allocation, instead of when we actually get a stream frame with those lower ids.
With the new flag the callbacks will be raised to the user on actual arrival of the frame with the stream id on the wire.
Reviewed By: mjoras
Differential Revision: D36465978
fbshipit-source-id: 91faaadaad106a2bf62348c8fe8f00b3d7c61c2c
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: In order to provide more information about datagrams when we receive them, I'm adding a wrapper around the BufQueue that we currently hold so we can include receiveTimePoint and any other metadata we might want to expose for the Datagram API.
Reviewed By: mjoras
Differential Revision: D33994358
fbshipit-source-id: 805f182cd350908320639bc07eb6f3b7349bbc05
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:
Adds a transport callback for ping received.
Uses the callback in the HTTP3 session to reset the timeout
Also now invokes the ping callbacks synchronously to avoid issues during connection shutdown
Reviewed By: mjoras
Differential Revision: D33753068
fbshipit-source-id: 99ac14d0e4b9539f3a20c5c55bb5241351bf1c57
Summary:
Adds a transport callback for ping received.
Uses the callback in the HTTP3 session to reset the timeout
Reviewed By: mjoras
Differential Revision: D33101180
fbshipit-source-id: d6fcecd22cbd5c311674dd9421c0c54eb04728a0
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
