Summary: To scale well when we have more transport knob params, server can maintain a map from param id -> handler function. The handler function should avoid storing states / perform proper checking if it does. Most use cases should be covered by the "server_conn" param.
Reviewed By: mjoras
Differential Revision: D24584358
fbshipit-source-id: e45da50deb6ebd385b6a71e1b48f4650bc6ace91
Summary: Temporarily, we use a simple json-format for transport knob. Currently it's limited to a single packet, but that should be enough for current usage.
Reviewed By: mjoras
Differential Revision: D24584261
fbshipit-source-id: f51ed0fb7560cda2ca447a49d9ad6575c1b2a59f
Summary: The D6DEnabled integration test was flaky because of a race condition between stats callback and client's transportReady callback. Simple fix by ~~send some data and wait for echo~~ calling terminateLoopSoon in onConnectionD6DStarted. Still, it makes sense to make transportReady the last callback in server so I switched the order.
Reviewed By: yangchi
Differential Revision: D24198017
fbshipit-source-id: 36a7b6377c970b2f2f5d072f03d8bc8b0837fd79
Summary: This reduces dependencies for both testing and instrumentation.
Reviewed By: mjoras
Differential Revision: D23997313
fbshipit-source-id: 5eb3a790c7bb2569dc1e941e3911ad4aac4e9258
Summary:
After some experiments where probes are obviously causing congestion, it now
makes sense to pose some delay before sending the next probe. This is not
mentioned in the d6d spec, but in a related spec rfc4821 iirc, where consecutive
probes should have 1sec delay.
Reviewed By: mjoras
Differential Revision: D23910766
fbshipit-source-id: dcf5d05c4590489be503563c98144e12c3987cff
Summary: As a drive-by: fixed a bug where I didn't cancel the pending event when timeouts expire.
Reviewed By: mjoras
Differential Revision: D23910767
fbshipit-source-id: 233e590c17a6c6b7a5f4a251bd8f78f6dfae3c0b
Summary:
This glues together the d6d lifecycle via probe timeout and raise timeout.
Had to put these two timeouts in the base transport because it has all the
necessary accountings (e.g. check close state, process callbacks) that should
happen before scheduling timeouts.
Other notable changes (included here because code is simple):
- Keep track of d6d probes in loss state. Upon second thought, it makes more
sense because we are reducing the available bandwidth as a result of sending
probes anyway. And not tracking them imposes a delay on congestion controller.
I think this does not violate the d6d spec's point of not penalizing congestion
window for d6d probes, because
- 1. we don't put losses of d6d probes in loss event. Therefore from the POV of
congestion controller, d6d probes never get lost.
- there will be at most kDefaultD6DMaxOutstandingProbes losses (i.e. 2)
that we don't tell congestion controller about. Even if those are actually
caused by congestion, it should have minimal impact because 2 is small and if there's really a congestion, the loss of normal packets should provide the signal.
- Pacing d6d probes
- Kick off d6d after a delay of 1s. This should filter out short-lived connections where probing is relatively expensive and less useful.
Reviewed By: mjoras
Differential Revision: D23736656
fbshipit-source-id: 8121fa8bcebab10a56a4e046c32c4e99ed6c3013
Summary:
This is one of the pre-condition of starting d6d probing. Starting d6d once
hasWriterCipher() is too early becaue an overlarge PMTU could potentially
cause the server to send oversized handshake packet and disrupt handshake.
Reviewed By: yangchi
Differential Revision: D23698784
fbshipit-source-id: f18824a8ef516421832d8cc769f4880a0841e492
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 diff adds an API to expose client certificate from QuicSocket.
Reviewed By: mjoras, lnicco
Differential Revision: D20374792
fbshipit-source-id: 1b0691bba49ef57cb824aba8563644afc7b1962b
Summary:
First step towards d6d. Semantically we need to separate the old `udpSendPacketLen` into `peerMaxPacketSize` as well as `currPMTU`. The former is directly tied to the peer's max_packet_size transport parameter whereas the second is controlled by d6d. To get the actual udp mss, call `conn_->getUdpSendPacketLen()`, which will use the minimum of the two if d6d is enabled, otherwise it will fallback to use `peerMaxPacketSize` only.
During processClientInitialParams and processServerInitialParams, we no longer need to check whether `canIgnorePathMTU` is set because that logic is moved to `setUdpSendPacketLen`. If d6d is enabled, we set both `peerMaxPacketSize` and `currPMTU` to `packetSize` because receiving an initial packet of size x indicates both that the peer accepts x-sized packet and that the PMTU is at least x.
Many call sites and tests are changed.
Faebook:
For now, d6d is considered enabled if `canIgnorePathMTU==false` and `turnoffPMTUD==true`. Down the road, from semantic & practical POV at least one of them should be renamed to something like `enableD6D`, since enabling d6d implies turning off PMTUD and that we should not ignore PMTU. We can keep one for the sake of testing.
Reviewed By: mjoras
Differential Revision: D22049806
fbshipit-source-id: 7a9b30b7e2519c132101509be56a9e63b803dc93
Summary:
The way libccp is structured, it requires each instance of the QuicCCP cc algorithm to access a field (`ccpDatapath`) from the corresponding ServerWorker handling that connection. However, the constructor for cc algorithms only takes a single input, a `QuicConnectionStateBase`. So, the only way to pass it through without changing the API is to add it as a field to this struct.
On the server, the QCSB will always be an instance of the subclass `QuicServerConnectionState` -- since CCP will only ever be used on the server side, we can add `ccpDatapath` to QSCS and then in QuicCCP we can `static_cast` the `QCSB` to a `QSCS` to access it. This makes it possible to build the client without any dependency on CCP related things.
Reviewed By: udippant
Differential Revision: D21854348
fbshipit-source-id: a1f44ac177459880d850660039ce7477e6f57132
Summary:
On loss timer, currently we knock all handshake packets out of the OP
list and resend everything. This means miss RTT sampling opportunities during
handshake if loss timer fires, and given our initial loss timer is likely not a
good fit for many networks, it probably fires a lot.
This diff keeps handshake packets in the OP list, and add packet cloning
support to handshake packets so we can clone them and send as probes.
With this, the handshake alarm is finally removed. PTO will take care of all
packet number space.
The diff also fixes a bug in the CloningScheduler where we missed cipher
overhead setting. That broke a few unit tests once we started to clone
handshake packets.
The writeProbingDataToSocket API is also changed to support passing a token to
it so when we clone Initial, token is added correctly. This is because during
packet cloning, we only clone frames. Headers are fresh built.
The diff also changed the cloning behavior when there is only one outstanding
packet. Currently we clone it twice and send two packets. There is no point of
doing that. Now when loss timer fires and when there is only one outstanding
packet, we only clone once.
The PacketEvent, which was an alias of PacketNumber, is now a real type that
has both PacketNumber and PacketNumberSpace to support cloning of handshake
packets. I think in the long term we should refactor PacketNumber itself into a
real type.
Reviewed By: mjoras
Differential Revision: D19863693
fbshipit-source-id: e427bb392021445a9388c15e7ea807852ddcbd08
Summary:
Now we won't have a zero PTO and we will properly clear out the outstanding packets.
Note that this cipher dropping is not what the draft prescribes, instead dropping both the initial and handshake ciphers when we know 1-rtt communication is functioning.
Reviewed By: yangchi
Differential Revision: D20388737
fbshipit-source-id: 0b89eb80c8faa796ab09eda3eaa10a00dcf7bae9
Summary:
This ensures they are available to the whole stack rather than the transport only. The validator needs it in the server case, and will soon need it in the client case, so that seems appropriate to make it available.
Pull Request resolved: https://github.com/facebookincubator/mvfst/pull/117
Reviewed By: yangchi
Differential Revision: D20536366
Pulled By: mjoras
fbshipit-source-id: a76d369c0a82b9be1f985aed1f33f7a6b338a2ae
Summary:
The CID Algo can possibly generate a CID that's already in the map.
This diff adds a mechanism to reject such CID and try another one.
ServerStateMachine will loop encoding CIDs until either QuicServerWorker no
longer rejects, or encode fails
Reviewed By: udippant, vchynarov
Differential Revision: D20251043
fbshipit-source-id: a38e4e8b33007779a9710c32057d47f32f7d1774
Summary:
This eliminatees some tech debt by completely removing the notion of version from the core transport parameters structure and the app token for zero rtt.
Note that for the draft-27 changes we will need to temporarily re-introduce it, but to a different layer (the extension encoding itself).
Reviewed By: JunqiWang
Differential Revision: D20073578
fbshipit-source-id: 2b55af621566bf1c20e21dd17251116de1788fa0
Summary: This implements the handshake done signal and also cipher dropping.
Reviewed By: yangchi
Differential Revision: D19584922
fbshipit-source-id: a98bec8f1076393b051ff65a2d8aae7d572b42f5
Summary:
All instancesi of LIKELY and UNLIKELY probably should be removed. We will
add them back in if we see pathologies in performance profiles.
Reviewed By: mjoras
Differential Revision: D19163441
fbshipit-source-id: c4c2494d18ecfd28f00af1e68ecaf1e85c1a2e10
Summary:
Currently, before server generate the destination CID, we route packets with client's address, port and client's source connection ID. But now that client can use 0-len source connection ID, the different connections from the same client address and port will be routed to the same server connections.
This diff changes it to use client's initial destination connection ID as part of the routing key.
Reviewed By: udippant
Differential Revision: D19268354
fbshipit-source-id: 837f5bd2f1e3a74957afacf7aabad922b1719219
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:
It was hard to understand which names refer to which limits.
This diff makes it simple:
* conn.transportSettings.selfActiveConnectionIdLimit
represents how many of its peer's connection ids it will hold. It sends this to
the peer as the active_connection_id_limit
* conn.peerActiveConnectionIdLimit represents the value the peer sends as its
active_connection_id_limit. This should be defaulted to 0 and only changed
when we receive the transport parameters
Reviewed By: udippant
Differential Revision: D18531733
fbshipit-source-id: 53709ccaa58f835fd654ac28cdd740be46e65289
Summary:
Use the helper function from earlier to create new connection id with sequence
number, and stateless reset token.
Add each of those new connection ids to the routing callback. Add a CHECK()
for routingCallback, because it should be set.
Add a new parametrized set of tests, to test the value of the
active_connection_id_limit transport parameter sent from client.
Reviewed By: yangchi
Differential Revision: D15178642
fbshipit-source-id: 37b4879b09a47d371100c7ac0ab4f01130245715
Summary:
Using the helper introduced in the previous diff, we replace generating the
initial server connection id, adding it to the selfConnectionIds, and
generating the initial stateless reset token.
Reviewed By: yangchi
Differential Revision: D18066595
fbshipit-source-id: f37450eecf0926ec145bc7a7194f2a1651cae024
Summary:
My goal in this stack is to properly support the functionality of issuing new
connection ids.
There are at least three cases in which this is done (server-side):
* generate initial server connection id based on host id parameters
* provide several NewConnectionId frames to the client (for use with
migration) after handshake
* responding to RetireConnectionIdFrame and issuing more NewConnectionId frames
as needed
**Changes**:
* move connIdAlgo away from parent ConnectionStateBase to just
QuicServerConnectionState
* create virtual function in ConnectionStateBase to generate
folly::none ConnectionIdData by default,
and server subclass will generate a new server id
* add unit tests to test this behaviour.
Intended for Junqi's diff of server issuing 7 new connection ids
to be rebased on this (D15178642)
Reviewed By: yangchi
Differential Revision: D17840780
fbshipit-source-id: 1a01077742d01d058eb0a726c4c9c44e39eafb24
Summary:
Everytime a client/server sets a client/server conn id, it adds it to the
respective self/peer connection id data collections.
Reviewed By: sharma95
Differential Revision: D17577333
fbshipit-source-id: de8b887c1f3acb142c070727fb98ca0841337369
Summary:
This diff is the one that introduces+passes the test of multiple server conn
ids routing to a single transport.
Currently, there is still only one time in non-test code that more than one
server connection id is added. This diff ensures when we call connectionUnbound
we remove all associated connection ids.
Reviewed By: udippant
Differential Revision: D17479563
fbshipit-source-id: 8d0ebbf26251f7bef44cdea53dfd4ff93ab465ae
Summary:
Currently, there is a 1:1 relationship between conn Id and transports. So for
all connections with a bound connection id, we iterate over all
connectionIdMap_ entries and delete the associate transport.
This won't work if connectionIdMap_ will map multiple conn ids to a single
transport.
Reviewed By: JunqiWang
Differential Revision: D17456965
fbshipit-source-id: 8923540abf205cfd10ad09d637e8c05790b4acf9
Summary:
Add new data structure, `ConnectionIdInfo` which is will store
ConnectionId, folly::SocketAddress, and uint64_t sequenceNumber.
Add `addConnectionId()` method to QuicServerTransport, which will both
create+add a `ConnectionIdData` object to `ServerConnectionIds` and
call `routingCb_->onConnectionIdAvailable`.
Add a matching-source address check in QuicServerWorker when matching
based on ConnectionId.
Update QuicServerTest to test for change of address on one conn id.
Right now the semantics (apart from dropping mismatched source addresses
on the same connection id) are the same. Only one conn id can ever be added.
After this, we need to allow the QuicServerWorker to
hold a list of transports. Right now, `shutdownAllConnections()` works
because the mapping of conn Id<->transport is 1:1.
Reviewed By: JunqiWang
Differential Revision: D17427573
fbshipit-source-id: 47b34d722fce8b48a5e185b0aeb05624a4fb8e94
Summary:
Use the new Pacer interface in the transport where we currently
directly use CongestinoController interrace for paciner related APIs.
Reviewed By: mjoras
Differential Revision: D16918672
fbshipit-source-id: 410f72d567e71bdd3279e344f6e9abf5e132518e
Summary:
This parameter duplicates the one in the QuicConnectionStateBase.
It is set everywhere the canonical one is, and it isn't used directly in any
manner that is separate from QuicConnectionStateBase::clientConnId.
This diff changes the following:
* removed clientConnId from ServerConnIdParams
* add setter to directly initialize clientConnId on the transport from the
connection
* update tests
Reviewed By: udippant
Differential Revision: D16711180
fbshipit-source-id: 63e667aa0df8a79757f18fb3ffc178c7c9613e1a
Summary: Update qlog format to be more complete. Adding the summary section (with extra fields like title, description, etc). This diff is just to make the format more on par with Robin's schema.
Reviewed By: mjoras
Differential Revision: D16499808
fbshipit-source-id: 56cfbb95404f7e3c6638bffda18b53f7d83048a1
Summary: Add transportStateUpdate event so it can be part of qlog.
Reviewed By: mjoras
Differential Revision: D16342467
fbshipit-source-id: 109189275d44996850b82646bab4a733a3a4c7a1
Summary: validate app params (H3) with new API, server side
Reviewed By: mjoras
Differential Revision: D16228821
fbshipit-source-id: e97e4425e9ca7316c92f0e646902c63dd46374f8
Summary:
Draft-22 onwards uses two one byte length fields to encode connection ID length instead of one one byte length field.
To support this without disrupting existing clients we need to make our parsing version dependent. This diff accomplishes it by special casing the existing Facebook client QUIC version (0xfaceb000), and changing the default Facebook client version going forward to 0xfaceb001.
Note that this diff also changes the behavior of the ticket transport parameters. When we changed from draft-18 to draft-19 transport parameters I apparently forgot to update the ticket transport parameters to the new format.
Reviewed By: yangchi
Differential Revision: D16205090
fbshipit-source-id: e74a92fa959d308f4bb43bad76e58d4b58d07322
Summary:
This ensure a lot of code do not depend on fizz anymore.
Pull Request resolved: https://github.com/facebookincubator/mvfst/pull/26
Reviewed By: mjoras, JunqiWang
Differential Revision: D16030663
Pulled By: yangchi
fbshipit-source-id: a3cc34905a6afb657da194e2166434425e7e163c
Summary: Replace hard coded stateless reset token with a token from the stateless reset token generator.
Reviewed By: yangchi
Differential Revision: D15481858
fbshipit-source-id: 30c96843c38c616600466b2fabb6defd5fcc5799
