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

http3 - when closing, http3.Server.Serve returns quic.ErrServerClosed instead of http.ErrServerClosed #3898

Closed
kgersen opened this issue Jun 13, 2023 · 4 comments · Fixed by #3900

Comments

@kgersen
Copy link

kgersen commented Jun 13, 2023

The returned error for http3.Server.Serve(...) is quic.ErrServerClosed in case of a normal shutdown/close.
It should be http.ErrServerClosed instead.
see method serveConn (some return paths already use http.ErrServerClosed).

@marten-seemann
Copy link
Member

Is that incorrect? You can have a closed QUIC server, while the HTTP/3 server is still running.

@kgersen
Copy link
Author

kgersen commented Jun 13, 2023

what is a "QUIC server" when working at HTTP/3 api level ? Do we need to know about such 'quic' stuff at this level ?
The http3 package tries to reproduce the same API as the net/http package.

With net/http, when we use ListenAndServeTLS or just ServeTLS and the call ends, it means the server stopped. As per the documentation:

ServeTLS always returns a non-nil error. After Shutdown or Close, the returned error is ErrServerClosed.
source: https://pkg.go.dev/net/http#Server.ServeTLS

So when http3.Server.Serve(...) ends, if the returned error is quic.ErrServerClosed what is the meaning of this ?

If we have existing code that use net/http and check for http.ErrServerClosed, when we switch or add http/3 we must add a special case to check for quic.ErrServerClosed which is confusing (which is what we're currently doing now).

@marten-seemann
Copy link
Member

Should we replace the error if s.closed == true?

@kgersen
Copy link
Author

kgersen commented Jun 13, 2023

Should we replace the error if s.closed == true?

no that's the only place where it is correct.

other return paths should be changed.

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

Successfully merging a pull request may close this issue.

2 participants