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
net/http: Server panics on nil connection #37663
Comments
Go only documents unusual cases. If a func returns (T, error), that means by default that exactly 1 of T or error must be non-nil. If both can be non-nil or both can be nil, then we document it. |
I don't think there's anything to do here. Maybe we could panic with a more helpful string, but I'd still panic. |
Can I just ask, where is that in the spec? I haven't read the spec lately, it's been a few years, but I don't remember seeing that and some grepping through the spec right now doesn't pull anything up. |
It's not in the spec. The language permits returning multiple non-zero values. But the convention is that when returning an error the other values are returned as the zero values for their type. Functions that do not follow that convention are expected to document that fact. There is no need to document the fact that a function does follow that convention. |
In fact, it's actively harmful, because then people wonder why some places document it and some don't. We've actually removed redundant documentation in the past because it was causing such confusion elsewhere. |
There's a difference between "this is what I see a lot of places" and "this is a convention that is expected to be followed". If this isn't in the spec, you are relying on the user's memory and/or experience and/or for them to never actually just make mistakes and have bugs (as happened in this case) in order to not trip up the stdlib with an iron-clad but undocumented convention you are expected to follow. Maybe I'm alone here but it feels pretty user-hostile to require developers to just "know convention", especially for a language as generally well-specified as Go. Regardless, I don't even really see what all of that that has to do with this report. My custom listener had a bug and it triggered a panic. Wouldn't it be better to do a nil check and return an error than panic on a nil pointer? Defense in depth and all. |
What version of Go are you using (
go version
)?Go 1.14
Does this issue reproduce with the latest release?
Yes, as well as 1.13.8.
What did you do?
When using a custom
net.Listener
there was a case where a nilnet.Conn
could be returned, along with a nil error. The listen function doesn't check whether or not the connection coming from Accept is nil: https://golang.org/src/net/http/server.go?s=90476:90522#L2901This can lead to a hard to track down panic, because if this happens e.g. on program shutdown whether you actually see the panic or not may be timing-dependent.
I should note that the
net.Listener
documentation does not say that it is required to return a non-nilnet.Conn
if err is nil.What did you expect to see?
A proper nil check.
What did you see instead?
A panic.
The text was updated successfully, but these errors were encountered: