etcd/client: ensure response body is always closed #620
Conversation
|
@unihorn how about this? |
|
I don't think it fixes the bug because |
|
Ah, now I see what you mean. |
|
@unihorn take a look now? |
| respchan := make(chan struct { | ||
| r *http.Response | ||
| b []byte | ||
| e error |
There was a problem hiding this comment.
If we're going to communicate an error through this struct, maybe we can drop the other channel (errchan chan error)?
There was a problem hiding this comment.
Good call, updated.
|
@jonboulle This approach is good to me. :) |
| case err = <-errchan: | ||
| return nil, nil, err | ||
| case res := <-respchan: | ||
| return res.r, res.b, res.e |
There was a problem hiding this comment.
How about we change this line to:
resp, body, err = res.r, res.b, res.e
Then we put a bare return at the bottom of the function
|
@bcwaldon updated with tests and complete coverage for |
| @@ -22,6 +22,13 @@ type Client interface { | |||
| Wait(Action, <-chan bool) (*Result, error) | |||
| } | |||
|
|
|||
| // transport wraps http.Transport to provide an interface which can | |||
| // be substituted for testing | |||
There was a problem hiding this comment.
Maybe making a note about needing this because http.RoundTripper interface does not require the CancelRequest method?
There was a problem hiding this comment.
hah, I had that originally. Will add.
|
@jonboulle lgtm - thanks for slogging through this |
|
@unihorn any final thoughts? |
|
@jonboulle Some squashing would be appropriate before merge. |
|
Sure thing, guy.
|
There was a race condition when cancelling a request since the actual RoundTrip is performed in a goroutine, and the cancel is done asynchronously. This reworks the goroutine to ensure that the Body is closed whenever a non-nil response is retrieved from the RoundTrip. It also introduces a transport interface to support testing of requestHTTP and adds complete coverage of the function.
etcd/client: ensure response body is always closed
No description provided.