Summary:
Fixing dependency loop introduced by D37799050 (96abc8160d)
Running `autodeps` yields the following patch:
```
--- a/xplat/quic/state/TARGETS
+++ b/xplat/quic/state/TARGETS
@@ -43,8 +43,8 @@
exported_deps = [
"//folly:random",
"//quic:constants",
+ "//quic/codec:codec",
"//quic/codec:types",
- "//quic/common:circular_deque",
"//quic/common:interval_set",
],
)
```
If this patch is applied, there is a circular dependency loop between `//quic/codec:codec` and `//quic/state:ack_states` by way of `//quic/codec:types`; this loop was introduced by D37799050 (96abc8160d).
Fixed by separating out headers files and targets. In parallel, renamed structures used for writing ACK frames (which were the reason this loop occurred) to make their role clear.
Differential Revision: D42281359
fbshipit-source-id: 8514c99f3fe72ff1d942d7f303e4a209838c7623
Summary:
We previously made the decision to disallow the writing of a FIN on a DSR stream from a non-DSR stream frame as there's not really much value in allowing it.
The code that was previously allowing this to work has a bad bug. If the DSR bytes for a stream were written completely before the non-DSR bytes had been written, the scheduling code would set `canSetFin = true`, which would cause a FIN to be written at the `bufMetaStartingOffset`. This would race with the FIN sent by the backend and, if not cause a connection error, cause an HTTP truncated body of length 0.
We can avoid this by finishing the job of not allowing DSR streams to be FIN'd by non-DSR stream frames.
Reviewed By: hanidamlaj
Differential Revision: D41479274
fbshipit-source-id: 66b321db72fdb4a16378e33048e3fb97133ced92
Summary: We need to do this to maintain consistency with the schedulers. Prior to this fix when we ran out of connection flow control and had lost DSR data the non-DSR stream scheduler would do an empty write _and_ the DSR stream scheduler would do an empty write. To fully resolve the issue we need to track the streams separately (note that the same stream can be in both sets).
Reviewed By: jbeshay
Differential Revision: D40003361
fbshipit-source-id: fd3e567bc8a2bc8b71d4c4543894053fa6e24dc4
Summary:
When 0-rtt is used and an initial packet is lost, the retransmission of this packet may end up shorter than allowed because the padding frames are subject to the cwnd.
This change makes the padding of initial packets not subject to the cwnd.
Reviewed By: mjoras
Differential Revision: D39594390
fbshipit-source-id: 2d714c921f243f8a59577a6edaaeaa1c7e2be815
Summary: Add the new IMMEDIATE_ACK frame from the ack frequency draft.
Reviewed By: mjoras
Differential Revision: D38523438
fbshipit-source-id: 79f4a26160ccf4333fb79897ab4ace2ed262fa01
Summary:
We may end up writing acks after a length-less stream frame and it
will be considered garbage data by the peer. The acks will be written later as
usual, outside of cloning scheduler.
Reviewed By: mjoras
Differential Revision: D35561912
fbshipit-source-id: 9334523b984870fa525c856a504ae7d2ae4f34c3
Summary:
Adds paddingModulo to QuicPacketBuilder, which pads up short header packets so that the length remaining of a packet is a multiple of paddingModulo
ie a message of length 25 and max packet size of 40 with a paddingModulo of 8 will get 7 padding frames to a total length of 32. (and a message of length 26 will get 6 padding frames to also go up to a total length of 32)
Reviewed By: mjoras
Differential Revision: D32858254
fbshipit-source-id: 99ada01108429db9f9bba1e342daf99e80385179
Summary: When there is not enough space in the QUIC packet to write a datagram frame, we should not dequeue the datagram
Reviewed By: mjoras
Differential Revision: D33109603
fbshipit-source-id: 937a5a0ffe55c7f88e39faf224e7ad06ca599708
Summary:
This is a bug that could prevent us from writing data if we ran out of connection flow control while we had lost data.
The last attempt missed a mistake in the scheduling of sequential priority streams.
Reviewed By: kvtsoy
Differential Revision: D33030784
fbshipit-source-id: e1b82234346a604875a9ffe9ab7bc5fb398450ed
Summary: This is causing empty write loops for some reason.
Reviewed By: jbeshay, lnicco
Differential Revision: D32990291
fbshipit-source-id: 2a183d591de54c7fe0ca54aea828a697a4cd9af0
Summary: This is an edge case we weren't handling properly. This can lead to situations where we run out of conn flow control, the peer hasn't received all data, and then we never write the data to trigger a flow control update.
Reviewed By: afrind
Differential Revision: D32887140
fbshipit-source-id: df5dfad8e0775ef43e5ca6ab98b8ca6b5987ce31
Summary:
These are either no longer relevant, are unlikely to be done, or are spculative enough that they don't deserve code space.
Hope here is to make our search for TODOs higher signal.
Reviewed By: lnicco
Differential Revision: D29769792
fbshipit-source-id: 7cfa62cdc15e72d8b7b0cd5dbb5913ea3ca3dc5a
Summary:
This is a bug fix. When a stream finished sending all it's BufMetas, it can
send EOF from frontend QUIC host. In that case, it will use
currentWriteOffset value which is wrong.
This diff changes it so that (1) frontend can only write FIN only frame if
writeBufMeta's offset is <= finalWriteOffset; (2) When it writes such frame
it will use finalWriteOffset as the offset value in the frame.
Reviewed By: lnicco
Differential Revision: D28727029
fbshipit-source-id: 8f963c2e2d66f921f8a9704ed4671ec4f7c04df7
Summary:
When rebuilding outstanding packets, if the packet contains an ACK, use a fresh ACK state instead of the potentially stale one from the outstanding packet.
Collateral changes:
- The AckVisitor logic in processAckFrame now visits AckFrames all the time. This is to guarantee that they are visited even if they belong to cloned packets. The behavior for all other frame types remains unchanged.
- If rebuilding the AckFrame is not successful, it is ignored. The rest of the clone packet is still sent.
I have tried to address all the concerns that were previously raised on D27377752
Reviewed By: yangchi
Differential Revision: D28659967
fbshipit-source-id: fc3c76b234a6e7140dbf038b2a8a44da8fd55bcd
Summary:
This was completely missing previously, which led to Client quickly
shutting down a connection with flow control violation when server oversends in
DSR mode.
Reviewed By: mjoras
Differential Revision: D27940953
fbshipit-source-id: 5644c1a3da5217365df9de33258bb5b071ff8187
Summary:
This diff hooks the DSR write function into QuicServerTransport's
write path.
Reviewed By: mjoras
Differential Revision: D27890711
fbshipit-source-id: ac4452373c0baafe091f93fb54fccf87be604a9c
Summary: For now let it fallback to ping.
Reviewed By: mjoras
Differential Revision: D27481741
fbshipit-source-id: 874529a06ab882d9651e06f615bc82505c1c2872
Summary:
The cloning scheduler will later bypass all D6D probes anyway. So the
hasData() API should answer False if only thing outstanding is D6D packets.
Reading the code, it's actually not very clear to me if the counter is correct
though. :/ So i left some TODO warning comments to the d6d enabling flag
(Note: this ignores all push blocking failures!)
Reviewed By: mjoras
Differential Revision: D27480434
fbshipit-source-id: 12ddded1b496b92ff5c03125c5283b8c7f3fcdb3
Summary:
Two bugs introduced in the pervious diff that merged write buffer and
loss buffer of a QUIC stream:
(1) We have stopped writing streams' loss buffers if connection is flow control
blocked.
(2) When stream write fails due to running out of packet space, it would be
mistakenly categorized as flow control has ran out. As a result, the loop in
writeStreamsHelper() would continue to try to write loss buffers data, assuming
loss buffer is still OK to write since they don't limit by flow control window.
But because the actually problem is the packet is full, the loss buffer write
would fail anyway. So we wasted some CPU trying to do a write that's destined
to fail.
Reviewed By: mjoras
Differential Revision: D27117026
fbshipit-source-id: db508fa101e04fbe4b5fe21c0c7013b0c0b07936
Summary: they need to be prioritized together
Reviewed By: mjoras
Differential Revision: D26918282
fbshipit-source-id: 061a6135fd7d31280dc4897b00a17371044cee60
Summary: Sending a potentially over-sized d6d packet on PTO might be a bad idea. Plus, a PTO probe containing app data sounds more useful than only "empty" packet.
Reviewed By: yangchi
Differential Revision: D25109633
fbshipit-source-id: ded0c02397b7dc4c346f3aa9b61869836791baec
Summary: As in title. No reason to make a string copy, these always refer to string literals.
Reviewed By: yangchi
Differential Revision: D25288518
fbshipit-source-id: 489f0ac22aa86aa4a4acb562245e98b6d33a10fd
Summary: Adds a top level API to set stream priorities, mirroring what is currently proposed in httpbis. For now, default all streams to the highest urgency, round-robin, which mirrors the current behavior in mvfst.
Reviewed By: mjoras
Differential Revision: D20318260
fbshipit-source-id: eec625e2ab641f7fa6266517776a2ca9798e8f89
Summary: This reduces dependencies for both testing and instrumentation.
Reviewed By: mjoras
Differential Revision: D23997313
fbshipit-source-id: 5eb3a790c7bb2569dc1e941e3911ad4aac4e9258
Summary:
Due to high number of RTT samples I refactored the OutstandingPacket to
split the packet data and packet metrics. We know have access to metrics
without the need of saving the packet data.
Pull Request resolved: https://github.com/facebookincubator/mvfst/pull/178
Reviewed By: mjoras
Differential Revision: D23711641
Pulled By: bschlinker
fbshipit-source-id: 53791f1f6f6e184f37afca991a873af05909fbd2
Summary:
According to the spec v21
(https://tools.ietf.org/id/draft-ietf-tsvwg-datagram-plpmtud-21.html#name-sending-quic-probe-packets),
d6d probe should only contain a PING followed by many PADDING frames. As a first step, we fully comply with the spec.
Future optimizations:
- We can potentially put loss data in the probe packet to perform retransmission. This might make the probe packet more useful by increasing goodput.
Reviewed By: mjoras
Differential Revision: D22557962
fbshipit-source-id: 9a6584bc46aeb29981c4e2c4121ded127a7f2f06
Summary: Implemented necessary changes to emit DATA BLOCKED frame when a QUIC connection is write blocked.
Reviewed By: mjoras
Differential Revision: D23067313
fbshipit-source-id: f80d7425c9a3c4e9b81405716bcd944c83b97ac2
Summary:
It is useful to be able to account for when we detect packets as lost but actually receive an ACK for them later. This accomplishes that by retaining the packets in the outstanding packet list for a certain amount of time (1 PTO).
For all other purposes these packets are ignored.
Reviewed By: yangchi
Differential Revision: D22421662
fbshipit-source-id: 98e3a3750c79e2bcb8fcadcae5207f0c6ffc2179
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:
The AckScheduler right now has two modes: Immediate mode which always
write acks into the current packet, pending mode which only write if
needsToSendAckImmediately is true. The FrameScheduler choose the Immdiate mode
if there are other data to write as well. Otherwise, it chooses the Pending
mode. But if there is no other data to write and needsToSendAckImmediately is
false, the FrameScheduler will end up writing nothing.
This isn't a problem today because to be on the write path, the shouldWriteData
function would make sure we either have non-ack data to write, or
needsToSendAckImmediately is true for a packet number space. But once we allow
packets in Initial and Handshake space to be cloned, we would be on the write
path when there are probe quota. The FrameScheduler's hasData function doesn't
check needsToSendAckImmediately. It will think it has data to write as long as
AckState has changed, but can ends up writing nothing with the Pending ack
mode.
I think given the write looper won't be schedule to loop when there is no
non-ack data to write and needsToSendAckImmediately is true, it's safe to
remove Pending ack mode from AckScheduler.
Reviewed By: mjoras
Differential Revision: D22044741
fbshipit-source-id: 26fcaabdd5c45c1cae12d459ee5924a30936e209
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:
otherwise we keep encoding packet headers and potentially write into
the write buffer without actually generating a packet. It leaves a trail of bad
headers inside the buffer
Reviewed By: mjoras
Differential Revision: D21626733
fbshipit-source-id: 3bdcf6277beccb09b390a590ba2bb0eb8e68e6c1
