-
Notifications
You must be signed in to change notification settings - Fork 65
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 support for 0rtt connection resumption to DoH and DoQ listeners. #414
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. Let's make sure that closing the connection in multiple places is actually necessary.
doqlistener.go
Outdated
s.metrics.stream.Add(1) | ||
|
||
// DoQ requires a length prefix, like TCP | ||
var length uint16 | ||
if err := binary.Read(stream, binary.BigEndian, &length); err != nil { | ||
s.metrics.err.Add("read", 1) | ||
log.WithError(err).Error("failed to read query") | ||
CloseConn(connection, log) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a fan of closing the connection in multiple places, including from within the stream. One effect of doing it here will be that connection.AcceptStream(ctx)
above will return an error, which will then call CloseConn
a second time. It should write "closing connection"
twice in the logs too.
The prior implementation only closed it in one place. Is that not working? If a stream fails, I'd expect AcceptStream()
to also fail, which then closes the connection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. I did remove the changes and updated the PR
There is currently still a problem when the server restarts and the client still holds an old session ticket. I think the client should automatically retry without the session ticket after it failed to (re)open the connection. Will work on this now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you
Add support for 0rtt connection resumption to DoH and DoQ listeners.