-
Notifications
You must be signed in to change notification settings - Fork 23
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Set custom http client #67
Conversation
async_transport.go
Outdated
@@ -27,6 +28,7 @@ type AsyncTransport struct { | |||
PrintPayloadOnError bool | |||
bodyChannel chan payload | |||
waitGroup sync.WaitGroup | |||
httpClient *http.Client |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PS: I believe that is also true of all of the above methods as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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! |
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.DefaultClient
or to re-implement transport'sSend
method. Neither is handy. If you're ok with general direction will add specs and doc comments. Thanks.