Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add a PING frame, when acknowledging packets during the handshake #3851

Open
marten-seemann opened this issue May 22, 2023 · 6 comments
Open

Comments

@marten-seemann
Copy link
Member

When acknowledging packets during the handshake, we should add a PING frame to the packet if otherwise we'd just send an ACK frame. This commonly happens when the ClientHello is split across two packets, and one of them is lost.

This will allow the server to generate an RTT sample, speeding up the rest of the handshake.

@marten-seemann
Copy link
Member Author

@SahibYar You were asking for good first issue you could pick up. Want to take a look at this one?
Let me know if you need any pointers.

@SahibYar
Copy link

Sure, thank you @marten-seemann, let me see this.

@SahibYar
Copy link

SahibYar commented Jun 27, 2023

@marten-seemann Adding these lines inside handleAckFrame() in connection.go will be sufficient ?

if s.perspective == protocol.PerspectiveClient && !s.handshakeConfirmed {
   s.handleHandshakeConfirmed()
   
   // See https://github.com/quic-go/quic-go/issues/3851 for details
   // This is not strictly necessary, but it speeds up the handshake.
   s.queueControlFrame(&wire.PingFrame{})
}

@marten-seemann
Copy link
Member Author

handleAckFrame is called when we receive an ACK frame. This needs to be part of the packet packer, probably somewhere in the PackCoalescedPacket code path.

SahibYar added a commit to SahibYar/quic-go that referenced this issue Jul 1, 2023
SahibYar added a commit to SahibYar/quic-go that referenced this issue Jul 1, 2023
SahibYar added a commit to SahibYar/quic-go that referenced this issue Jul 2, 2023
SahibYar added a commit to SahibYar/quic-go that referenced this issue Jul 2, 2023
SahibYar added a commit to SahibYar/quic-go that referenced this issue Jul 3, 2023
SahibYar added a commit to SahibYar/quic-go that referenced this issue Jul 3, 2023
@SahibYar
Copy link

@marten-seemann you got the chance to review this PR ?

@marten-seemann
Copy link
Member Author

Sorry, pretty busy right now. I haven't forgotten about it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants