[fix] should not break directly

[ylht]

We should invoke the take function for sent_frames, which is the same as in lines 709 and 753.

[djc]

Thanks! We should probably have a regression test for this...

[ylht]

I realized that the error is not coming from func take, this line is correct. The error comes from the fact that we shouldn't just break when not padding, otherwise re-instantiating let builder = builder_storage.get_or_insert(PacketBuilder::new( on line 783 won't work. This will cause that we use the last builder when going to the logic below // Finish the last packet on line 924.

[ylht]

It's hard for me to provide a regression test for my PR, but it does solve my problem temporarily.

[djc]

I'm going to wait for a review from @Ralith here, who touched this code last.

It would be helpful if you could explain in your PR description or in an issue what the symptoms are that led you to this fix. (For example, is this the same as or different from #1850?)

[ylht]

It's not clear what this change is intended to accomplish, but its effect is solely to disable intended behavior. Refer to the comments on MAX_PADDING just above your change. Perhaps you could start by describing the problem you're having?

Thanks for your contribution, I use Quinn to implement a private proxy. Recently, I update Quinn to 0.11.0, but I cannot pass the test of speedtest.net. It reports the error stream reset by peer: error 0.

I test your recent commits one by one using binary search (utilizing quinn={git="...", rev="..."}). I find that the first commit came from d5fc528930c0761215fcad13a7797e9904ae4b6a. I attempt to review this commit and make a PR based on my understanding.

[Ralith]

Recently, I update Quinn to 0.11.0, but I cannot pass the test of speedtest.net. It reports the error stream reset by peer: error 0.

I don't know what "the test of speedtest.net" is. An unexpected "stream reset by peer" error is almost certainly an application logic error. You should investigate why the sender is deciding to issue a reset.

I find that the first commit came from https://github.com/quinn-rs/quinn/commit/d5fc528930c0761215fcad13a7797e9904ae4b6a

This is most likely a coincidence arising from your application logic being timing-sensitive.

[ylht]

Yes, you are right. I try to check again.

[ylht]

Oh~~, I locate the bug, but I cannot fix it.

I find the unit test gso_truncation in this commit https://github.com/quinn-rs/quinn/commit/d5fc528930c0761215fcad13a7797e9904ae4b6a is wrong. In fact, it does not really pass.

Specifically, I run the command cargo test --color=always --lib tests::gso_truncation -- --show-output to observe its output log. The client cannot decrypt the second packet, i.e., server: quinn_proto::connection: failed to authenticate packet.

2024-05-08T12:56:36.427721Z TRACE server: quinn_proto::connection: got Data packet (1053 bytes) from [::1]:44433 using id 4a9a80b04ca84a27 2024-05-08T12:56:36.427744Z TRACE server:recv{space=Data pn=10}: quinn_proto::connection: got datagram frame len=1024 2024-05-08T12:56:36.427761Z TRACE server: quinn_proto::connection: got Data packet (781 bytes) from [::1]:44433 using id 4a9a80b04ca84a27 2024-05-08T12:56:36.427776Z TRACE server: quinn_proto::connection::packet_crypto: decryption failed with packet number 10883353 2024-05-08T12:56:36.427783Z DEBUG server: quinn_proto::connection: failed to authenticate packet 2024-05-08T12:56:36.427790Z TRACE server: quinn_proto::connection: got Data packet (797 bytes) from [::1]:44433 using id 4a9a80b04ca84a27 2024-05-08T12:56:36.427810Z TRACE server:recv{space=Data pn=12}: quinn_proto::connection: got datagram frame len=768

After I delete the following code in the commit https://github.com/quinn-rs/quinn/commit/d5fc528930c0761215fcad13a7797e9904ae4b6a. The unit test gso_truncation exactly pass. However, I cannot expose the deep reason. Please help me.

                        let packet_len_unpadded = cmp::max(builder.min_size, buf.len())
                            - datagram_start
                            + builder.tag_len;
                        if packet_len_unpadded + MAX_PADDING < segment_size {
                            trace!(
                                "GSO truncated by demand for {} padding bytes",
                                segment_size - packet_len_unpadded
                            );
                            break;
                        }

2024-05-08T12:59:08.856743Z TRACE server: quinn_proto::connection: got Data packet (1053 bytes) from [::1]:44433 using id 8c32e49c8d5f276a 2024-05-08T12:59:08.856789Z TRACE server:recv{space=Data pn=10}: quinn_proto::connection: got datagram frame len=1024 2024-05-08T12:59:08.856824Z TRACE server: quinn_proto::connection: got Data packet (1053 bytes) from [::1]:44433 using id 8c32e49c8d5f276a 2024-05-08T12:59:08.856867Z TRACE server:recv{space=Data pn=11}: quinn_proto::connection: got datagram frame len=768 2024-05-08T12:59:08.856990Z TRACE server: quinn_proto::connection: got Data packet (797 bytes) from [::1]:44433 using id 8c32e49c8d5f276a 2024-05-08T12:59:08.857034Z TRACE server:recv{space=Data pn=12}: quinn_proto::connection: got datagram frame len=768

[ylht]

Please check the unit test further. @Ralith

[ylht]

I have fixed the unit test, and open a new PR #1853.