Allow Addresses to have a max calls per connection#8386
Allow Addresses to have a max calls per connection#8386
Conversation
This adds the ability for the Address Policy to also contain a maximum number of connections per Stream. In some environments it seems that OkHttp's HTTP/2 gets worse the more streams that are packed onto a single connection. By adding in the ability for users to alter their client's AddressPolicy to set a maximum number of connections per stream, they can control how much (if any) packing they want OkHttp to perform.
okhttp/src/main/kotlin/okhttp3/internal/connection/RealConnection.kt
Outdated
Show resolved
Hide resolved
|
|
||
| private fun getMaximumAllocationLimit(): Int { | ||
| // if we have not negotiated a max per streams yet, don't check for the policy override | ||
| val negotiatedMaxCurrentStreams = lastMaxConcurrentStreamsFromSettings ?: return 1 |
There was a problem hiding this comment.
Isn't this just defaulted to 1 for Http/1 and Http2Connection.DEFAULT_SETTINGS.getMaxConcurrentStreams() for Http/2?
should it be a lateinit and start sets to one or the other?
There was a problem hiding this comment.
maybe?
private var lastMaxConcurrentStreamsFromSettings = 1 would avoid the early return, but then we're checking against the policy.
Would it be better to have the null and early return or check the policy every time, even if we're not in HTTP/2 land?
ean5533
left a comment
ean5533
left a comment
There was a problem hiding this comment.
Seems reasonable to me, but I recommend waiting for a review from a more experienced maintainer
peckb1
force-pushed
the
feature.peckb.address-policy-max-calls-per-connection
branch
from
c6a955f to
96345cf
Compare
| import java.util.concurrent.locks.ReentrantLock | ||
| import javax.net.ssl.SSLPeerUnverifiedException | ||
| import javax.net.ssl.SSLSocket | ||
| import kotlin.concurrent.withLock |
There was a problem hiding this comment.
no longer need when the references from lock.withLock were changed to this.withLock
- though that extension function still forwards to
lock.withLockso same concurrent method used one step further.
peckb1
changed the title
| * The maximum number of concurrent calls per connection for a given address. | ||
| * Set this value to 1 to disable HTTP/2 connection coalescing | ||
| */ | ||
| @JvmField val maximumConcurrentCallsPerConnection: Int? = null, |
There was a problem hiding this comment.
nit: I think we use the max prefix elsewhere.
| @JvmField val maximumConcurrentCallsPerConnection: Int? = null, | |
| @JvmField val maxConcurrentCallsPerConnection: Int? = null, |
| class ConnectionPoolPolicy( | ||
| /** | ||
| * The maximum number of concurrent calls per connection. | ||
| * Set this value to 1 to disable HTTP/2 connection coalescing |
There was a problem hiding this comment.
I'm not sure "connection coalescing" is the right term here.
From https://daniel.haxx.se/blog/2016/08/18/http2-connection-coalescing/
Connection coalescing means that the browser tries to determine which of the remote hosts that it can reach over the same TCP connection.
| ) | ||
|
|
||
| /** | ||
| * A default policy for all connections. Overridable by [AddressPolicy] |
There was a problem hiding this comment.
AddressPolicy is internal, so this effectively makes a public feature expose an internal one.
I'd like @swankjesse take on this.
I guess I wonder whether this indicates rather than two tiered policy (ConnectionPoolPolicy then AddressPolicy), that a single ConnectionPoolPolicy interface that can answer either by global or per address logic.
|
AddressPolicy went internal after this PR, in order to ship 5.0. So worth discussion (including @swankjesse) about the public API for these features before too much rework on this PR. |
|
Can you fix the merge conflicts? |
3 participants
Overview
This adds the ability for the Address Policy to also contain a maximum number of connections per Stream.
Additionally creates a ConnectionPoolPolicy which can have default options set up for all connections in the pool.
Context
In some environments it seems that OkHttp's HTTP/2 gets worse the more streams that are packed onto a single connection.
By adding in the ability for users to alter their client's AddressPolicy to set a maximum number of connections per stream, they can control how much (if any) packing they want OkHttp to perform.