rickbassham
commented
4 years ago Thanks for the catch!
Closed iwvelando closed 4 years ago
rickbassham
commented
4 years ago Thanks for the catch!
iwvelando
commented
4 years ago @rickbassham will definitely squash commits when ready, thanks for the reminder there.
Regarding checking for a nil body, I checked the docs and I'm not certain that's a possibility:
$ go doc net/http.Response.Body
package http // import "net/http"
type Response struct {
// Body represents the response body.
//
// The response body is streamed on demand as the Body field is read. If the
// network connection fails or the server terminates the response, Body.Read
// calls return an error.
//
// The http Client and Transport guarantee that Body is always non-nil, even on <-----------------
// responses without a body or responses with a zero-length body. It is the
// caller's responsibility to close Body. The default HTTP client's Transport
// may not reuse HTTP/1.x "keep-alive" TCP connections if the Body is not read
// to completion and closed.
//
// The Body is automatically dechunked if the server replied with a "chunked"
// Transfer-Encoding.
//
// As of Go 1.12, the Body will also implement io.Writer on a successful "101
// Switching Protocols" response, as used by WebSockets and HTTP/2's "h2c"
// mode.
Body io.ReadCloser
// ... other fields elided ...
}What do you think?
iwvelando
commented
4 years ago Ah right, I see your point. roundTrip itself can return a nil response in the event of certain errors, so we can check for that.
iwvelando
commented
4 years ago Applied nil check and squashed.
According to https://golang.org/src/net/http/transport.go in roundTrip one early failure case that should result in a nil response and an error is an invalid scheme; I'll force failures by setting a bogus scheme. Also, it only returns a non-nil response if errors are nil, so I think it's sufficient to verify that the response is not nil (i.e. we don't also have to check -- after that -- that resp.Body is not nil).
I made this local patch:
$ git diff
diff --git a/conn.go b/conn.go
index 33f444a..2edaa30 100644
--- a/conn.go
+++ b/conn.go
@@ -84,6 +84,7 @@ func (c *Conn) doRequest(method, url string, reqBody, respBody interface{}) erro
req.Header.Add("Authorization", fmt.Sprintf("Bearer %s", c.accessToken))
}
+ req.URL.Scheme = "HT"
resp, err := c.rt.RoundTrip(req)
defer resp.Body.Close()and rebuilt:
$ ./tesla-stats-collector
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x40 pc=0x67f14a]
goroutine 1 [running]:
github.com/iwvelando/tesla.(*Conn).doRequest(0xc0000dbc50, 0x761b38, 0x4, 0x763a0c, 0xc, 0x6d70a0, 0xc00008e0f0, 0x6d7120, 0xc00005e540, 0x0, ...)
/home/iwvelando/go/src/github.com/iwvelando/tesla/conn.go:90 +0x2aa
github.com/iwvelando/tesla.(*Conn).Authenticate(0xc0000dbc50, 0xc000018360, 0x13, 0xc000016740, 0xf, 0xc00001a217, 0x1f)
/home/iwvelando/go/src/github.com/iwvelando/tesla/auth.go:42 +0x181
main.main()
/home/iwvelando/go/src/github.com/iwvelando/tesla-stats-collector/main.go:164 +0x725As you pointed out this needs to be covered.
Now I made this local patch:
$ git diff
diff --git a/conn.go b/conn.go
index 33f444a..3866e3c 100644
--- a/conn.go
+++ b/conn.go
@@ -84,9 +84,12 @@ func (c *Conn) doRequest(method, url string, reqBody, respBody interface{}) erro
req.Header.Add("Authorization", fmt.Sprintf("Bearer %s", c.accessToken))
}
+ req.URL.Scheme = "HT"
resp, err := c.rt.RoundTrip(req)
- defer resp.Body.Close()
+ if resp != nil {
+ defer resp.Body.Close()
+ }
if err != nil {
return fmt.Errorf("error performing http request: %w", err)Now after rebuilding:
$ ./tesla-stats-collector
{"level":"fatal","ts":1593056337.2011576,"caller":"tesla-stats-collector/main.go:166","msg":"failed to connect to Tesla API","op":"main","error":"error performing http request: unsupported protocol scheme \"HT\"","stacktrace":"main.main\n\t/home/iwvelando/go/src/github.com/iwvelando/tesla-stats-collector/main.go:166\nruntime.main\n\t/usr/local/go/src/runtime/proc.go:203"}Finally we'll just leave in the response nil check:
$ git diff
diff --git a/conn.go b/conn.go
index 33f444a..67afbe5 100644
--- a/conn.go
+++ b/conn.go
@@ -86,7 +86,9 @@ func (c *Conn) doRequest(method, url string, reqBody, respBody interface{}) erro
resp, err := c.rt.RoundTrip(req)
- defer resp.Body.Close()
+ if resp != nil {
+ defer resp.Body.Close()
+ }
if err != nil {
return fmt.Errorf("error performing http request: %w", err)Rebuild and add some debug output:
$ ./tesla-stats-collector
[{vehicle_charge_state map[api_version:8...[redacted]@rickbassham let me know what you think of the latest version or if you have any further requests. Thanks.
rickbassham
commented
4 years ago :tada: This PR is included in version 1.0.1 :tada:
The release is available on GitHub release
Your semantic-release bot :package::rocket:
Background
@rickbassham , I've been using your library for a tesla stats poller service. Recently I noticed I've been getting 408 response codes with some frequency, and eventually data stops flowing in. I noticed:
Turns out the service was leaving behind these connections after these errors happened.
Testing
Pre-patch
Here's the status of my poller:
So it got its first 408, and the result:
You can see one additional connection to the API endpoint.
After some time passes:
and you can see how this will become problematic.
Post-patch
Here is the same service running with this patch.
We've gotten several 408's, but now...
Much better.