Set custom http client#67
Conversation
| PrintPayloadOnError bool | ||
| bodyChannel chan payload | ||
| waitGroup sync.WaitGroup | ||
| httpClient *http.Client |
There was a problem hiding this comment.
Btw, wdyt about extracting duplicated code into BaseTransport struct and embedding it in both transports?
|
@Bobochka This looks like a good idea. I'd recommend storing the client in |
|
@waltjones Thanks for the feedback! I might be missing something, but it seems that because of https://github.com/rollbar/rollbar-go/blob/master/client.go#L603 if client will be stored in |
|
@Bobochka I think I looked too quickly and thought it was
|
| } | ||
|
|
||
| io.Copy(ioutil.Discard, resp.Body) | ||
| resp.Body.Close() |
There was a problem hiding this comment.
@waltjones note the change in 2 lines above. Body draining allows connection reuse, currently it's reconnecting every request.
|
@waltjones changed accordingly, take a look please. Specs and comments to go. |
| } | ||
|
|
||
| // SetHTTPClient sets custom http client. http.DefaultClient is used by default | ||
| func (t *baseTransport) SetHTTPClient(c *http.Client) { |
There was a problem hiding this comment.
I'm pretty sure this needs to be protected by a mutex otherwise it's a race condition once the transport is actively sending things to rollbar.
There was a problem hiding this comment.
PS: I believe that is also true of all of the above methods as well.
There was a problem hiding this comment.
Well, technically you're right, but in reality nothing bad can happen - pointers aren't spread through multiple machine words (at least on most architectures), meaning it's not possible that broken pointer will be read by transport if it's already running.
There was a problem hiding this comment.
There seem to be a variety of potential concurrency issues. I'd rather address them together, and not burden this PR with it.
|
@waltjones Please let me know if other changes needed. Thanks. |
|
@Bobochka I expect to be able to get to it this week. Thank you! |
|
@Bobochka It looks like the default http client doesn't get initialized. This condition can be exercised by running tests with a live token: For the lint error, it looks like you could just swap the return values. Let me know if there's any reason not to do that. Overall everything else is looking good. I'd like to do more testing once the default client issue is resolved. |
|
@waltjones sorry, my bad. Fixed both issues. |
|
@Bobochka thank you! |
3 participants
Hello.
I wish there could be a way to set custom http client. Currently the only way to change http settings is either to change
http.DefaultClientor to re-implement transport'sSendmethod. Neither is handy. If you're ok with general direction will add specs and doc comments. Thanks.