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
Summary: no more surprises in upper layer
Reviewed By: mjoras
Differential Revision: D19976510
fbshipit-source-id: 3487e9aa2cb28d7bc748f13bc2bbc393216b4a8a
Summary: We send conn close packets when network is un-rechable, this task fixes the issue and we no longer send conn close frames when network is un-rechable.
Reviewed By: yangchi, lnicco
Differential Revision: D18469028
fbshipit-source-id: 3090b6b3d63e1fc9448b8d945c645f4da8747ffc
Summary:
Previously we track them since we thought we can get some additional
RTT samples. But these are bad RTT samples since peer can delays the acking of
pure acks. Now we no longer trust such RTT samples, there is no reason to keep
tracking pure ack packets.
Reviewed By: mjoras
Differential Revision: D18946081
fbshipit-source-id: 0a92d88e709edf8475d67791ba064c3e8b7f627a
Summary:
In the current client code we read one packet, go back to epoll, and then read
another packet. This is not very efficient.
This changes it so that we can read multiple packets in one go from an epoll
callback.
This only performs changes on the client
Reviewed By: mjoras
Differential Revision: D18797962
fbshipit-source-id: 81be82111064ade4fe3a07b1d9d3d01e180f29f5
Summary: The state machine logic is quite abstruse, this modifies it to make it more readable.
Reviewed By: siyengar
Differential Revision: D18488301
fbshipit-source-id: c6fd52973880931e34904713e8b147f56d0c4629
Summary:
F14 should be faster and have lower memory urilization for near-empty sets and maps. For most H3 connections these are mosotly going to be near-empty, so CPU wins will likely be minimal.
For usecases that have extremely high numbers of streams, there are likely going to be CPU wins.
Reviewed By: yangchi
Differential Revision: D18484047
fbshipit-source-id: 7f5616d6d6c8651ca5b03468d7d8895d1f51cb53
Summary: inspect PN, and when it reaches 2^62 -2 trigger a transport close through a pending event.
Reviewed By: yangchi
Differential Revision: D18239661
fbshipit-source-id: 1a218678099016693149e12ff121e2a39b95aecc
Summary:
Removes clientConnId completely from ServerConnectionIdParams.
This diff first fixes an incorrect assumption; it calls
`shortHeader.getConnectionId()` which is actually the destination id (server)
not the client connection id.
Next, this entire block is unnecessary, because this will be called after
the transport is created, so the clientConnectionId will always be set.
This also setsconn.serverConnectionId earlier (shouldn't depend on
connClientId).
Reviewed By: yangchi
Differential Revision: D16792866
fbshipit-source-id: 537ba12baa9939c9d5512e46eb914c1d3a7a9aa2
Summary: Add transportStateUpdate event so it can be part of qlog.
Reviewed By: mjoras
Differential Revision: D16342467
fbshipit-source-id: 109189275d44996850b82646bab4a733a3a4c7a1
Summary:
When a write callback x is already defined for a stream
and notifyPendingWriteOnStream is called with the same callback x,
it is safe to not return an error.
Building on master is currently broken due to 2040a13e40 (diff-ddd35d436da215e41b89fd5afe4acebbR11), but this commit builds when rebased before that.
udippant
Pull Request resolved: https://github.com/facebookincubator/mvfst/pull/18
Reviewed By: yangchi
Differential Revision: D15562893
Pulled By: afrind
fbshipit-source-id: 44e62bb9bb0002508257acdc419f20bcb973ca64
Summary:
This introduce quic::Aead as a simple typedef to fizz::Aead and update the codebase to use quic::Aead . This should not impact the functionality of the code in any way.
This is a first step toward introducing an interface that is specific for mvfst so that mvfst can swap fizz for something else.
Pull Request resolved: https://github.com/facebookincubator/mvfst/pull/12
Reviewed By: JunqiWang
Differential Revision: D15335324
Pulled By: mjoras
fbshipit-source-id: fef166a9a5c2cbae08ad9511d0abd749f330c221
Summary:
Current app-limited is defined as the connection doesn't have a no
non-control stream. I'm changing this to be app-idle. And app-limited will be
changed to a state in connection where app isn't sending bytes fast enough to
keep congestion controller probing higher bandwidth.
Reviewed By: mjoras
Differential Revision: D15456199
fbshipit-source-id: e084bc133284ae37ee6da8ebb3c12f505011521e
Summary:
Peek callback wasn't raised on data arrival. The fix is to:
* always update peek looper before read looper
* always update `peekableList` together with `readableList`
Differential Revision: D14757064
fbshipit-source-id: bb1e43762f54d024050eced425351f03a0cffd9f
Summary:
Previously updateAppLimited was not accounting for peer created streams.
This moves app limited accounting into the stream manager
This also makes a subtle change to the applimited callback. We assume that
we start off as not app limited now, so if we create a stream we will not call
the congestion controller app limited callback.
Reviewed By: mjoras
Differential Revision: D15166114
fbshipit-source-id: 1a8d573533861f53bb1bd9fdc605dfefe68902dc
