-
Notifications
You must be signed in to change notification settings - Fork 9.2k
Allow Addresses to have a max calls per connection #8386
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
base: master
Are you sure you want to change the base?
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
@@ -368,6 +384,18 @@ class RealConnection( | |||
} | |||
} | |||
|
|||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
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.
Seems reasonable to me, but I recommend waiting for a review from a more experienced maintainer
c6a955f
to
96345cf
Compare
@@ -26,7 +26,7 @@ import java.util.concurrent.TimeUnit.MILLISECONDS | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no longer need when the references from lock.withLock
were changed to this.withLock
- though that extension function still forwards to
lock.withLock
so same concurrent method used one step further.
* 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
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.