Summary:
Adds stream specific details to `AckEvent` for each ACKed packet, including:
- the number of stream frame bytes newly ACKed
- the number of stream frame bytes newly ACKed by a packet containing a retransmission of said frames (meaning that the original packet was lost, or that the packet with the retransmission arrived at the receiver and corresponding ACK arrived at the sender faster (unlikely), or that the receiver ACK including the original packet was lost, and the ACK interval was pruned too quickly)
- the change in the stream delivery offset for each stream triggered by ACK arrival (if any)
- the stream intervals (and more specifically, the stream frames) that were "dupacked" by the ACK of this packet, meaning that ACK of some other packet had already marked those frames / stream bytes as delivered
Information about "dupacked" packets will not be available until processing of ACKs for packets spuriously marked as lost is enabled; this will come in a follow up diff.
Reviewed By: mjoras
Differential Revision: D31915844
fbshipit-source-id: b8056ea1b5f6093b61e5463c6b4c11cd83bd2916
Summary: As titled. Reduces confusion, as we always expect this to be populated.
Differential Revision: D31887097
fbshipit-source-id: 153b05bae8abd559fe49d2c07c64d2ad0d92a809
Summary: The `AckPacket` stored in each `AckEvent` currently contain a subset of fields extracted from the `OutstandingPacketMetadata` that belonged to the packet that was ACKed. Switch to just storing the entire `OutstandingPacketMetadata`, using `move` to reduce overhead of this operation.
Reviewed By: mjoras
Differential Revision: D31211177
fbshipit-source-id: 08b42ec6e8e1a06648b75b229717b217159f5cfd
Summary:
The util function used to create large packet in
NetworkTestResetLargePacket actually doesn't respect packet len limit
Reviewed By: lnicco
Differential Revision: D28255699
fbshipit-source-id: e4b546625773ec45cd36265ee5c201034e329e67
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 be used in other test cases too
(Note: this ignores all push blocking failures!)
Reviewed By: mjoras
Differential Revision: D27786212
fbshipit-source-id: 2e21073962f20bd69ba1d16ae6d5752c9ee75b74
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:
Previously, we maintained state and counters to count both, header and body
bytes together. This commit introduces additional counters and state to keep
track of just the body bytes that were sent and acked etc. Body bytes received
will be implemented later.
Reviewed By: bschlinker
Differential Revision: D27312049
fbshipit-source-id: 33f169c9168dfda625e86de45df7c00d1897ba7e
Summary:
Previously, we only had support for notifying observers when a QUIC socket
became application rate limited.
This change adds a similar notification when a socket does not become
application rate limited, i.e when the application *starts* writing data to the
socket - either for the first time on a newly established socket or after a
previous block of writes were written and the app had no more to write.
In addition, we include the number of outstanding packets (only those that are
carrying app data in them) so that observers can use this data to timestamp the
start and end of periods where the socket performs app data writes.
Reviewed By: yangchi
Differential Revision: D26559598
fbshipit-source-id: 0a8df7082b83e2ffad9b5addceca29cc03897243
Summary:
- Adds counter of the number of ack-eliciting packets sent; useful for understanding lost / retransmission numbers
- Adds the following to `OutstandingPacketMetadata`
- # of packets sent and # of ack-eliciting packets sent; this enables indexing of each packet when processed by an observer on loss / RTT event, including understanding its ordering in the flow of sent packets.
- # of packets in flight; useful during loss analysis to understand if self-induced congestion could be culprit
- Fixes the inflightBytes counter in `OutstandingPacketMetadata`; it was previously _not_ including the packets own bytes, despite saying that it was.
Right now we are passing multiple integers into the `OutstandingPacket` constructor. I've switched to passing in `LossState` to avoid passing in two more integers, although I will need to come back and clean up the existing ones.
Differential Revision: D25861702
fbshipit-source-id: e34c0edcb136bc1a2a6aeb898ecbb4cf11d0aa2c
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: As in title. Also use the same starting value for each space, since our outstanding packets structure keeps all of them globally ordered.
Reviewed By: yangchi
Differential Revision: D23882335
fbshipit-source-id: 83c35d50a30593d2f596715ba5fc52c2a639ffd6
Summary:
Adding setter for QuicServer to pass down connection ID version.
Also updating hostId setter to uint32 from uint16, I've udpated ServerConnectionIdParams to uint32 earlier, but not server setters.
Reviewed By: udippant
Differential Revision: D23917110
fbshipit-source-id: e3bef08c91b52fccc3ef4b2f3cc6aa67e24c089d
Summary:
Also move encoding/decoding of the AppToken to be transmitted via fizz in its own file.
Pull Request resolved: https://github.com/facebookincubator/mvfst/pull/175
Reviewed By: yangchi
Differential Revision: D23681956
Pulled By: mjoras
fbshipit-source-id: dc98d0b4ba2bee4a05ae8832d36ff4a116cfbd0d
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: 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:
(1) Only read out the token if the parsing host is a client and the
token matches client's token
(2) More fallbacks to Stateless reset when parsing short header packet. The
only exception would be when we don't have 1-rtt cipher.
Reviewed By: mjoras
Differential Revision: D21868631
fbshipit-source-id: 159edf7ab21061ddd5a5ef17f6b18209c3de24e7
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:
Currently the packet builder contructor will encode the packet
builder. This is fine when the builder creates its own output buffer. If later
on we decides not to use this builder, or it fails to build packet, the buffer
will be thrown away. But once the builder uses a buffer provided by caller, and
will be reused, we can no longer just throw it away if we decide not to use
this builder. So we have to delay the header encoding until we know we will use
the builder.
This is still not enough to solve the case where we want to use this builder,
it builds, then it fails . For that, we will need to retreat the tail position
of the IOBuf.
Reviewed By: mjoras
Differential Revision: D21000658
fbshipit-source-id: 4d758b3e260463b17c870618ba68bd4b898a7d4c
Summary: This is useful when you want to ensure that the IOBuf you pass in is encrypted inplace, as opposed to potentially creating a new one.
Reviewed By: yangchi
Differential Revision: D21135253
fbshipit-source-id: 89b6e718fc8da1324685c390c721a564bb77d01d
Summary:
Other frames have been using the BufQueue version of frame writing.
Change this for Crypto streams as well
Reviewed By: mjoras
Differential Revision: D20947921
fbshipit-source-id: 1cfa0f3806e8d74e8c8a4864498e1c06b55d5292
Summary:
As title.
For now the buildPacket() api will still build out separate IOBufs for header and body even though they are just two separate IOBuf wrapping continuous memory. This is so that API in other layers don't have to change for now, and I can reuse all the existing packet builder unit tests for the new builder.
Reviewed By: mjoras
Differential Revision: D20781977
fbshipit-source-id: 64e5ed9fbcff102ca20d3730511b02e6e7426b40
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: 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:
The fizz `encrypt` API will opportunistically use inplace encryption if the `IOBuf` you pass it is correctly sized. If it is not it will manually create the output buffer.
It turns out doing inplace versus encrypting with a given target output is a bit faster in microbenchmarks, and it will also allow experimenting with caching `IOBuf`s on the send path.
Reviewed By: knekritz
Differential Revision: D19506291
fbshipit-source-id: 3ef41290538ceac34a344114badbd167e2c25a50
Summary:
Previously we stored an `IntervalSet` in each `WriteAckFrame`. We don't need to do this, as by the time we are writing an `ACK` the `IntervalSet` is complete. Instead of bothering with the `IntervalSet` operations, we can simply serialize it to a reverse-sorted `vector.`
Additionally this has some micro-optimizations for filling the ACK frame, with a new function for getting varint size.
Reviewed By: yangchi
Differential Revision: D19397728
fbshipit-source-id: ba6958fb36a4681edaa8394b1bcbbec3472e177d
Summary:
This moves the fizz specific part of the handshake into its own folder and library.
There is a bit of smurf naming going on as a result, not sure it is worth fixing or not at this stage. Maybe this code should be a in namespace named quic::fizz .
This should be doable with the client as well as soon as the key cache situation is figured out.
---
## Proxygen Canary
Reviewed By: yangchi
Differential Revision: D19290919
fbshipit-source-id: 48d7f7c70db42c65f7dffe3256805c268a481198
Summary: We'd like to remove this param from our decrypt() api, as it's no longer needed.
Reviewed By: reanimus
Differential Revision: D18855369
fbshipit-source-id: cfe5b3d847918a9ef4a4834df716b79baf0e804a
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: By modifying `IntervalSet` a bit we can make it so it takes a `folly::small_vector` as the container. We expect that for real traffic there will not generally be a lot of ACK blocks per frame, so optimize for that.
Reviewed By: siyengar
Differential Revision: D18919975
fbshipit-source-id: 199a2ea9ba5003382e2d7d99fc7a6de7e8aafdca
Summary:
Don't use IOBufQueue for most operations in mvfst and use BufQueue instead. Since BufQueue did not support a splitAtMost, added it in instead.
The only place that we still use IOBufQueue is in crypto because fizz still requires it
Reviewed By: mjoras
Differential Revision: D18846960
fbshipit-source-id: 4320b7f8614f8d2c75f6de0e6b786d33650e9656
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:
stop carrying OutstandingPacket around for congestion controller
states updating since OutstandingPackets also contains all the sent frames.
Reviewed By: siyengar
Differential Revision: D17914261
fbshipit-source-id: dd16934393bce5cb7be3bae2371e06cbb18e7f4a
Summary:
Bug fix. Since we changed the iteration order of ack processing to be
in reverse order of packet numbers, there has been a bug and both BBR and Cubic
has been using the earliest acked packet instead of the largest acked packet as
the largest acked packet.
Reviewed By: mjoras
Differential Revision: D17909490
fbshipit-source-id: 15310aad53b4a9e5190ed65c3b2df496db77b1ae
Summary: Use the custom variant type for errors.
Reviewed By: yangchi
Differential Revision: D17826935
fbshipit-source-id: 2bf0c3e1cc8ca84b504d201fd6c2a4266878b715
Summary:
They are strongly coupled, which indicate this is probably better to do it as one class.
Pull Request resolved: https://github.com/facebookincubator/mvfst/pull/44
Reviewed By: mjoras
Differential Revision: D17590918
Pulled By: yangchi
fbshipit-source-id: 2eaca079fd760107eefd2b74fa612d7a0c8b3001
Summary:
Use the custom variant type for write frames as well, now that
we use them for read frames.
Reviewed By: mjoras
Differential Revision: D17776862
fbshipit-source-id: 47093146d0f1565c22e5393ed012c70e2e23d279
Summary:
We erase packets from outstandingPackets list during ack handling.
Currently we handle acks in ascending order of the acked packet number, then
erase() the acked packet. Each erase() call only erase one packet, and it can
potentally make std::deque to move a few elements following the erased one.
This diff changes it to handle acks in descending order, and also erase acked
packets in continuous groups. This reduces erase() calls, and also reduces the
possibility that std::deque needs to move following elements in the erase().
Reviewed By: mjoras
Differential Revision: D17579683
fbshipit-source-id: 8bc11f6a7875beb70dc46c497857ab694df7b6a5
Summary: Having access to the state when decrypting tickets gives us more control over ticket acceptance policies.
Reviewed By: knekritz
Differential Revision: D17528945
fbshipit-source-id: a3cb3d4c0917f2494f5669f283cda70776b608c6
Summary:
Make a custom variant type for PacketHeader. By not relying on boost::variant
this reduces the code size of the implementation.
This uses a combination of a union type as well as a enum type to emulate a variant
Reviewed By: yangchi
Differential Revision: D17187589
fbshipit-source-id: 00c2b9b8dd3f3e73af766d84888b13b9d867165a
Summary:
Prior to this diff we would clone out an entire flow control's worth of data from the writebuffer and then clone out a smaller portion of that to write into the packet builder. This is extremely wasteful when we have a large flow control window.
Additionally we would always write the stream data length field even when we are going to fill the remainder of the packet with the current stream frame. By first calculating the amount of data that needs to can be written and writing the header, we can now omit the data length field when we can fill the whole packet.
Reviewed By: yangchi
Differential Revision: D15769514
fbshipit-source-id: 95ac74eebcde87dd06de54405d7f69c42362e29c
Summary:
The CryptoFactory is extended with makePacketNumberCipher . In order to support that feature, FizzCryptoFactory now explicitly takes a QuicFizzFactory as argument instead of a generic fizz::Factory, which is the only type that is used in practice anyways.
The cypher argument was removed because:
1/ Only one cypher is used at all. Fizz also supports ChaCha20, but using it in mvfst will throw an exception.
2/ it seems like the factory should know what cypher it is dealing with.
If a choice of cypher needs to be supported going forward, it can be done by adding state to FizzCryptoFactory.
Pull Request resolved: https://github.com/facebookincubator/mvfst/pull/40
Reviewed By: mjoras
Differential Revision: D16785274
Pulled By: yangchi
fbshipit-source-id: a1c490e34c5ddd107e8e068d8b127c1ed00a59ec
