Do not override the provided HTTP client in the fetcher #302
Conversation
fetcher/fetcher.go
Outdated
| defaultTransport := http.DefaultTransport.(*http.Transport).Clone() | ||
| defaultTransport.IdleConnTimeout = DefaultIdleConnTimeout | ||
| defaultTransport.MaxIdleConns = DefaultMaxConnections | ||
| defaultTransport.MaxIdleConnsPerHost = DefaultMaxConnections |
There was a problem hiding this comment.
The one behavioral change here for clients who are not passing their own rosetta API client is that we're not leveraging the specified max connections
We could potentially change this to occur after applying the options and check if a client is initialized, e.g.
for _, opt := range options {
opt(f)
}
if f.rosettaClient == nil {
f.rosettaClient = newDefaultAPIClient(c.maxConnections)
}Also, that way we're also not building out that client, if it is already being passed in with the WithClient option.
There was a problem hiding this comment.
Great catch! And great suggestion! There is no need for us to init the fetcher with a client at all. We can delay ituntil after the opts and that gives us much more sensible code!
Pull Request Test Coverage Report for Build 14509
💛 - Coveralls |
| ) | ||
| fetcher := New("https://serveraddress", WithClient(apiClient)) | ||
| var assert = assert.New(t) | ||
| assert.Same(httpClient, fetcher.rosettaClient.GetConfig().HTTPClient) |
There was a problem hiding this comment.
lmk if you think we should assert that the Transport field of the httpClient remains the same as well.
fetcher/fetcher.go
Outdated
| client := client.NewAPIClient(clientCfg) | ||
| f.rosettaClient = client |
There was a problem hiding this comment.
nit: seems like we can consolidate them into (but it's trivial)
| client := client.NewAPIClient(clientCfg) | |
| f.rosettaClient = client | |
| f.rosettaClient = client.NewAPIClient(clientCfg) |
|
Review Error for alex-stone @ 2021-03-03 02:25:14 UTC |
Motivation
When creating a fetcher instance, we always override the
Transportof thehttp.Clientpresent inside theAPIClientobject. This leads to unexpected behavior when callers provide their ownhttp.Clientvia theWithClientoption.Solution
This change makes it so that we do not override the Transport when it is already provided . Note we still update the Transport's
TLSClientConfigfield wheninsecureTLSis set to true.Open questions