net/http: better error with nil *http.Client #53521
Comments
|
It looks like a pretty normal stack trace to me. Maybe the library you're using should check to see if the Client is nil and return an error or produce their own non-nil Client? Or, perhaps Client should Just Work if it's nil? None of the fields are required. The deadline method could check if the receiver is nil and return zero if so. |
|
cc @neild |
I guess the issue is that it's a more confusing stack trace than "http: Client can't be nil" or similar. I think I am pretty experienced and if it took me ~5 minutes of troubleshooting my heuristic is that it is likely confusing other people as well.
Sure that's fine with me too. |
cagedmantis
added
the
NeedsInvestigation
|
We don't generally test for unexpectedly nil function parameters, including receivers. It would be possible to make a nil http.Client Just Work, but it'd make the implementation more complicated for dubious benefit. The usual expectation is that calling a method with a nil receiver will panic; the cases where a nil receiver is acceptable are less common and generally a special case of some form. |
|
Closing as working as intended |
I found a few places where we do do that: rand/zipf.go: // Uint64 returns a value drawn from the Zipf distribution described
// by the Zipf object.
func (z *Zipf) Uint64() uint64 {
if z == nil {
panic("rand: nil Zipf")
}In os and fs, we return an error for a nil receiver instead of letting people call methods willy nilly func (f *File) Close() error {
if f == nil {
return ErrInvalid
}In text/template, it's important to return a better error message than to just let the code randomly panic Generally one of the reasons that I like Go is because when you make a mistake it's pretty easy to figure out what went wrong. It's sort of frustrating to hear that we can't do anything in this case. Particularly with Could we change the panic error message to direct people to the type that was nil, if we have it? Or make it more obvious that a nil receiver was the issue? |
|
Change https://go.dev/cl/413975 mentions this issue: |
dmitshur
added
NeedsFix
Callers who invoke `*http.Client.Do` with a nil *Client will now panic at the top of c.Do, instead of panicking when `deadline` attempts to read `c.Timeout`. Errors inside of net/http can be difficult to track down because the caller is often invoking the standard library code via an SDK. This can mean that there are many places to check when code panics, and raises the importance of being clear about error messages. If nil receiver calls panic during the `deadline()` call, callers may confuse the error with a more common timeout or deadline misconfiguration, which may lead a caller who passed a nil receiver (the author, for example) down the wrong rabbit hole, or cause them to suspect their timeout/deadline logic. It is less common to configure client.Jar, so the probability of detecting the actual problem, given the underlying error cause, is higher. Fixes #53521. Change-Id: If102d17bed56fdd950da6e87762166fd29724654 Reviewed-on: https://go-review.googlesource.com/c/go/+/413975 Run-TryBot: Emmanuel Odeke <emmanuel@orijtech.com> Reviewed-by: Damien Neil <dneil@google.com> Reviewed-by: David Chase <drchase@google.com> Reviewed-by: Emmanuel Odeke <emmanuel@orijtech.com> Auto-Submit: Emmanuel Odeke <emmanuel@orijtech.com> TryBot-Result: Gopher Robot <gobot@golang.org> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Callers who invoke `*http.Client.Do` with a nil *Client will now panic at the top of c.Do, instead of panicking when `deadline` attempts to read `c.Timeout`. Errors inside of net/http can be difficult to track down because the caller is often invoking the standard library code via an SDK. This can mean that there are many places to check when code panics, and raises the importance of being clear about error messages. If nil receiver calls panic during the `deadline()` call, callers may confuse the error with a more common timeout or deadline misconfiguration, which may lead a caller who passed a nil receiver (the author, for example) down the wrong rabbit hole, or cause them to suspect their timeout/deadline logic. It is less common to configure client.Jar, so the probability of detecting the actual problem, given the underlying error cause, is higher. Fixes golang#53521. Change-Id: If102d17bed56fdd950da6e87762166fd29724654 Reviewed-on: https://go-review.googlesource.com/c/go/+/413975 Run-TryBot: Emmanuel Odeke <emmanuel@orijtech.com> Reviewed-by: Damien Neil <dneil@google.com> Reviewed-by: David Chase <drchase@google.com> Reviewed-by: Emmanuel Odeke <emmanuel@orijtech.com> Auto-Submit: Emmanuel Odeke <emmanuel@orijtech.com> TryBot-Result: Gopher Robot <gobot@golang.org> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Currently, if I try to issue a HTTP request with a nil *http.Client, I get a fairly inscrutable error message - here's a sample from the aws-sdk-go library. My initial thought was the library did something wrong with timeouts.
The actual problem is that we are trying to make a HTTP request with a nil
http.Client, which you can reproduce with the following two lines.What do you think about adding a nil check inside
*Client.do(), and then panicking with a better error message?The text was updated successfully, but these errors were encountered: