Previously, we limited the number of streams that could be opened by
the peer but not `accept`ed by the application. However, to guarantee
bounded resource use, applications will typically want to limit the
number of streams they process concurrently. While this could
be *approximately* implemented at the application layer by controlling
calls to accept, that approach has a significant drawback: If streams
are slow to process, the worst-case per-stream latency observed by a
peer who opens the maximum number of streams can be arbitrarily bad,
because streams may be opened above the limit the local application is
willing to process. Reducing the number of unaccepted streams
tolerated can reduce the proportion of streams affected, but reducing
it too low will increase the number of round trips required to open
any given number of streams, increasing average latency significantly.
As a side benefit, this reduces the effort needed for applications to
limit concurrency to a fixed quantity, which is expected to be the
overwhelmingly common case. Should a use case for dynamic concurrency
limits arise, we can expose a setter.
The pacer's behavior currently makes the library extremely inefficient.
Each `Pacer::delay` call would only allow a single
datagram to be sent, and then instruct the connection to wait a tiny
time-slice (which is far smaller than the timer granularity). When the timer
really elapses (e.g. a tokio timer after 1ms) only 1 packet can be sent
again.
This change improves on this by deriving the pacer capacity based on
how big bursts should be, and how much delay we want to have between
those. A pacer delay bigger than timer granularity is desirable for
efficiency and performance reasons. Here 2ms had been chosen, which
had proven effective in tests with an injected RTT.
The pacing implementation did so far not do anything. The reason for
that is the timestamp when tokens had been last generated was never
updated. Therefore each Pacer::delay call where not enough tokens
had been available calculated tokens based on the time back to when
the Pacer was initially created, which fully refills the capacity.
This is easily fixed by storing the timestamp when tokens are replenished.
The variable `defragmented` wasn't properly updated in the
`read_chunks` method, since it directly manipulated the map instead
of delegating to the `Assembler::pop` method. This caused an underflow
when trying to insert more data into the map.
This change fixes that, and adds a repro test for it.
Fixes#982
During testing with a multithreaded runtime I discovered that threads
block a fair amount of time on Mutexes, even though everything in the
state machine should be non-blocking.
By inserting some more timing checks I discovered that the `poll_write`
call took more than 10ms and locked the Mutex for this duration. This
happened due to the `BytesMut::extend_from_slice` taking this time.
The reason here is that extending the buffer require a reallocation of
the buffer and a copy of the complete data. This can be a big size
(up the maximum internal buffer size) - even if only a tiny chunk of
data is added.
This change improves on this by using a non-contiguous send buffer.
New data is appended as a new `Bytes` segment, which only requires
an allocation for this particular size. The 10ms blocking problem is
gone with this.
Another benefit of this change is that it can easily enable zero-copy
writes by accepting `Bytes` as a parameter in the user-facing API.
This is however not performed here yet.
```
Jan 18 11:14:00.064 WARN quinn_proto::connection::send_buffer: self.unacked.extend_from_slice(data[..1048576]) took 10.3452ms. Now unacked size: 94924462
Jan 18 11:14:00.064 WARN quinn_proto::connection::streams: Long Send::write. Total: 10.3943ms, Budget check: 0ns, data_len: 1048576, written: 1048576
Jan 18 11:14:00.064 WARN quinn_proto::connection::streams: Long Streams::write. Total: 10.4203ms, Limit check: 100ns, Unsent check: 200ns, Write: 10.4202ms. data_len: 1048576, written: 1048576
Jan 18 11:14:00.064 WARN quinn::streams: Long poll_write time: close_check 100ns, write_time 10.4456ms, end_time 10.4456ms
```
If an a range is acknowledged where a part of it had been previously
acknowledged - ignore the acknowledged range. While the
retransmission logic guarantees this property at the moment, it
might not always hold true.
This change fixes 2 issues in the congestion controller:
Extra slow slow start
===
The congestion controller had an issue where it didn't ramp up by
doubling the window in the slow start phase as expected. The reason
here was the usage of the "congestion_blocked" field, which aims to
remove the avoid growing the window when there is no data to send.
The issue with the field was that the first incoming ACK for every batch
would increase the congestion window, which would let the next
call to `congestion_blocked()` return false, and thereby let the
congestion controller ignore all of the following ACKs in a received
batch up the point where the next `poll_transmit()` call is made and
the window is filled again. Since often all ACKs for one round-trip
arrive in a single batch that means only the first ACK had an effect
and the others where ignored -> which lead to increasing the
window by 1 MTU instead of doubling it in the slow start phase.
The new approach introduces a new `app_limited` field which
caches whether the last `poll_transmit` attempt couldn't produce
data because there was no application data available.
Numeric precision issue in congestion avoidance
===
The formula
```
self.window += self.config.max_datagram_size * bytes / self.window;
```
which is specified in the RFC requires floating point arithmetic.
If integer arithmetic is used, the multiplication of the first 2 values
yields about 1.7MB for 1300 byte packets. As soon as the window is
bigger than this value, the window won't change at all due the
division leading to 0.
The new implementation follow this guidance from the quic recovery
specification:
> In congestion avoidance, implementers that use an integer
> representation for congestion_window should be careful with division,
> and can use the alternative approach suggested in Section 2.1 of
> [RFC3465].
This is an alternative version of #972, where we add the new stats
to ConnectionStats instead of exposing additional accessors.
The naming of `recovery` could probably be improved. `path` might
be an option. But where would we add something like packet loss?
That seems more like an overall stat.
This is a follow-up for #943. When a socket is bound to a wildcard
IP address, sending the outgoing IP might use a different source IP
address than the one the packet was received on, since the OS might
not be able to identify the necessary route. This would lead packets
not allowing to reach the client.
This change adds a setting which will set an explicit source address
in all outgoing packets. The source address which will be used is
the local IP address which was used to receive the initial incoming
packet.
Defends against amplification attacks where an existing connection is
hijacked by modifying the source address of a legitimate packet, as
required by the spec. Previously we only defended against similar
attacks involving brand new connections being created by an attacker.
Previously, we checked against a quantity that might not have actually
been communicated to the peer yet, which would cause us to tolerate
illegally optimistic peers.
Dirkjan Ochtman