Summary:
This test tries to construct a bad packet by using server CID as the
dest CID in a server to client packet. It hopes to verify client will fail
parsing and throw. It used to work fine.
Then a couple month ago, our client code switched to use 0-len client CID. Now
client always read 0 bytes out as Dest CID on received packet. But in this test
case server writes a >0 len CID into the packet as Dest CID since the test case
tries to construct a bad packet. Then client treats these bytes as frame bytes
since it assumes 0 len DestCID. Then it parse frames from there. Now sometimes
parsing fails and we throw from parsing. Sometimes parsing accidentally goes
through, then we may or may not throwing when we process the parsed frames.
For example, if we parse out a stream frame successfully with a local stream ID
that's not open, we throw from there. But sometimes, nothing gets throw, since
we can potentially, for example, parse out a stream frame with a remote stream
ID and just simply create it. Sometime the test just hangs, since the stream ID
is huge and we create thousands of streams.
It's hard to fix this test case and keep it unflaky. For example, I figured by
using (0, 5, 3) instead of (0, 0, 0) as the CID encoding params, the test always
passes. That's because the server generates a CID that will definitely fail
parsing on client side. But if some day draft changes the frame type ID values
again, the test will fail again. And someone else will have to go through this
debugging process all over again.
I think we can just remove this test case altogether.
Reviewed By: mjoras
Differential Revision: D19268239
fbshipit-source-id: 66930d6e9044c863a26e29be1dbad683abc6c96c
Summary:
In QuicExceptions, in the case where the toString method was able to statically determine the response strings, we simply return the string literals in a folly::StringPiece instead of unnecessarily copying them into std::string.
Some toString methods had some dynamically generated responses and thus could not be updated. Added a TODO explaining the fact.
Reviewed By: mjoras
Differential Revision: D19192117
fbshipit-source-id: d9e5f202f9bf240009e8b8fd16f337b0506fbeb0
Summary:
Revert back to using getReadBUffer for reading from the udp socket.
There are a few issues we found while testing, so reverting for now, we can fix them later.
Reviewed By: yangchi
Differential Revision: D19055438
fbshipit-source-id: 2288e94e0aeab95a5cc6ade39f744b5d98062281
Summary:
That ensure the connect API from ClientHandshake doesn't depend on fizz specific things anymore.
Pull Request resolved: https://github.com/facebookincubator/mvfst/pull/76
Reviewed By: yangchi
Differential Revision: D18888115
Pulled By: mjoras
fbshipit-source-id: 00103d629708796b73787b3dabb6f8d3815ff976
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:
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:
Use the new recvmsg api on the client to receive a packet
from AsyncUDPSocket
Reviewed By: mjoras
Differential Revision: D18797963
fbshipit-source-id: 319d5c41f3a868e7b78947fdbcf2c411b6d7fbf0
Summary: As a part of Draft 17, application close frame has been removed, we use connection close frame to represent both application close and connection close.
Reviewed By: mjoras
Differential Revision: D18580856
fbshipit-source-id: d274fa2d3dbc59b926bca5a2b8a20328ae582703
Summary:
An endpoint receiving a PathChallenge will throw a
TransportException if they don't have have enough remaining
peer connection ids.
* Update PathChallenge tests
Reviewed By: JunqiWang
Differential Revision: D18600143
fbshipit-source-id: 933d62d022912754d2829cd6ff8a29bf7e57e82f
Summary:
Allows both client/server to update its peer connection id based on available ids.
This method returns false if there are no available ids.
Reviewed By: JunqiWang
Differential Revision: D18575129
fbshipit-source-id: 0e6969354ee9941d44f6533c003546955e11d098
Summary:
Per
[d-24](https://tools.ietf.org/html/draft-ietf-quic-transport-24#section-19.15)
> An endpoint that is sending packets with a zero-length Destination
> Connection ID MUST treat receipt of a NEW_CONNECTION_ID frame as a
> connection error of type PROTOCOL_VIOLATION.
Reviewed By: JunqiWang
Differential Revision: D18619942
fbshipit-source-id: 9f453c8fb3ecdbc694813613006d1e88198cd4d2
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:
Starting to migrate fizz specific features to the fizz specific handshake class.
Pull Request resolved: https://github.com/facebookincubator/mvfst/pull/65
Reviewed By: siyengar
Differential Revision: D18575268
Pulled By: mjoras
fbshipit-source-id: dc1a2f1705e28e1a7f857d9b026c8f15d735c455
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: There's no reason to make this a huge value to check correctness.
Reviewed By: lnicco
Differential Revision: D18573251
fbshipit-source-id: 4c651c0890868c0ed36e510122205398f38b211e
Summary:
This will allow to be able to create different kind of handshake going forward.
Pull Request resolved: https://github.com/facebookincubator/mvfst/pull/59
Reviewed By: siyengar
Differential Revision: D18088574
Pulled By: mjoras
fbshipit-source-id: 0732bb63a9e243fef77cdaf4f76e711fcb09ecdc
Summary: Enable pacing when CC is changed to BBR
Reviewed By: yangchi
Differential Revision: D18231888
fbshipit-source-id: a54b6313d089c2ae24ce6bf6ee56c4fe0d6b4722
Summary:
I think not doing this, and throw an error at the point of output to
files is kinda surprising to users
Reviewed By: sharma95
Differential Revision: D18016183
fbshipit-source-id: b16881b365ea82b75842f89dc364054804808116
Summary:
Mainly based on Junqi's changes in D15230415
Added the start of several error checking. Section 19.15
of d-23
(https://tools.ietf.org/html/draft-ietf-quic-transport-23#section-19.15)
specifies 2 cases where PROTOCOL_VIOLATION should be thrown during processing:
* retire_prior_to is larger than sequence number
* new_connection_id is sent by peer using 0-len conn id (will handle later)
I don't think it was specified what happens if we get a NEW_CONN_ID frame
after the the endpoint already has enough of the peer's conn ids - just ignore
the frame.
Reviewed By: sharma95
Differential Revision: D17578167
fbshipit-source-id: 6e410b6110970a6e52970576ac3ff2b1c7d5c06a
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: Use the custom variant type for errors.
Reviewed By: yangchi
Differential Revision: D17826935
fbshipit-source-id: 2bf0c3e1cc8ca84b504d201fd6c2a4266878b715
Summary:
This will treat incoming ping frames as retransmittable frames and
update ack state accordingly.
Reviewed By: siyengar
Differential Revision: D17832707
fbshipit-source-id: 69e4a95a62bf86a707bac94399588df61c947bd3
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:
It looks like under some compiler flags, type alias follows a very
different rule of name shadowing. This diff adds a trailing "_E" to generated
enum names to unbreak builds. I will send another diff to make it less ugly
later. And eventually the QuicSimpleFrame won't be a type alias once it's no
longer a boost_variant.
Reviewed By: siyengar
Differential Revision: D17766330
fbshipit-source-id: 7b3c5847fd2c1eae10757bfbf9558a38f3085f10
Summary:
Create a custom type for read frame types. This allows us to reduce size of code.
We use a macro to generate new variant types whenever we need to.
Reviewed By: yangchi
Differential Revision: D17266468
fbshipit-source-id: 59a1183dce728e71f0924f39f95a7b78449642b0
Summary: remove the variant for codec result and replace it with a custom variant type
Reviewed By: yangchi
Differential Revision: D17247099
fbshipit-source-id: 19e24c14732eb6e8496aee7064f20c48bdf254e0
Summary:
Allow the client to set zero-length id for itself.
If enabled, saves 8 bytes (if packet isn't padded to minimum).
However, will be disabled if connection migration is enabled.
Main changes include updating the short header parsing to pass
in the connection id length, since unlike long headers,
short header's dst conn id isn't preceeded by length.
Created a new `QuicClientTransportAfterStartTestBase` class
to move `testing::WithParamInterface<>` inheritance to subclasses.
(Only one parameter per test class).
Reviewed By: JunqiWang
Differential Revision: D17188777
fbshipit-source-id: f60a7f3c07da1a8c83cec5b518075d23daedbe44
Summary:
Remove the variant type for RegularQuicPacket and VersionNegotiationPacket.
This allows us to move version negotiation parsing to be only used on the client and only done explicitly
before a version is negotiated.
Reviewed By: yangchi
Differential Revision: D17242788
fbshipit-source-id: 502caf6849f0b7e6778f1470dc160d01f17a33af
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:
Implement sending stream limit updates in a windowed fashion, so that as a peer exhausts its streams we will grant it additional credit. This is implemented by having the stream manager check if an update is needed on removing streams, and the api layer potentially sending an update after it initiates the check for closed streams.
This also makes some driveby changes to use `std::lower_bound` instead of `std::find` for the sorted collections in the stream manager.
Reviewed By: yangchi
Differential Revision: D16808229
fbshipit-source-id: f6e3460d43e4d165e362164be00c0cec27cf1e79
Summary: the timer may fire during handshake, which makes the test flaky
Reviewed By: mjoras
Differential Revision: D17206449
fbshipit-source-id: 5566b843d0df4ad27eea1ca6b9983f814e53b7cd
Summary: When happy eyeballs timer fires, declare all outstanding 0-RTT data as lost, so that they will be retransmitted to the second socket
Reviewed By: siyengar
Differential Revision: D17124225
fbshipit-source-id: af2f45f11a5f6eb567f626d577db2634693f67ab
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:
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
Summary:
Wrap the fizz::Aead as soon as fizz and it over to mvfst and use a quic::Aead everywhere else in ClientHandshake.
Pull Request resolved: https://github.com/facebookincubator/mvfst/pull/34
Reviewed By: yangchi
Differential Revision: D16710812
Pulled By: mjoras
fbshipit-source-id: 9e6e342205367f84fa4dad6847db0207de245f89
