Various small cleanups #139
Conversation
|
@ryanuber 👋 Hello! Any chance to get this reviewed please? |
client.go
Outdated
| // Always rewind the request body when non-nil. | ||
| if req.body != nil { | ||
| body, err := req.body() | ||
| if err != nil { | ||
| c.HTTPClient.CloseIdleConnections() |
There was a problem hiding this comment.
See #57 (comment) - this was actually intentional and wasn't meant to keep connections open and reusable outside of the retryable operation. I'm unsure of the extent of consequences if we change this, so I'm a bit hesitant to go ahead with this. Any particular reason you're wanting to change this?
There was a problem hiding this comment.
If the goal is to avoid sharing connections across client invocations then the current implementation is not achieving that. For concurrent invocations of the Do() method the underlying transport is reused and hence it may pick the same TCP connection between retries.
If we really want to always use fresh connections then a new transport needs to be created for each Do() invocation rather than juggling with CloseIdleConnections(). It would be cleaner and easier to understand.
I'd like to challenge this approach though. I think the job of the http client is to perform retries using a transport and that's it. Manage the lifecycle of tcp connections is not http client's job, it's the job of the transport. We are trying to do things at an abstraction level that is too high and that's why it feels clunky.
So I suggest to change things in the following way:
- Instead of the
http client, take an http client factory that returns an interface rather than a concrete type. The factory is called to get a client for eachDo()call. - Provide a factory implementation that returns a fresh client each time. This is used by default and achieves what was intended.
- Remove all
CloseIdleConnections()calls.
With this approach http client interface will only contain Do(*http.Request) *http.Response and no other methods. This is good enough for this library and would allow users to implement any kind of wrappers with any kind of behavior - shared clients with keep-alives, clients with no keep-alives, keep-alives within a single Do() call, client middlewares, , transport middlewares, etc. This would make this library even more flexible and composable.
WDYT? I'm happy to open a PR.
In my case (and I'm probably not alone here), I'd like to keep keep-alives and use a shared client.
There was a problem hiding this comment.
Well, the http client interface would also have a Done() method to let it be cleaned up/etc when it's no longer needed by the Do() method. It can then call CloseIdleConnections()/etc if it wants to.
There was a problem hiding this comment.
To clarify - the above approach would allow to inject not only an http client, but work with transports too as the factory would be able to do anything it needs to construct an http client, including configuring the transport.
There was a problem hiding this comment.
I've removed this change to not block the remaining things on it, but I'd like to continue the conversation.
There was a problem hiding this comment.
I agree it's worth revisiting. It sounds like this would circle around breaking change territory (whether or not we removed the HTTPClient struct field on *Client), so it's probably worth opening a separate issue or proposal PR. We can also bring in a few others to that conversation who were closer to the original CloseIdleConnections() implementation in case there are potential gotchas here we aren't thinking of.
No description provided.