feat: add mutex to make Client thread-safe #827
Conversation
|
@tttturtle-russ The PR has many changes, which will take some time to review; I will get back to you. Thanks. |
|
@jeevatkm Sure. If there is any problem just let me know. |
There was a problem hiding this comment.
@tttturtle-russ Thanks for creating for Resty v3 on branch main.
- Please look at the individual comment and address it
- PR level review comment: Please use the newly created getter methods on all test files (where applicable) instead of direct internal variable access.
client.go
Outdated
| // client.SetError(&error{}) | ||
| // // OR |
There was a problem hiding this comment.
@tttturtle-russ, please do not modify it; keep it as-is.
client.go
Outdated
| // // OR | ||
| // client.SetError(Error{}) | ||
| // client.SetError(error{}) |
There was a problem hiding this comment.
@tttturtle-russ, please do not modify it; keep it as-is.
client.go
Outdated
| @@ -689,86 +839,167 @@ func (c *Client) SetRedirectPolicy(policies ...interface{}) *Client { | |||
| } | |||
| return nil // looks good, go ahead | |||
| } | |||
|
|
|||
| c.lock.Unlock() | |||
There was a problem hiding this comment.
@tttturtle-russ Any reason you didn't use defer c.lock.Unlock() next to lock line on 842?
context_test.go
Outdated
There was a problem hiding this comment.
@tttturtle-russ, is there any reason to modify the log lines on this file to change the case?
There was a problem hiding this comment.
It's due to the global rename. Sorry for that.
example_test.go
Outdated
| @@ -154,7 +154,7 @@ func Example_put() { | |||
| Tags: []string{"article", "sample", "resty"}, | |||
| }). | |||
| SetAuthToken("C6A79608-782F-4ED0-A11D-BD82FAD829CD"). | |||
| SetError(&Error{}). // or SetError(Error{}). | |||
| SetError(&Error{}). // or SetError(error{}). | |||
There was a problem hiding this comment.
@tttturtle-russ The comment syntax does not need to be modified. If there is any reason, please don't hesitate to let me know.
request.go
Outdated
| @@ -333,7 +333,7 @@ func (r *Request) SetResult(res interface{}) *Request { | |||
| return r | |||
| } | |||
|
|
|||
| // SetError method is to register the request `Error` object for automatic unmarshalling for the request, | |||
| // SetError method is to register the request `error` object for automatic unmarshalling for the request, | |||
There was a problem hiding this comment.
@tttturtle-russ This is incorrect; please revert this line.
request_test.go
Outdated
There was a problem hiding this comment.
@tttturtle-russ There is no need to modify the log lines in the file.
response.go
Outdated
There was a problem hiding this comment.
@tttturtle-russ There is no need to modify the log lines in the file.
resty_test.go
Outdated
There was a problem hiding this comment.
@tttturtle-russ There is no need to modify the log lines in the file.
retry.go
Outdated
| @@ -178,7 +178,7 @@ func sleepDuration(resp *Response, min, max time.Duration, attempt int) (time.Du | |||
| return jitterBackoff(min, max, attempt), nil | |||
| } | |||
|
|
|||
| retryAfterFunc := resp.Request.client.RetryAfter | |||
| retryAfterFunc := resp.Request.client.retryAfter | |||
There was a problem hiding this comment.
@tttturtle-russ Please use the newly created getter method instead of direct internal field access.
retryAfterFunc := resp.Request.client.RetryAfter()
|
@jeevatkm Thanks for your review. I will revise them. |
There was a problem hiding this comment.
@tttturtle-russ Code updates looks good, can you please focus on test failures and test coverage improvements?
|
@jeevatkm I have fix the CI error and pushed a new commit. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #827 +/- ##
==========================================
- Coverage 96.55% 95.53% -1.02%
==========================================
Files 11 11
Lines 1682 1925 +243
==========================================
+ Hits 1624 1839 +215
- Misses 37 65 +28
Partials 21 21 ☔ View full report in Codecov by Sentry. |
|
@tttturtle-russ Thanks for fixing the test cases. Please have a look and take care of test case coverage. |
|
@jeevatkm Thanks for reviewing. I fixed the test case coverage and found a duplicated method. I remove it in the newest commit and now the code cov should be good. |
|
Thanks, @tttturtle-russ, for improving test cases. Can you please check the code conflicts? |
|
@jeevatkm I've resolved the conflict. I see a new public field in |
|
@tttturtle-russ Lets do that in a separate PR. For now, let's get this PR finished and merged. |
|
@tttturtle-russ There is Code format issue, maybe you can add that public field covered. |
|
@jeevatkm Sure |
|
@jeevatkm Everything should be fine now. |
|
@tttturtle-russ The |
@jeevatkm Actually it works well from my side. |
|
@tttturtle-russ What do you mean it works? Can you try this command at your end? You can also see failure in the tab |
|
@jeevatkm I am sorry, it's not OK :( |
Refer to #812 for more info.
closes #812