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: Client panics if RoundTripper returns neither response nor error #33563

Closed
Teelevision opened this issue Aug 9, 2019 · 2 comments
Closed

Comments

@Teelevision
Copy link

@Teelevision Teelevision commented Aug 9, 2019

What did you do?

Use a http.RoundTripper that returns neither response nor error.

https://play.golang.org/p/EGEjS2P2xQI

What did you expect to see?

Documentation that the RoundTripper MUST return either response or error.

What did you see instead?

go/src/net/http/client.go

Lines 119 to 125 in 1dc0110

// RoundTrip should not attempt to interpret the response. In
// particular, RoundTrip must return err == nil if it obtained
// a response, regardless of the response's HTTP status code.
// A non-nil err should be reserved for failure to obtain a
// response. Similarly, RoundTrip should not attempt to
// handle higher-level protocol details such as redirects,
// authentication, or cookies.

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0xffffffff addr=0x0 pc=0x24bc42]

goroutine 1 [running]:
net/http.redirectBehavior(0x311a6b, 0x3, 0x0, 0x84c510, 0x0, 0x0, 0x0, 0x0)
	/usr/local/go/src/net/http/client.go:422 +0x22
net/http.(*Client).do(0x840940, 0x84c510, 0x0, 0x0, 0x0, 0x0)
	/usr/local/go/src/net/http/client.go:655 +0x3a0
net/http.(*Client).Do(...)
	/usr/local/go/src/net/http/client.go:509
net/http.(*Client).Get(0x840940, 0x3122b2, 0x7, 0x5503, 0x0, 0x0, 0x0, 0x5503)
	/usr/local/go/src/net/http/client.go:398 +0xa0
main.main()
	/tmp/sandbox656600978/prog.go:15 +0xc0

Program exited: status 2.
@odeke-em

This comment has been minimized.

Copy link
Member

@odeke-em odeke-em commented Aug 10, 2019

Hello @Teelevision, thank you for filing this issue and welcome to the Go project!

So my apologies if am preaching to the choir; in Go, the idiomatic way of communicating
between functions is by returning values as well as error as per (values..., error). You'll commonly see in Go code handling for which:

  1. A non-nil error signals a non-normal flow/state of the program and requires a form of handling
  2. If no error was encountered, the value is then assumed to be okay and can be dereferenced/used

if we have the case of (*http.Response)(nil), (error)(nil) both being nil, we are in an invalid state where the expectation of a valid *http.Response is no longer fulfilled. It would break from the norm to expect that every function handle both error == nil and values == nil

Right above the lines that you quoted, we've got

go/src/net/http/client.go

Lines 115 to 117 in 9c1f14f

type RoundTripper interface {
// RoundTrip executes a single HTTP transaction, returning
// a Response for the provided Request.

which says that RoundTrip returns a Response.

Also I believe this doc already says what you'd like it to report

go/src/net/http/client.go

Lines 120 to 123 in 9c1f14f

// particular, RoundTrip must return err == nil if it obtained
// a response, regardless of the response's HTTP status code.
// A non-nil err should be reserved for failure to obtain a
// response. Similarly, RoundTrip should not attempt to

Screen Shot 2019-08-09 at 11 08 33 PM

and implicitly a nil Response can't have a status code.

What do you think?

@Teelevision

This comment has been minimized.

Copy link
Author

@Teelevision Teelevision commented Aug 12, 2019

Ok, thank you, this clarifies it for me.

threez added a commit to pace/bricks that referenced this issue Aug 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.