Conversation
|
Probably ignore the paste advisory for now? aws-lc-rs are on it. |
|
Nice! |
This caused rare flakes in at least single_ack_eliciting_packet_triggers_ack_after_delay by classifying a the connection as active for longer than expected.
|
|
||
| // Retransmit the data of the oldest in-flight packet | ||
| if !self.pending.is_empty(streams) { | ||
| if !self.pending.is_empty(streams) || !datagrams.outgoing.is_empty() { |
There was a problem hiding this comment.
I don't think this is sufficient, because the next user datagram might not be able to fit in the next QUIC packet. And in that case you'd still have to include a ping frame. I guess the bug in this scenario is worse than sending the extra ping frame.
I also think it would be really good to write a test for this.
There was a problem hiding this comment.
Hmm, good catch. I think we actually want exactly the same judgement as Connection::can_send_1rtt here. It looks like that would actually fix another pre-existing bug here, since lazy cleanup of reset streams means self.pending.is_empty doesn't actually necessarily imply the ability to transmit any stream frames.
There was a problem hiding this comment.
TIL, I assumed the stream side of this was correct.
There was a problem hiding this comment.
I'm actually waffling a bit on whether the datagram side is an issue or not. The interaction of TLP queue accounting and size limits is nontrivial. I think I'll need to build some tests to draw empirical conclusions and validate a solution.
There was a problem hiding this comment.
I'm wondering whether the best thing is to revert this change and create an issue for the extra ping. That way we have a correct-but-slightly-wasteful implementation and don't forget to follow up.
There was a problem hiding this comment.
I'm wondering whether the best thing is to revert this change and create an issue for the extra ping. That way we have a correct-but-slightly-wasteful implementation and don't forget to follow up.
3 participants
No description provided.