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

net/http: better error with nil *http.Client #53521

Closed
kevinburkesegment opened this issue Jun 23, 2022 · 7 comments
Closed

net/http: better error with nil *http.Client #53521

kevinburkesegment opened this issue Jun 23, 2022 · 7 comments
Labels
NeedsInvestigation
Milestone

Comments

@kevinburkesegment
Copy link

@kevinburkesegment kevinburkesegment commented Jun 23, 2022

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.

Error
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x28 pc=0x662c54]

goroutine 35 [running]:
net/http.(*Client).deadline(0xb75780?)
	/usr/local/go/src/net/http/client.go:189 +0x14
net/http.(*Client).do(0x0, 0xc000440300)
	/usr/local/go/src/net/http/client.go:611 +0x225
net/http.(*Client).Do(...)
	/usr/local/go/src/net/http/client.go:593
github.com/aws/aws-sdk-go/aws/corehandlers.sendFollowRedirects(0xc0a54e5cabd8ad81?)
	/src/vendor/github.com/aws/aws-sdk-go/aws/corehandlers/handlers.go:120 +0x27
github.com/aws/aws-sdk-go/aws/corehandlers.glob..func3(0xc000442500)
	/src/vendor/github.com/aws/aws-sdk-go/aws/corehandlers/handlers.go:112 +0x176 

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.

	var client *http.Client
	client.Get("http://jsonip.com")

What do you think about adding a nil check inside *Client.do(), and then panicking with a better error message?

@ericlagergren
Copy link
Contributor

@ericlagergren ericlagergren commented Jun 23, 2022

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.

@seankhliao
Copy link
Contributor

@seankhliao seankhliao commented Jun 23, 2022

cc @neild

@kevinburke
Copy link
Contributor

@kevinburke kevinburke commented Jun 23, 2022

It looks like a pretty normal stack trace to me

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.

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.

Sure that's fine with me too.

@cagedmantis cagedmantis added the NeedsInvestigation label Jun 23, 2022
@cagedmantis cagedmantis added this to the Backlog milestone Jun 23, 2022
@neild
Copy link
Contributor

@neild neild commented Jun 24, 2022

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.

@seankhliao
Copy link
Contributor

@seankhliao seankhliao commented Jun 24, 2022

Closing as working as intended

@seankhliao seankhliao closed this as not planned Jun 24, 2022
@kevinburke
Copy link
Contributor

@kevinburke kevinburke commented Jun 24, 2022

We don't generally test for unexpectedly nil function parameters, including receivers.

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

	if receiver.Kind() == reflect.Interface && isNil {
		// Calling a method on a nil interface can't work. The
		// MethodByName method call below would panic.
		s.errorf("nil pointer evaluating %s.%s", typ, fieldName)
		return zero
	}

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 *http.Client, since basically every call goes through do(), it makes it pretty easy to add the error check there.

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?

@gopherbot
Copy link

@gopherbot gopherbot commented Jun 24, 2022

Change https://go.dev/cl/413975 mentions this issue: net/http: change variable initialization order

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

No branches or pull requests

7 participants