ConfigureTransport: enable compression for non unix/npipe protos #106
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
`ConfigureTransport` is used by the official Docker client to set up its `http.Transport`: * [`NewClientWithOpts()`][1] starts by calling `defaultHTTPClient()` which creates a new `http.Transport` and call `ConfigureTransport` with either the default unix socket address, or the default npipe socket address (see [here][2]). This first call to `ConfigureTransport` will set `DisableCompression` to false. * Then, when [`WithHost()`][3] is called, `ConfigureTransport` is called once again with the same `http.Transport` instance. If that hostURL uses tcp proto, we fall in `ConfigureTransport`'s default switch case, which doesn't reset `DisableCompression`. This effectively means that **the Docker client has (presumably) never been using HTTP compression for TCP hosts.** Note that `WithHost` is called whenever the `WithHostFromEnv` or `FromEnv` option funcs are used. We think it _does_ make sense to keep compression disabled for unix / npipe sockets (it would just make the CPU needlessly spin more if we enable it), and we think it generally also makes sense to enable it for HTTP over TCP. We discussed creating a new `http.Transport` in Docker client codebase whenever `ConfigureTransport` is called, but we can't do that without introducing major side-effects (see [here][4]). So we still have to leave in a world where multiple calls to `ConfigureTransport` might be made for the same `http.Transport`. As such, this commit make sure compression is enabled for non unix / npipe protos. If `ConfigureTransport` callers really want to make sure compression is disabled, they should set `DisableCompression` _only after_ all subsequent calls to `ConfigureTransport` have been made. The comment on that function has also been updated to reflect that. For the record, `DisableCompression = true` was first introduced in this repo by 88d5fb2, which can be tracked down to this initial change in moby/moby: moby/moby@fb7ceeb [1]: https://github.com/moby/moby/blob/e9efc0a361de39903d47fe12f628c92fc095397b/client/client.go#L181-L197 [2]: https://github.com/moby/moby/blob/e9efc0a361de39903d47fe12f628c92fc095397b/client/client.go#L237-L238 [3]: https://github.com/moby/moby/blob/e9efc0a361de39903d47fe12f628c92fc095397b/client/options.go#L71-L73 [4]: docker#103 (comment) Signed-off-by: Albin Kerouanton <[email protected]>
akerouanton
force-pushed
the
fix-DisableCompression
branch
from
e6e0adf to
2573a15
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
ConfigureTransportis used by the official Docker client to set up itshttp.Transport:NewClientWithOpts()starts by callingdefaultHTTPClient()which creates a newhttp.Transportand callConfigureTransportwith either the default unix socket address, or the default npipe socket address (see here). This first call toConfigureTransportwill setDisableCompressionto false.WithHost()is called,ConfigureTransportis called once again with the samehttp.Transportinstance. If that hostURL uses tcp proto, we fall inConfigureTransport's default switch case, which doesn't resetDisableCompression. This effectively means that the Docker client has (presumably) never been using HTTP compression for TCP hosts.Note that
WithHostis called whenever theWithHostFromEnvorFromEnvoption funcs are used.We think it does make sense to keep compression disabled for unix / npipe sockets (it would just make the CPU needlessly spin more if we enable it), and we think it generally also makes sense to enable it for HTTP over TCP.
We discussed creating a new
http.Transportin Docker client codebase wheneverConfigureTransportis called, but we can't do that without introducing major side-effects (see here). So we still have to leave in a world where multiple calls toConfigureTransportmight be made for the samehttp.Transport.As such, this commit make sure compression is enabled for non unix / npipe protos.
If
ConfigureTransportcallers really want to make sure compression is disabled, they should setDisableCompressiononly after all subsequent calls toConfigureTransporthave been made. The comment on that function has also been updated to reflect that.For the record,
DisableCompression = truewas first introduced in this repo by 88d5fb2, which can be tracked down to this initial change in moby/moby: moby/moby@fb7ceeb