Add a ConnectionListener#7456
Conversation
| @Flaky // STDOUT logging enabled for test | ||
| @Timeout(30) | ||
| @Tag("Slow") | ||
| class ConnectionListenerTest { |
There was a problem hiding this comment.
This is currently just a subset of EventListenerTest without too much thought of the interesting cases.
swankjesse
left a comment
swankjesse
left a comment
There was a problem hiding this comment.
I think this is a great idea. Let’s ship it in 5.0.
| open fun connectionOpened(connection: Connection) {} | ||
| open fun connectionClosed(connection: Connection) {} | ||
| open fun connectionAcquired(connection: Connection, call: Call) {} | ||
| open fun connectionReleased(connection: Connection, call: Call) {} |
There was a problem hiding this comment.
I wonder if we should have successCount here?
There was a problem hiding this comment.
Or perhaps something else where we track possibly-non-fatal exceptions, like RealConnection.trackFailure() ?
There was a problem hiding this comment.
Yep - let's add in a follow up. We should definitely expose this information here, but worth dedicated focus.
There was a problem hiding this comment.
Good call. Maybe it aught to be a property on the connection.
# Conflicts: # okhttp/src/jvmMain/kotlin/okhttp3/internal/connection/RealRoutePlanner.kt
yschimke
changed the title
| field = value | ||
| if (!previousValue) { | ||
| queueEvent { | ||
| connectionListener.noNewExchanges(this) |
There was a problem hiding this comment.
This is the one that feels unfortunate... Relies on connection.synchronizedWithEvents
There was a problem hiding this comment.
Yeah I think we should just pull up the event-notifying side-effect into the callsite
swankjesse
left a comment
swankjesse
left a comment
There was a problem hiding this comment.
I’d like to find a way to avoid the event queueing. . . might not be possible? Lemme take a stab at it.
|
I think it’s possible 'cause we already plumbed through the |
swankjesse
force-pushed
the
connection_listener
branch
from
ab75541 to
6c1c796
Compare
swankjesse
force-pushed
the
connection_listener
branch
from
6c1c796 to
e7d750b
Compare
| logEvent(ConnectionEvent.ConnectEnd(System.nanoTime(), connection)) | ||
| } | ||
|
|
||
| override fun connectionClosed(connection: Connection) = logEvent(ConnectionEvent.ConnectionClosed(System.nanoTime(), connection)) |
There was a problem hiding this comment.
We should assert that Start & Close events are balanced.
| logEvent(ConnectionEvent.ConnectionAcquired(System.nanoTime(), connection, call)) | ||
| } | ||
|
|
||
| override fun connectionReleased(connection: Connection, call: Call) { |
There was a problem hiding this comment.
We should assert that Acquire & Release events are balanced.
There was a problem hiding this comment.
I can add. Probably as a fast follow.
| private val handshakeCertificates = localhost() | ||
|
|
||
| open val fastFallback: Boolean = true | ||
| open val fastFallback: Boolean get() = true |
There was a problem hiding this comment.
What's the difference here?
There was a problem hiding this comment.
Avoids a potential problem accessing state from the superclass constructor.
| "ConnectionAcquired", | ||
| "NoNewExchanges", | ||
| "ConnectionReleased", | ||
| "ConnectionClosed", |
| } | ||
|
|
||
| private fun logEvent(e: ConnectionEvent) { | ||
| if (e.connection != null) { |
| /** | ||
| * Invoked when a connection is marked for no new exchanges. | ||
| */ | ||
| open fun noNewExchanges(connection: Connection) {} |
There was a problem hiding this comment.
FYI, with my pushed commits we don’t send this event if we’re definitely going to close the connection immediately
| connection.assertThreadHoldsLock() | ||
|
|
||
| connections.add(connection) | ||
| // connection.queueEvent { connectionListener.connectEnd(connection) } |
There was a problem hiding this comment.
What to do here? I think we need a test to check symmetry before we can drop this comment
There was a problem hiding this comment.
OK, I can add, unless you plan to.
There was a problem hiding this comment.
Lets do follow-up. This PR is big enough!
| Platform.get().logCloseableLeak(message, callReference.callStackTrace) | ||
|
|
||
| references.removeAt(i) | ||
| connection.noNewExchanges = true |
There was a problem hiding this comment.
Very small behavior change here - if we leak a Call that doesn‘t immediately mark the connection with noNewExchanges. That’s fine.
2 participants
Given that we have happy eyeballs, and are considering features like client side load balancing, having visibility into connections seems warranted.
Also specifically for Android, dealing with different networks makes this important.