net/http: Server panics on nil connection

[jefferai]

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 nil net.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#L2901

This 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-nil net.Conn if err is nil.

What did you expect to see?

A proper nil check.

What did you see instead?

A panic.

[bradfitz]

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.

[bradfitz]

I don't think there's anything to do here. Maybe we could panic with a more helpful string, but I'd still panic.

[jefferai]

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.

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.

[ianlancetaylor]

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.

[bradfitz]

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.

[jefferai]

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.