diff --git a/kuri-bind/src/jvmMain/kotlin/org/dexpace/kuri/bind/internal/BoundedCache.kt b/kuri-bind/src/jvmMain/kotlin/org/dexpace/kuri/bind/internal/BoundedCache.kt new file mode 100644 index 0000000..68078f7 --- /dev/null +++ b/kuri-bind/src/jvmMain/kotlin/org/dexpace/kuri/bind/internal/BoundedCache.kt @@ -0,0 +1,77 @@ +/* + * Copyright (c) 2026 dexpace and Omar Aljarrah + * SPDX-License-Identifier: MIT + */ +package org.dexpace.kuri.bind.internal + +import java.util.concurrent.locks.ReentrantLock +import kotlin.concurrent.withLock + +/** + * A thread-safe, fixed-capacity least-recently-used cache. + * + * [PlanCompiler] and [KotlinReflectMemberScanner] key their reflective caches by `KClass`, which + * strongly retains its backing `java.lang.Class` and, through that, the defining `ClassLoader`. An + * unbounded cache pins every distinct type ever looked up — and its `ClassLoader` — for the life of the + * process; in a host that mints classes dynamically (per-deployment loaders, hot code redeploy, + * bytecode-generated proxies) that retention grows without bound. Capping the cache at [maxSize] + * entries bounds that retention: once full, inserting a new entry evicts the least-recently-used one, + * so a churning stream of distinct keys only ever pins the most recently used [maxSize] of them. + * + * Backed by a single [LinkedHashMap] in access order, guarded by a [ReentrantLock] rather than + * `synchronized` (the project avoids `synchronized` because it pins carrier threads under Loom); every + * operation runs in amortized O(1) under the lock. + */ +internal class BoundedCache( + private val maxSize: Int, +) { + init { + require(maxSize > 0) { "maxSize must be positive, was $maxSize" } + } + + private val lock = ReentrantLock() + + // `accessOrder = true` reorders an entry to most-recently-used on every `get`/`put`, so + // `removeEldestEntry` evicting once the map grows past `maxSize` always drops the + // least-recently-used entry rather than merely the oldest-inserted one. + private val delegate = + object : LinkedHashMap(INITIAL_CAPACITY, LOAD_FACTOR, true) { + override fun removeEldestEntry(eldest: MutableMap.MutableEntry): Boolean = size > maxSize + } + + /** + * Returns the cached value for [key], computing it via [compute] and caching the result on a miss. + * + * A concurrent miss on the same [key] may run [compute] more than once: the lock is released while + * [compute] runs, so two racing callers can each compute before either one inserts, and only one + * computed result survives in the cache. Every caller in this module compiles/scans a type as a + * pure function of that type, so the discarded result is equal to the one that wins and the race is + * harmless — see [PlanCompiler.planFor] and [KotlinReflectMemberScanner.scan]. + * + * @param key the cache key. + * @param compute produces the value for [key] on a cache miss; runs with the lock released (see + * above), so a reentrant call back into this cache for a different key — or a concurrent call on + * another thread — is safe and cannot deadlock. Recursing back into [getOrPut] for the SAME [key] + * from within [compute] is still unsafe, though not by deadlocking: [key] is only inserted after + * [compute] returns, so the recursive call also misses and invokes [compute] again, causing + * unbounded recursion rather than reusing an in-flight result. + * @return the cached or freshly computed value for [key]. + */ + fun getOrPut( + key: K, + compute: (K) -> V, + ): V { + lock.withLock { delegate[key] }?.let { return it } + val computed = compute(key) + return lock.withLock { delegate.getOrPut(key) { computed } } + } + + /** The number of entries currently held; exposed so tests can assert the size bound holds. */ + val size: Int + get() = lock.withLock { delegate.size } + + private companion object { + const val INITIAL_CAPACITY = 16 + const val LOAD_FACTOR = 0.75f + } +} diff --git a/kuri-bind/src/jvmMain/kotlin/org/dexpace/kuri/bind/internal/MemberScanner.kt b/kuri-bind/src/jvmMain/kotlin/org/dexpace/kuri/bind/internal/MemberScanner.kt index 08356c5..39f0c34 100644 --- a/kuri-bind/src/jvmMain/kotlin/org/dexpace/kuri/bind/internal/MemberScanner.kt +++ b/kuri-bind/src/jvmMain/kotlin/org/dexpace/kuri/bind/internal/MemberScanner.kt @@ -4,7 +4,6 @@ */ package org.dexpace.kuri.bind.internal -import java.util.concurrent.ConcurrentHashMap import kotlin.reflect.KClass import kotlin.reflect.KFunction import kotlin.reflect.KProperty1 @@ -71,14 +70,20 @@ internal interface MemberScanner { * * Inherited members are automatically included because `memberProperties` and `memberFunctions` * already traverse the full class hierarchy. + * + * The scan cache is capped at [maxCacheSize] entries (default [DEFAULT_MAX_SCAN_CACHE_SIZE]): a `KClass` + * key strongly retains its `ClassLoader`, so an unbounded cache would pin every distinct type ever + * scanned for the life of the process (kuri-bind#89). */ -internal class KotlinReflectMemberScanner : MemberScanner { +internal class KotlinReflectMemberScanner( + maxCacheSize: Int = DEFAULT_MAX_SCAN_CACHE_SIZE, +) : MemberScanner { // kotlin-reflect member discovery is the heavy path (memberProperties + memberFunctions + per- // property annotation-site probing) and a single compile scans a type several times — the root, // each complex `@Query`/`@Path` member via `hasBindingMembers`, and each `@Url`/`@Uri` merge type. // Memoize per type so each is scanned once; readers are instance-parameterized and stateless, so a // cached member list is safe to reuse across instances. - private val scanCache = ConcurrentHashMap, List>() + private val scanCache = BoundedCache, List>(maxCacheSize) override fun scan(type: KClass<*>): List = scanCache.getOrPut(type) { @@ -280,3 +285,8 @@ internal class KotlinReflectMemberScanner : MemberScanner { const val IS_PREFIX_LEN = 2 } } + +// Bounds the scan cache well above the number of distinct types any realistic caller scans in one +// process, while still capping the ClassLoader retention an unbounded cache would otherwise cause +// (kuri-bind#89). A caller with a genuinely larger working set only pays for a cache miss, not a leak. +private const val DEFAULT_MAX_SCAN_CACHE_SIZE = 2048 diff --git a/kuri-bind/src/jvmMain/kotlin/org/dexpace/kuri/bind/internal/PlanCompiler.kt b/kuri-bind/src/jvmMain/kotlin/org/dexpace/kuri/bind/internal/PlanCompiler.kt index b0815ba..7e33f34 100644 --- a/kuri-bind/src/jvmMain/kotlin/org/dexpace/kuri/bind/internal/PlanCompiler.kt +++ b/kuri-bind/src/jvmMain/kotlin/org/dexpace/kuri/bind/internal/PlanCompiler.kt @@ -15,7 +15,6 @@ import org.dexpace.kuri.bind.QueryMap import org.dexpace.kuri.bind.Scheme import org.dexpace.kuri.bind.UserInfo import org.dexpace.kuri.bind.Username -import java.util.concurrent.ConcurrentHashMap import kotlin.reflect.KClass import kotlin.reflect.full.findAnnotation import org.dexpace.kuri.bind.PathTemplate as PathTemplateAnn @@ -30,13 +29,19 @@ import org.dexpace.kuri.host.Host as KuriHost * The cache is profile-agnostic: a plan describes both a `Url` and a `Uri` binding, since the profile * only narrows which components project later. One [PlanCompiler] therefore backs every bind through * both profiles, and a type compiled for a URL bind is reused verbatim for a URI bind. Concurrent binds - * are safe: [planFor] memoizes through a [ConcurrentHashMap], and a rare double-compile under a race is + * are safe: [planFor] memoizes through a [BoundedCache], and a rare double-compile under a race is * harmless because compilation is pure and both racers produce an equal plan. + * + * The cache is capped at [maxCacheSize] entries (default [DEFAULT_MAX_PLAN_CACHE_SIZE]) rather than + * growing without bound: a `KClass` key strongly retains its `ClassLoader`, so an unbounded cache would + * pin every distinct type ever compiled — including nested value types routed through + * `runtimeValueIsBindable` — for the life of the process (kuri-bind#89). */ internal class PlanCompiler( private val scanner: MemberScanner, + maxCacheSize: Int = DEFAULT_MAX_PLAN_CACHE_SIZE, ) { - private val plans = ConcurrentHashMap, TypePlan>() + private val plans = BoundedCache, TypePlan>(maxCacheSize) /** * The compiled plan for [type], computing and caching it on first use (compile-once per type). @@ -303,3 +308,8 @@ private fun isCollectionType(type: KClass<*>): Boolean = Iterable::class.java.isAssignableFrom(type.java) || type.java.isArray private fun isMapType(type: KClass<*>): Boolean = Map::class.java.isAssignableFrom(type.java) + +// Bounds the plan cache well above the number of distinct root/nested types any realistic caller binds +// in one process, while still capping the ClassLoader retention an unbounded cache would otherwise cause +// (kuri-bind#89). A caller with a genuinely larger working set only pays for a cache miss, not a leak. +private const val DEFAULT_MAX_PLAN_CACHE_SIZE = 2048 diff --git a/kuri-bind/src/jvmTest/kotlin/org/dexpace/kuri/bind/internal/BoundedCacheTest.kt b/kuri-bind/src/jvmTest/kotlin/org/dexpace/kuri/bind/internal/BoundedCacheTest.kt new file mode 100644 index 0000000..cffe443 --- /dev/null +++ b/kuri-bind/src/jvmTest/kotlin/org/dexpace/kuri/bind/internal/BoundedCacheTest.kt @@ -0,0 +1,201 @@ +/* + * Copyright (c) 2026 dexpace and Omar Aljarrah + * SPDX-License-Identifier: MIT + */ +package org.dexpace.kuri.bind.internal + +import java.util.concurrent.ConcurrentLinkedQueue +import java.util.concurrent.CountDownLatch +import java.util.concurrent.CyclicBarrier +import java.util.concurrent.TimeUnit +import java.util.concurrent.atomic.AtomicInteger +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertFailsWith +import kotlin.test.assertFalse +import kotlin.test.assertSame +import kotlin.test.assertTrue + +class BoundedCacheTest { + @Test + fun `rejects a non-positive max size`() { + assertFailsWith { BoundedCache(0) } + assertFailsWith { BoundedCache(-1) } + } + + @Test + fun `caches the computed value for repeated lookups of the same key`() { + val cache = BoundedCache(maxSize = 4) + var computations = 0 + val compute: (Int) -> String = { key -> + computations++ + "value-$key" + } + + val first = cache.getOrPut(1, compute) + val second = cache.getOrPut(1, compute) + + assertEquals("value-1", first) + assertSame(first, second) + assertEquals(1, computations) + } + + @Test + fun `never grows past its configured max size`() { + val maxSize = 8 + val cache = BoundedCache(maxSize) + + for (key in 0 until maxSize * 4) { + cache.getOrPut(key) { "value-$it" } + assertTrue(cache.size <= maxSize, "cache size ${cache.size} exceeded max $maxSize after key $key") + } + assertEquals(maxSize, cache.size) + } + + @Test + fun `evicts the least-recently-used entry once it grows past the max size`() { + val cache = BoundedCache(maxSize = 3) + cache.getOrPut(1) { "one" } + cache.getOrPut(2) { "two" } + cache.getOrPut(3) { "three" } + + // A fresh 4th key evicts key 1 (the least recently used), not 2 or 3. + cache.getOrPut(4) { "four" } + + var recomputed = false + val one = + cache.getOrPut(1) { + recomputed = true + "one-recomputed" + } + assertTrue(recomputed, "expected the evicted key to be recomputed on its next lookup") + assertEquals("one-recomputed", one) + } + + @Test + fun `touching an entry protects it from eviction ahead of an untouched older entry`() { + val cache = BoundedCache(maxSize = 3) + cache.getOrPut(1) { "one" } + cache.getOrPut(2) { "two" } + cache.getOrPut(3) { "three" } + + // Re-reading key 1 marks it most-recently-used, so inserting key 4 evicts key 2 instead. + cache.getOrPut(1) { "one" } + cache.getOrPut(4) { "four" } + + var oneRecomputed = false + cache.getOrPut(1) { + oneRecomputed = true + "one-recomputed" + } + assertFalse(oneRecomputed, "key 1 should have survived eviction after being touched") + + var twoRecomputed = false + cache.getOrPut(2) { + twoRecomputed = true + "two-recomputed" + } + assertTrue(twoRecomputed, "key 2 should have been evicted in favor of the touched key 1") + } + + @Test + fun `a concurrent miss on the same key computes for every racer but retains only one result`() { + val cache = BoundedCache(maxSize = 4) + val threads = 16 + val results = arrayOfNulls(threads) + val computeCount = AtomicInteger(0) + val startBarrier = CyclicBarrier(threads) + // Gates every compute call until all `threads` racers have missed the cache and entered compute, + // so none can finish and insert before the rest have raced past the first check — otherwise a + // scheduling fluke could serialize the callers and the double-checked-locking discard path + // (BoundedCache.getOrPut's second `lock.withLock` re-check) would never actually be exercised. + val allComputingBarrier = CyclicBarrier(threads) + val workers = + (0 until threads).map { index -> + Thread { + startBarrier.await() + results[index] = + cache.getOrPut(42) { + computeCount.incrementAndGet() + allComputingBarrier.await() + "value-$index-${System.nanoTime()}" + } + } + } + workers.forEach { it.start() } + workers.forEach { it.join() } + + assertEquals(threads, computeCount.get(), "every racing caller must have missed the cache and computed") + val distinctResults = results.toSet() + assertEquals(1, distinctResults.size, "all racing callers must observe the same winning value") + assertSame(results[0], cache.getOrPut(42) { "should not run" }) + } + + @Test + fun `concurrent inserts of distinct keys past the cap evict safely without exceeding the max size`() { + val maxSize = 4 + val cache = BoundedCache(maxSize) + val threads = 16 + val keysPerThread = 64 + val startBarrier = CyclicBarrier(threads) + // A worker thread's own AssertionError/exception is never seen by the JUnit-driving main thread + // unless caught and relayed explicitly, so every failure is queued here and asserted after all + // workers join rather than asserted from inside the worker. + val failures = ConcurrentLinkedQueue() + val workers = + (0 until threads).map { threadIndex -> + Thread { + try { + startBarrier.await() + for (key in threadIndex until threads * keysPerThread step threads) { + cache.getOrPut(key) { "value-$it" } + check(cache.size <= maxSize) { "cache size ${cache.size} exceeded max $maxSize" } + } + } catch (t: Throwable) { + failures.add(t) + } + } + } + workers.forEach { it.start() } + workers.forEach { it.join() } + + assertTrue(failures.isEmpty(), "worker threads recorded failures: $failures") + assertEquals(maxSize, cache.size) + } + + @Test + fun `compute for one key does not block a concurrent getOrPut for a different key`() { + val cache = BoundedCache(maxSize = 4) + val enteredCompute = CountDownLatch(1) + val releaseCompute = CountDownLatch(1) + var firstResult: String? = null + + val blockedComputation = + Thread { + firstResult = + cache.getOrPut(1) { + enteredCompute.countDown() + releaseCompute.await(AWAIT_TIMEOUT_SECONDS, TimeUnit.SECONDS) + "one" + } + } + blockedComputation.start() + assertTrue( + enteredCompute.await(AWAIT_TIMEOUT_SECONDS, TimeUnit.SECONDS), + "the blocked thread must have entered compute for key 1", + ) + + // Key 1's compute is still parked; a different key must not be blocked by its held reference to + // the released lock, proving getOrPut's lock is not held across a caller's compute call. + val second = cache.getOrPut(2) { "two" } + assertEquals("two", second) + + releaseCompute.countDown() + blockedComputation.join(TimeUnit.SECONDS.toMillis(AWAIT_TIMEOUT_SECONDS)) + assertEquals("one", firstResult) + } + + private companion object { + const val AWAIT_TIMEOUT_SECONDS = 5L + } +} diff --git a/kuri-bind/src/jvmTest/kotlin/org/dexpace/kuri/bind/internal/MemberScannerCacheTest.kt b/kuri-bind/src/jvmTest/kotlin/org/dexpace/kuri/bind/internal/MemberScannerCacheTest.kt new file mode 100644 index 0000000..e34392a --- /dev/null +++ b/kuri-bind/src/jvmTest/kotlin/org/dexpace/kuri/bind/internal/MemberScannerCacheTest.kt @@ -0,0 +1,53 @@ +/* + * Copyright (c) 2026 dexpace and Omar Aljarrah + * SPDX-License-Identifier: MIT + */ +package org.dexpace.kuri.bind.internal + +import org.dexpace.kuri.bind.Host +import kotlin.test.Test +import kotlin.test.assertNotSame +import kotlin.test.assertSame + +// Five distinct types so a scanner capped at four cached entries is guaranteed to evict at least one. +private class ScanTarget1( + @Host val h: String, +) + +private class ScanTarget2( + @Host val h: String, +) + +private class ScanTarget3( + @Host val h: String, +) + +private class ScanTarget4( + @Host val h: String, +) + +private class ScanTarget5( + @Host val h: String, +) + +class MemberScannerCacheTest { + @Test + fun `scan cache is bounded and recomputes for a type evicted by newer lookups`() { + val scanner = KotlinReflectMemberScanner(maxCacheSize = 4) + + val first = scanner.scan(ScanTarget1::class) + // Repeated lookups of the same cached type return the exact same instance (a cache hit). + assertSame(first, scanner.scan(ScanTarget1::class)) + + // Scanning four more distinct types past the cap evicts ScanTarget1 (least recently used). + scanner.scan(ScanTarget2::class) + scanner.scan(ScanTarget3::class) + scanner.scan(ScanTarget4::class) + scanner.scan(ScanTarget5::class) + + // The cache is bounded: ScanTarget1's list had to be recomputed (a new instance), proving the + // earlier entry was evicted rather than retained forever. + val recomputed = scanner.scan(ScanTarget1::class) + assertNotSame(first, recomputed) + } +} diff --git a/kuri-bind/src/jvmTest/kotlin/org/dexpace/kuri/bind/internal/PlanCompilerCacheTest.kt b/kuri-bind/src/jvmTest/kotlin/org/dexpace/kuri/bind/internal/PlanCompilerCacheTest.kt new file mode 100644 index 0000000..8d03fe9 --- /dev/null +++ b/kuri-bind/src/jvmTest/kotlin/org/dexpace/kuri/bind/internal/PlanCompilerCacheTest.kt @@ -0,0 +1,53 @@ +/* + * Copyright (c) 2026 dexpace and Omar Aljarrah + * SPDX-License-Identifier: MIT + */ +package org.dexpace.kuri.bind.internal + +import org.dexpace.kuri.bind.Host +import kotlin.test.Test +import kotlin.test.assertNotSame +import kotlin.test.assertSame + +// Five distinct types so a compiler capped at four cached plans is guaranteed to evict at least one. +private class PlanTarget1( + @Host val h: String, +) + +private class PlanTarget2( + @Host val h: String, +) + +private class PlanTarget3( + @Host val h: String, +) + +private class PlanTarget4( + @Host val h: String, +) + +private class PlanTarget5( + @Host val h: String, +) + +class PlanCompilerCacheTest { + @Test + fun `plan cache is bounded and recompiles for a type evicted by newer lookups`() { + val compiler = PlanCompiler(KotlinReflectMemberScanner(), maxCacheSize = 4) + + val first = compiler.planFor(PlanTarget1::class) + // Repeated lookups of the same cached type return the exact same instance (a cache hit). + assertSame(first, compiler.planFor(PlanTarget1::class)) + + // Compiling four more distinct types past the cap evicts PlanTarget1 (least recently used). + compiler.planFor(PlanTarget2::class) + compiler.planFor(PlanTarget3::class) + compiler.planFor(PlanTarget4::class) + compiler.planFor(PlanTarget5::class) + + // The cache is bounded: PlanTarget1's plan had to be recompiled (a new instance), proving the + // earlier entry was evicted rather than retained forever — the fix for kuri-bind#89. + val recompiled = compiler.planFor(PlanTarget1::class) + assertNotSame(first, recompiled) + } +}