laantungir/amethyst
1
0
Fork 0
mirror of https://github.com/vitorpamplona/amethyst.git synced 2026-09-14 00:55:08 +00:00
Code Issues Packages Projects Releases Wiki Activity

perf(http): drop the proxy-change pool eviction in both OkHttp factories

Browse Source 
Both factories emptied the whole connection pool whenever the proxy differed
from the last one they were handed. It was defensive, and it was not free.

It is not needed. OkHttp keys the pool by `Address`, and `Address.equalsNonHost`
compares `proxy` — so a call is only ever given a connection opened through the
very same route. A connection left over from an old proxy is already unreachable
by anything using the new one; it just ages out of the pool on its own. There was
never a stale-route connection to protect against.

The cost was real, though. `evictAll()` empties the ENTIRE shared pool, and each
factory mints BOTH clients: `DualHttpClientManager` builds defaultHttpClient
(always SOCKS, since buildLocalSocksProxy falls back to 9050 rather than
returning null) and defaultHttpClientWithoutProxy (always null) from one
instance, and they share one `rootClient.connectionPool`. A single "last proxy"
field therefore alternated forever, and every rebuild read as a route change and
dropped every warm connection the other client was relying on. Both are
stateIn(WhileSubscribed(1000)) flows collected from a composable, so it fired on
each isMobileDataProvider change and each foreground round trip — and the next
image then paid a fresh DNS + TCP + TLS. Plausibly the "first image after a pause
takes forever" stall the pingInterval above it was added for.

DualHttpClientManagerForRelays has the identical shape, so the relay factory is
fixed the same way. Less damaging there — evictAll spares connections with active
calls, so live relay sockets survived — but it was still discarding idle pooled
connections on every network flap.

