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

Close the QUIC connection to avoid retransmissions and icmp unreachable #88

Closed
wants to merge 1 commit into from

Conversation

dmachard
Copy link

This PR fixes #47.

Retransmissions are sent by the remote server to the client, it's why we have ICMP unreachable.

with the fix

image

without the fix

image

@natesales
Copy link
Owner

Thanks for the fix! This looks good, but it's going to break connection reuse (https://github.com/natesales/q/blob/7f6c321cae70f49468c5f9115854504a0f0cfa9e/transport/quic.go#L53C25-L53C34).

return q.connection().CloseWithError(DoQNoError, "")

Close should close the connection, but it seems it isn't actually being called anywhere. Can we keep the QUIC connection open, sending each transaction over a new stream, and close the connection after all queries are complete? i.e. after the main query loop at

q/main.go

Line 408 in 7f6c321

for _, serverStr := range opts.Server {

@dmachard
Copy link
Author

Thanks, I need to fix that.

I take a look to the code but I don't see where the conn is reused. I expect something in the newTransport function but I see nothing.
The connection reuse does not work in my side.

 go run . @quic://127.0.0.1 @quic://127.0.0.1 --tls-insecure-skip-verify www.google.com A --trace
DEBU[0000] Server(s): [quic://127.0.0.1 quic://127.0.0.1]
WARN[0000] TLS secret logging enabled                   
DEBU[0000] Using server 127.0.0.1:853 with transport quic 
DEBU[0000] Using QUIC transport: 127.0.0.1:853          
DEBU[0000] conn=<nil>, reuseconn: true                  
DEBU[0000] No ALPN tokens specified, using default: "doq" 
DEBU[0000] Dialing with QUIC ALPN tokens: [doq]         
DEBU[0000] Using server 127.0.0.1:853 with transport quic 
DEBU[0000] Using QUIC transport: 127.0.0.1:853          
DEBU[0000] conn=<nil>, reuseconn: true                  
DEBU[0000] Dialing with QUIC ALPN tokens: [doq doq-i11] 

I missed something ?

@natesales
Copy link
Owner

Connection reuse is intended for multiple queries to the same server rather than multiple queries to separate transport instances that happen to connect to the same server.

For example, q @quic://dns.adguard.com example.com A AAAA will send both the A and AAAA queries over the same QUIC connection.

Ideally we could just close the transport after all queries are exchanged before printing, but some output formatters make their own queries over the same transport. I've fixed this behavior in b2c9126

Can you retry with HEAD and see how QUIC retransmissions look?

@natesales
Copy link
Owner

@dmachard Can you confirm if this changes anything?

@dmachard
Copy link
Author

Sorry, I missing time. I will take a look and come back to you.

@natesales
Copy link
Owner

@dmachard Any update on this?

@dmachard
Copy link
Author

dmachard commented Oct 9, 2024

@natesales sorry for the delay,
Your fix is OK, I made some test with the HEAD and no more retransmission occurred.

@dmachard dmachard closed this Oct 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DNS over Quic request generate ICMP packets
2 participants