Repository navigation
Conversation
e424ed6 to
d656743
Compare
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
|
Review: 5 findings. The PR adds configurable ACK-progress timeouts and sent-packet limits across Rust, C, and tokio APIs, with migration and timeout tests. Five inline review comments were posted. |
e89f123 to
e8b3a37
Compare
|
/bonk |
|
Review: 5 findings. Adds configurable ACK-progress timeout and sent-packet limits across Rust, C, and tokio APIs, with migration and timeout coverage. Five inline review comments were posted. |
e8b3a37 to
f62db79
Compare
|
/bonk |
| // Use 64-bit arithmetic, as the product can overflow `usize` | ||
| // on 32-bit platforms. | ||
| let snd_cnt = (self.prr_delivered as u64) | ||
| .saturating_mul(ssthresh as u64) | ||
| .div_ceil(self.recoverfs as u64); | ||
|
|
||
| usize::try_from(snd_cnt) | ||
| .unwrap_or(usize::MAX) |
There was a problem hiding this comment.
Small pre-existing issue that showed up in one of the new test.
|
Since last review: 6 resolved, 4 still open, 4 new. Adds configurable ACK-progress and sent-packet limits across Rust, C, and tokio APIs, with extensive tests. Four new inline findings were posted. |
f62db79 to
9b586fe
Compare
|
/bonk |
|
Review: 2 findings. Adds configurable ACK-progress timeout and outstanding sent-packet limits across Rust, C, TLS callbacks, and tokio settings, with migration and recovery coverage. Two inline correctness comments were posted. Relevant tests passed. |
| // The default number of sent packets that can be tracked for loss detection | ||
| // before the connection is closed. The limit is raised to twice the largest | ||
| // congestion window, so that connections making progress are not affected. | ||
| const DEFAULT_MAX_OUTSTANDING_SENT_PACKETS: usize = 40_000; |
There was a problem hiding this comment.
nit: I don't think it needs to be twice the max cwnd. 1.2 -- 1.5 is probably plenty (and IIRC google quic has 10k as default if not hardcoded)
There was a problem hiding this comment.
One issue is that cubic doesn't have a max cwnd. Since this is the default and will be applied to all users I prefered to be on the safe side, we can always set a lower limits via configuration.
5002ca5 to
0b5325f
Compare
|
/bonk |
|
Review: 2 findings. Adds configurable ACK-progress timeouts and outstanding sent-packet limits across Rust, C, TLS callbacks, and tokio settings, with multipath/recovery tests. Two inline correctness comments were posted. |
48e4ec4 to
2883234
Compare
|
/bonk |
|
Review: 1 findings. Adds configurable ACK-progress timeout and sent-packet limits across Rust, C, TLS, and tokio APIs, with recovery and multipath tests. One inline correctness comment was posted. |
2883234 to
81a71f2
Compare
|
/bonk |
|
Review: 2 findings. Adds configurable ACK-progress and outstanding-packet limits across Rust, C, TLS callbacks, and tokio settings, with recovery and migration coverage. Two inline test-flakiness findings were posted. |
81a71f2 to
a995b7c
Compare
|
/bonk |
|
LGTM! |
Only took 8 reviews! |
ghedo
added
type: bugfix
Close connections whose packets in flight are not acknowledged for too long, or that track too many sent packets. Both limits are configurable.
a995b7c to
d8d9c60
Compare
This branch has not been deployed
4 participants
Close connections whose packets in flight are not acknowledged for too long, or that track too many sent packets. Both limits are configurable.