This replaces the ProxyRouteTracker approach from earlier on this branch, which
kept the eviction and merely made its bookkeeping correct. Deleting the mechanism
is the better answer, and it takes the class and its tests with it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TdidqyhRni5L3ft5h8qti5
This commit is contained in:
Claude
2026-09-13 01:43:43 +00:00
parent 5f53568204
commit 9e380cf9dc
3 changed files with 24 additions and 140 deletions
Download Patch File Download Diff File Expand all files Collapse all files
commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/service/http/OkHttpClientFactory.kt
+12 -44
View File
@@ -148,15 +148,22 @@ class OkHttpClientFactory(
.addInterceptor(OnionLocationInterceptor(onionCache))
.build()
private val proxyRoutes = ProxyRouteTracker()
// No connection-pool eviction when the proxy changes. OkHttp's `Address` -- the
// pool's lookup key -- includes the proxy (`Address.equalsNonHost`), so a call
// is only ever handed a connection opened through the very same route. A
// connection left over from an old proxy is already unreachable and simply ages
// out of the pool; evicting was defensive, not load-bearing.
//
// It also cost more than it looked. `evictAll()` empties the ENTIRE shared pool,
// and this one factory mints both the proxied and the direct client (see
// [DualHttpClientManager]) -- `buildLocalSocksProxy` never returns null, so those
// two alternated a single "last proxy" field forever. Every rebuild read as a
// route change and dropped every warm connection the other client was using, on
// each network-state emission and each resubscribe.
fun buildHttpClient(
proxy: Proxy?,
timeoutSeconds: Int,
): OkHttpClient {
if (proxyRoutes.shouldEvictFor(proxy)) {
rootClient.connectionPool.evictAll()
}
val seconds = if (proxy != null) timeoutSeconds * 3 else timeoutSeconds
return rootClient
.newBuilder()
@@ -202,42 +209,3 @@ class OkHttpClientFactory(
const val HTTP2_PING_INTERVAL_SECS: Long = 10
}
}
/**
* Decides when a rebuilt client must drop the pooled connections it shares with every other
* client [OkHttpClientFactory] mints.
*
* The eviction exists so a changed Tor route doesn't leave usable connections behind on the old
* one. The trap is that a single factory mints BOTH long-lived variants — [DualHttpClientManager]
* builds `defaultHttpClient` (always SOCKS, because `buildLocalSocksProxy` falls back to 9050
* rather than returning null) and `defaultHttpClientWithoutProxy` (always null) from the same
* instance, and they share one `rootClient.connectionPool`. Comparing every build against one
* "last proxy" field therefore saw the two variants alternate forever: each rebuild looked like a
* route change and wiped the pool they share. Both `stateIn` flows re-emit on every
* `isMobileDataProvider` change and every resubscribe (they are `WhileSubscribed(1000)`, collected
* from a composable), so in practice the pool was emptied whenever the network flapped or the app
* came back to the foreground — and the next image then paid a fresh DNS + TCP + TLS.
*
* Two rules fix it:
*
* - A direct build (`proxy == null`) never evicts. `null` is that variant's permanent route, so
* it can never have changed.
* - A proxied build evicts only when the proxy differs from the one the PREVIOUS proxied build
* used, i.e. the Tor port actually moved.
*
* Nothing is lost by being this narrow: OkHttp's `Address` — the connection-pool key — includes
* the proxy, so a direct connection and a SOCKS connection to the same host are already distinct
* entries that can never be handed to each other's calls.
*/
internal class ProxyRouteTracker {
private var lastProxy: Proxy? = null
@Synchronized
fun shouldEvictFor(proxy: Proxy?): Boolean {
if (proxy == null) return false
val previous = lastProxy
lastProxy = proxy
return previous != null && previous != proxy
}
}
commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/service/http/OkHttpClientFactoryForRelays.kt
+12 -6
View File
@@ -86,16 +86,22 @@ class OkHttpClientFactoryForRelays(
.addInterceptor(OnionLocationInterceptor(onionCache))
.build()
private var lastProxy: Proxy? = null
// No connection-pool eviction when the proxy changes. OkHttp's `Address` -- the
// pool's lookup key -- includes the proxy (`Address.equalsNonHost`), so a call
// is only ever handed a connection opened through the very same route. A
// connection left over from an old proxy is already unreachable and simply ages
// out of the pool; evicting was defensive, not load-bearing.
//
// It also cost more than it looked. `evictAll()` empties the ENTIRE shared pool,
// and this one factory mints both the proxied and the direct client (see
// [DualHttpClientManagerForRelays]) -- `buildLocalSocksProxy` never returns null, so those
// two alternated a single "last proxy" field forever. Every rebuild read as a
// route change and dropped every warm connection the other client was using, on
// each network-state emission and each resubscribe.
fun buildHttpClient(
proxy: Proxy?,
timeoutSeconds: Int,
): OkHttpClient {
if (proxy != lastProxy) {
rootClient.connectionPool.evictAll()
lastProxy = proxy
}
val seconds = if (proxy != null) timeoutSeconds * 3 else timeoutSeconds
return rootClient
.newBuilder()
commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/service/http/ProxyRouteTrackerTest.kt
-90
View File
@@ -1,90 +0,0 @@
/*
* Copyright (c) 2025 Vitor Pamplona
*
* Permission is hereby granted, free of charge, to any person obtaining a copy of
* this software and associated documentation files (the "Software"), to deal in
* the Software without restriction, including without limitation the rights to use,
* copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the
* Software, and to permit persons to whom the Software is furnished to do so,
* subject to the following conditions:
*
* The above copyright notice and this permission notice shall be included in all
* copies or substantial portions of the Software.
*
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS
* FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR
* COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN
* AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION
* WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
*/
package com.vitorpamplona.amethyst.commons.service.http
import java.net.InetSocketAddress
import java.net.Proxy
import kotlin.test.Test
import kotlin.test.assertFalse
import kotlin.test.assertTrue
class ProxyRouteTrackerTest {
private fun socks(port: Int) = Proxy(Proxy.Type.SOCKS, InetSocketAddress("127.0.0.1", port))
@Test
fun firstProxiedBuildDoesNotEvict() {
// Nothing is pooled on the old route because there is no old route.
assertFalse(ProxyRouteTracker().shouldEvictFor(socks(9050)))
}
@Test
fun directBuildNeverEvicts() {
val tracker = ProxyRouteTracker()
assertFalse(tracker.shouldEvictFor(null))
assertFalse(tracker.shouldEvictFor(null))
}
@Test
fun sameProxyRebuiltDoesNotEvict() {
val tracker = ProxyRouteTracker()
tracker.shouldEvictFor(socks(9050))
// A fresh-but-equal Proxy is what every rebuild hands us: buildLocalSocksProxy
// allocates a new instance each time, so this must compare by value.
assertFalse(tracker.shouldEvictFor(socks(9050)))
}
@Test
fun changedProxyPortEvicts() {
val tracker = ProxyRouteTracker()
tracker.shouldEvictFor(socks(9050))
assertTrue(tracker.shouldEvictFor(socks(9150)))
}
/**
* The regression this class exists for. [DualHttpClientManager] mints both variants from one
* factory — `defaultHttpClient` always proxied, `defaultHttpClientWithoutProxy` always direct
* — and both `stateIn` flows re-emit on every `isMobileDataProvider` change and every
* resubscribe. A single "last proxy" field saw that alternation as a route change every time
* and wiped the connection pool the two variants SHARE.
*/
@Test
fun alternatingBetweenProxiedAndDirectNeverEvicts() {
val tracker = ProxyRouteTracker()
repeat(10) {
assertFalse(tracker.shouldEvictFor(socks(9050)))
assertFalse(tracker.shouldEvictFor(null))
}
}
@Test
fun directBuildsBetweenProxyChangesDoNotMaskTheChange() {
val tracker = ProxyRouteTracker()
tracker.shouldEvictFor(socks(9050))
tracker.shouldEvictFor(null)
// The direct build in the middle must not reset what the proxied variant last used.
assertTrue(tracker.shouldEvictFor(socks(9150)))
}
}