Summary:
- Skip ACK-only response to initial crypto data from client
- Enabled through experimental transport settings
Reviewed By: mjoras
Differential Revision: D31863653
fbshipit-source-id: b290db0399dd7a3be41078c4a839b484da864f2f
Summary: Making it possible to reuse the foundation of `QuicServerTransportTest` elsewhere. Will be building a test that uses `TYPED_TEST` to test against both transports.
Reviewed By: lnicco
Differential Revision: D31886976
fbshipit-source-id: a158d94e1a75866e21f505f21f0f7401326e76f4
Summary: rename test local variables to be self documenting
Reviewed By: mjoras
Differential Revision: D32750782
fbshipit-source-id: 94ff5bbd34dbc804cd0229d8abd0ffd9891a44fc
Summary:
- Change the knob param handler to receive a Transport rather than a ConnectionState.
- Add a new transport knob that can set the priority threshold and target utilization factor for automatically controlling background mode based upon active streams.
Reviewed By: mjoras
Differential Revision: D31847477
fbshipit-source-id: 4cfeff04b9fe60eb25ab484f460e252ef6970646
Summary:
Experiment with a less aggressive variant of BBR that could be used for background requests in order to improve responsiveness of foreground requests.
BBR's agressiveness can be controlled through a new transport knob CC_AGRESSIVENESS_KNOB(0xccab) which accepts values between 25-100. The value indicates the percentage of the available BW the congestion controller should try to use, leaving headroom for other flows.
Aggressiveness of BBR is reduced by adjusting BBR's startup gain during the STARTUP phase. During the ProbeBW phase, BBR uses a longer pacing gain progression with values that intentionally underutilize the available BW.
Reviewed By: bschlinker, mjoras
Differential Revision: D31219364
fbshipit-source-id: c67ffcf03d27fd4d41be2d58f00f4f960c7646c7
Summary: As in title. This isn't actually very risky to have on, and is actually confusing if you try to use the APIs without the flag since nothing will happen.
Reviewed By: shodoco
Differential Revision: D30585491
fbshipit-source-id: d79598ab5c723675d63cb71f011b9107acda6d7d
Summary: It's useful at the end of a connection to know if we tried DSR.
Reviewed By: jbeshay
Differential Revision: D30545282
fbshipit-source-id: bbb2f3408f7a2d5666676c9e2583bf8fd9f18911
Summary:
- Removed packetNum field from CipherUnavailable struct.
- Removed all instances referring to the field and fixed tests accordingly.
Reviewed By: mjoras
Differential Revision: D29968168
fbshipit-source-id: 9802b8cd66f43f2a8d54340f2d00639ee4679aaf
Summary:
clear DSR buffers, release flow control, calling release() on the
packetization request sender. For now the stream will own the sender until
stream itself is dead. We need to change this ownership model later to be able
to reset the pointer when we reset the stream.
Reviewed By: mjoras
Differential Revision: D27901663
fbshipit-source-id: d9d12ef95ae59c6f0fe7ac1b1589d8527b1bc48d
Summary:
This diff hooks the DSR write function into QuicServerTransport's
write path.
Reviewed By: mjoras
Differential Revision: D27890711
fbshipit-source-id: ac4452373c0baafe091f93fb54fccf87be604a9c
Summary:
will use it for other tests as well
(Note: this ignores all push blocking failures!)
Reviewed By: lnicco
Differential Revision: D27024118
fbshipit-source-id: 27b0182525787aa2969f789b1e4eb8deca296e69
Summary:
as title. This also moves a FizzCryptoTestFactory from FizzCryptoFactoryTest to TestUtils so that it can be used in other test code
This change has an unfortunate side-effect that cryptoFactory_ in both client and server will be moved from stack to heap.
Reviewed By: mjoras
Differential Revision: D27264488
fbshipit-source-id: febc307fb02cb136d58fe70bee648d35431acff0
Summary: This seems to cause some issues with CGNAT type networks where the client is actually using v6 or v4.
Reviewed By: yangchi
Differential Revision: D27772485
fbshipit-source-id: ac118441caad38301f2a22e657cefb398a5210da
Summary:
On receiving a QUIC packet, if the packet has no frames we should end the connection with PROTOCOL_VIOLATION.
This fixes the error reported by h3spec test `/QUIC servers/MUST send PROTOCOL_VIOLATION on no frames [Transport 12.4]/`
This change adds the check right after a packet is successfully parsed for both the client and server.
Reviewed By: mjoras
Differential Revision: D27483874
fbshipit-source-id: 9b648709e6985f151ba0ffc973aa05c28683fbe9
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: 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:
Before the change, there's no good way to recreate Cubic CC instance with custom CC factory, because Cubic is created by default.
On client side this requires calling setCongestionControl() or setTransportSettings() after calling setCongestionControllerFactory(), which is normally the case.
Reviewed By: yangchi
Differential Revision: D26401996
fbshipit-source-id: dfda39be835c67b9db42f726b3ac64c7b3d37c2f
Summary:
Having the lines `using namespace testing;` and `using namespace folly;` or `using folly::Optional;` causes a build error with the new googletest version because of `testing::Optional` being a new symbol from googletest
## How I made this diff:
From the list of files
[rquitt@devbig013.ftw5 ~/fbsource/fbcode/scripts/rquitt] grep -r "'Optional' is ambiguous" | sed 's/stderr: //' | cut -d ':' -f 2 | sort -u
admarket/adreview/video_understanding/server/test/VusMothershipManagerTest.cpp
admarket/adreview/video_understanding/server/test/VusTaskManagerTest.cpp
archive_service/blockio/SdcLogicalBlockWriterTest.cpp
...
Open all the files in Vim and use a command like: `:%s/[^y/:]Optional/folly::Optional/`
Also removed all the `using folly::Optional;` lines to discourage further use of ambiguous `Optional` before googletest change is landed.
The alternative to this diff would be to not export `testing::Optional` from googletest.
Reviewed By: igorsugak
Differential Revision: D26358560
fbshipit-source-id: ae8695b3525bf333758b012adcfe944383777625
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: Adds another knob param to enforce udp payload size. This is basically a "canIgnorePathMTU" knob that client has.
Reviewed By: mjoras
Differential Revision: D24586165
fbshipit-source-id: befb265a24fae8f450f323cf2d652a8b448e698c
Summary: Blackhole detection has quite a lot false positives, we want to have a way to run d6d without it.
Reviewed By: mjoras
Differential Revision: D24584357
fbshipit-source-id: ab27655ec38d62fd6deffe41cb156de0c981cf6d
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: Since the DCID is often empty for clients, this is otherwise not a very useful field.
Reviewed By: yangchi
Differential Revision: D23998639
fbshipit-source-id: b8949ca6913ed270e5ebd0a0c5335b224f817774
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:
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:
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: This diff creates a new `ServerCongestionControllerFactory` that will be used to create cc instances on the server only and thus can have different dependencies. At the moment this mirrors `DefaultCongestionControllerFactory`, but later in the stack when CCP is added, only the server cc factory will depend on (and be able to create) CCP instances, but the default one will not. This prevents any client builds from depending on CCP.
Reviewed By: yangchi
Differential Revision: D22139289
fbshipit-source-id: 2987f0234bc54ea6101ca0030b319b460571adeb
Summary:
The problem with Ping being a simple frame:
(1) All SimpleFrames are in the same scheduler. So sending ping means we may
also send other frames which can be problematic if we send in Initial or
Handshake space
(2) Ping isn't retranmisttable. But other Simple frames are. So we are
certainly setting this wrong when we send pure Ping packet today.
That being said, there are cases where we need to treat Ping as retransmittable.
One is when it comes to update ack state: If peer sends us Ping, we may want to
Ack early rather than late. so it makes sense to treat Ping as retransmittable.
Another place is insertion into OutstandingPackets list. When our API user sends
Ping, then also add a Ping timeout. Without adding pure Ping packets into OP list,
we won't be able to track the acks to our Pings.
Reviewed By: mjoras
Differential Revision: D21763935
fbshipit-source-id: a04e97b50cf4dd4e3974320a4d2cc16eda48eef9
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:
This implements the connection ID validation via transport parameters. Note we don't do anything with the retry transport parameter yet.
This will probably require further surgery to tests when we want the MVFST version to do this, but for now I'm punting on that test plumbing.
This retains support for h3-27.
Reviewed By: yangchi
Differential Revision: D22045631
fbshipit-source-id: e93841e734c0683655c751d808fd90b3b391eb3e
Summary: 0 is now a valid packet number, so we should make these optional. In cases where they are needed to construct packet builder, it should be safe to use 0 as default since it's only used for computing `twiceDistance` in PacketNumber.cpp.
Reviewed By: yangchi
Differential Revision: D21948454
fbshipit-source-id: af9fdc3e28ff85f1594296c4d436f24685a0acd6
Summary: There's no particular reason to wait for the drain period before removing state. By doing this a failed migration will immediately trigger the server to drop state, triggered a stateless reset signal to the client sooner.
Reviewed By: yangchi, lnicco
Differential Revision: D21643179
fbshipit-source-id: a60ca2c92935a3e6ba773d7936c25317733f7b97
Summary:
Becuase when we clone an existing packet, the logic inside the current
writetStreamFrameHeader is no longer correct.
Reviewed By: mjoras
Differential Revision: D21383828
fbshipit-source-id: 8e6bbb048eefd97ca7cf17b89edc2f395f274a73
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
