Fix data race in hystrix client #67
Conversation
| @@ -179,7 +179,7 @@ func (hhc *Client) Do(request *http.Request) (*http.Response, error) { | |||
| } | |||
|
|
|||
| err = hystrix.Do(hhc.hystrixCommandName, func() error { | |||
| response, err = hhc.client.Do(request) | |||
| resp, err := hhc.client.Do(request) | |||
There was a problem hiding this comment.
Can you elaborate on this fix plz ?
There was a problem hiding this comment.
There are two possible source of errors, hystrix and net/http. If both having the same timeout value, the err will be in race, as it can override one another.
There was a problem hiding this comment.
I was having the same issue in a project of mine, in which I do:
for _, someURL := range someURLs {
resp, err := customClient.Get(ctx, someURL, http.Header{})
if err != nil {
continue
}
defer log.CloseOrLog(resp.Body)
body, err = ioutil.ReadAll(resp.Body)
if err != nil {
continue
}
myVar += string(body)
}
And I had the race condition like:
==================
WARNING: DATA RACE
Write at 0x00c000176040 by goroutine 37:
repos_host/team/project/vendor/github.com/gojektech/heimdall/hystrix.(*Client).Do()
/GO_PATH/go/src/repos_host/team/project/vendor/github.com/gojektech/heimdall/hystrix/hystrix_client.go:181 +0x3de
Some trace up to a line of code in which we do a `hystrixClient.Do()` call...
testing.tRunner()
/usr/local/Cellar/go/1.12.7/libexec/src/testing/testing.go:865 +0x163
After trying with this branch, that race condition didn't happen anymore. cc @senekis
| @@ -407,25 +407,10 @@ func BenchmarkHystrixHTTPClientRetriesPostOnFailure(b *testing.B) { | |||
| } | |||
| } | |||
|
|
|||
| func TestHystrixHTTPClientReturnsFallbackFailureWithoutFallBackFunction(t *testing.T) { | |||
| @@ -518,6 +503,40 @@ func TestCustomHystrixHTTPClientDoSuccess(t *testing.T) { | |||
| assert.Equal(t, "{ \"response\": \"ok\" }", string(body)) | |||
| } | |||
|
|
|||
| func TestHystrixHTTPClientGetReturnedURLTimeout(t *testing.T) { | |||
There was a problem hiding this comment.
I have tested this branch and the tests pass successfully
|
Hello, is there any progress on this? Is this repo still being maintained? |
|
@ardhitama Can you help resolve conflicts if this is still relevant? |
|
fixed in latest master |
This fix #63