diff --git a/README.md b/README.md index fd57a3e..270bcfd 100644 --- a/README.md +++ b/README.md @@ -100,6 +100,44 @@ refresh dependencies with `./gradlew build --refresh-dependencies`. - The example modules contain working implementations for `RedisTemplate`, `ReactiveRedisTemplate` and the coroutine extensions. +### Local lock map size + +- The local lock (`isLocalLock = true`, the default) keeps one entry per `(cache key, lock type)` it is currently + locking, in a map shared by the whole JVM. A cache key that is both created and refreshed therefore uses up to two + entries. +- An entry is dropped as soon as the lock is released; an entry whose holder never released it is dropped by the + cleanup monitor once `lockTimeoutMillis` has passed, which runs on a 1 s interval and so can lag by up to that much. + The map's size therefore tracks how many keys are being locked **at the same time**, not how many distinct keys the + application sees. A service with 50 ms of supplier latency at 10k rps holds roughly 500 locks at once. +- The map is uncapped by default. To bound it, set `req-shield.lock.max-entries`; `0` (the default) means uncapped. + Size it against concurrent lock ownership, not key cardinality. + + ```yaml + # Spring modules: read from the Environment, so application.yml works + req-shield: + lock: + max-entries: 2000 + ``` + + ```bash + # core / core-reactor / core-kotlin-coroutine used directly: system property + java -Dreq-shield.lock.max-entries=2000 -jar app.jar + ``` + + The Spring modules read the same key from the `Environment`, so a `-D` override still outranks the yml entry there, + and `REQ_SHIELD_LOCK_MAX_ENTRIES` works as an environment variable. +- Once the map is full, a request for a **new** key is handed a permit that no map entry backs. It then runs exactly as + if it had taken the lock - it calls the supplier and writes the cache - so what the cap costs is request collapsing + for that key, and nothing else: no added latency, and the cache still gets populated. Keys whose entry is already in + the map are not subject to the cap at all. +- The cap is a soft one. The size check is an estimate and is not atomic with the insertion it guards, so concurrent + callers can push the map slightly past the configured number. +- The first refusal and every 1000th after it are logged at WARN. +- A value that cannot be read as a non-negative number is logged at WARN and ignored, leaving the current cap + unchanged. This is deliberate in both directions: a class initializer that throws would poison the library for the + whole JVM, and an application context should not fail to start over a tuning knob typo. Setting the cap + programmatically (`LocalLockLimit.maxEntries = -1`) still fails fast, because there the stack trace is actionable. + ### Cache eviction semantics - `@ReqShieldCacheEvict` evicts **after** the annotated method completes successfully (Spring's `@CacheEvict` default). diff --git a/core-kotlin-coroutine/src/main/kotlin/com/linecorp/cse/reqshield/kotlin/coroutine/KeyLocalLock.kt b/core-kotlin-coroutine/src/main/kotlin/com/linecorp/cse/reqshield/kotlin/coroutine/KeyLocalLock.kt index 9439fcc..0694dae 100644 --- a/core-kotlin-coroutine/src/main/kotlin/com/linecorp/cse/reqshield/kotlin/coroutine/KeyLocalLock.kt +++ b/core-kotlin-coroutine/src/main/kotlin/com/linecorp/cse/reqshield/kotlin/coroutine/KeyLocalLock.kt @@ -16,6 +16,7 @@ package com.linecorp.cse.reqshield.kotlin.coroutine +import com.linecorp.cse.reqshield.support.config.LocalLockLimit import com.linecorp.cse.reqshield.support.constant.ConfigValues.LOCK_MONITOR_INTERVAL_MILLIS import com.linecorp.cse.reqshield.support.utils.nowToEpochTime import kotlinx.coroutines.CancellationException @@ -55,9 +56,11 @@ class KeyLocalLock(private val lockTimeoutMillis: Long) : KeyLock, CoroutineScop */ val isHeld: AtomicBoolean = AtomicBoolean(false), /** - * Token of the current owner, or null when the lock is not held. + * Token of the current owner. * Only the owner that presents this exact token may release the lock, so a holder whose * lock already expired and was reacquired by somebody else cannot release the new owner. + * It is null only while the entry is being built inside compute(): unLock and the monitor + * drop the whole entry instead of clearing the token, so every entry in the map is held. */ @Volatile var token: String? = null, ) @@ -171,6 +174,15 @@ class KeyLocalLock(private val lockTimeoutMillis: Long) : KeyLock, CoroutineScop } existing } else { + // At the cap, hand out a permit that no map entry backs instead of refusing. + // The caller then takes its normal path and writes the cache; making it wait + // would stall it on a holder that does not exist. Only this key's collapsing + // is lost, and the later unLock simply finds nothing to release. + if (LocalLockLimit.rejectsNewEntry(lockMap.mappingCount())) { + acquiredToken.set(nextToken()) + return@compute null + } + // New entry: create and acquire val token = nextToken() val newLock = LockInfo(Semaphore(1), now + lockTimeoutMillis, token = token) @@ -192,16 +204,19 @@ class KeyLocalLock(private val lockTimeoutMillis: Long) : KeyLock, CoroutineScop val released = AtomicBoolean(false) // Release inside compute() so that it is atomic with respect to acquisition and cleanup - // of the same key: the token check and the semaphore release cannot be interleaved with a - // reacquisition. The entry itself is kept so a waiting caller can still acquire it. + // of the same key: the token check, the semaphore release and the removal cannot be + // interleaved with a reacquisition. lockMap.compute(completeKey) { _, existing -> if (existing == null) return@compute null // Only the current owner may release: a stale token belongs to an expired holder. if (existing.token == token && existing.isHeld.compareAndSet(true, false)) { - existing.token = null existing.semaphore.release() released.set(true) + // Drop the entry immediately rather than leaving it for the monitor. No reference + // to it escapes this lambda, and the next tryLock() just creates a fresh one, so + // keeping it would only pin the key until its lockTimeoutMillis runs out. + return@compute null } existing } diff --git a/core-kotlin-coroutine/src/test/kotlin/com/linecorp/cse/reqshield/kotlin/coroutine/KeyLocalLockTest.kt b/core-kotlin-coroutine/src/test/kotlin/com/linecorp/cse/reqshield/kotlin/coroutine/KeyLocalLockTest.kt index dcef50b..f4289ad 100644 --- a/core-kotlin-coroutine/src/test/kotlin/com/linecorp/cse/reqshield/kotlin/coroutine/KeyLocalLockTest.kt +++ b/core-kotlin-coroutine/src/test/kotlin/com/linecorp/cse/reqshield/kotlin/coroutine/KeyLocalLockTest.kt @@ -17,6 +17,8 @@ package com.linecorp.cse.reqshield.kotlin.coroutine import com.linecorp.cse.reqshield.support.BaseKeyLockTest +import com.linecorp.cse.reqshield.support.config.LocalLockLimit +import com.linecorp.cse.reqshield.support.constant.ConfigValues.UNLIMITED_LOCK_ENTRIES import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.async import kotlinx.coroutines.awaitAll @@ -393,5 +395,114 @@ class KeyLocalLockTest : BaseKeyLockTest { keyLock.cancel() } + /** + * The lock map is private to the companion, so reflection is the only way to observe that + * unLock really drops the entry rather than leaving it for the expiry monitor. + */ + @Suppress("UNCHECKED_CAST") + private fun readLockMap(): Map { + // The companion's private val is compiled as a static field on the outer class. + val field = KeyLocalLock::class.java.getDeclaredField("lockMap") + field.isAccessible = true + return field.get(null) as Map + } + + @Test + fun `unLock removes the map entry right away instead of leaving it until expiry`() = + runBlocking { + // A long lock timeout rules the monitor out as the remover: were the entry kept by + // unLock, it would still be in the map for the next 60 seconds. + val keyLock = KeyLocalLock(60_000L) + val key = "unlock-removal-test-${java.util.UUID.randomUUID()}" + val mapKey = lockKeyOf(key, LockType.CREATE) + + val token = assertNotNull(keyLock.tryLock(key, LockType.CREATE)) + assertTrue(readLockMap().containsKey(mapKey), "A held lock must have an entry in the map") + + assertTrue(keyLock.unLock(key, LockType.CREATE, token)) + assertFalse(readLockMap().containsKey(mapKey), "unLock must drop the entry, not wait for the monitor") + + keyLock.cancel() + } + + @Test + fun `at the cap tryLock still grants a permit but stops adding map entries`() = + runBlocking { + val keyLock = KeyLocalLock(60_000L) + val first = "cap-test-first-${java.util.UUID.randomUUID()}" + val second = "cap-test-second-${java.util.UUID.randomUUID()}" + + // Taken while still uncapped, so it always succeeds. Its 60s timeout keeps the entry + // in the map for the rest of the test, which is what makes a cap of one deterministic + // here: the map can only grow from this point, never shrink below one. + val firstToken = assertNotNull(keyLock.tryLock(first, LockType.CREATE)) + assertTrue(readLockMap().containsKey(lockKeyOf(first, LockType.CREATE))) + + LocalLockLimit.maxEntries = 1 + try { + val secondToken = + assertNotNull( + keyLock.tryLock(second, LockType.CREATE), + "past the cap the caller must still get a permit, so it writes the cache " + + "instead of waiting for a holder that does not exist", + ) + assertFalse( + readLockMap().containsKey(lockKeyOf(second, LockType.CREATE)), + "a permit handed out past the cap must not add a map entry", + ) + assertFalse( + keyLock.unLock(second, LockType.CREATE, secondToken), + "the permit is backed by no entry, so releasing it finds nothing", + ) + + // Losing collapsing for that key is exactly what the cap costs. + assertNotNull( + keyLock.tryLock(second, LockType.CREATE), + "past the cap a second caller for the same key is not collapsed either", + ) + } finally { + LocalLockLimit.maxEntries = UNLIMITED_LOCK_ENTRIES + } + + keyLock.unLock(first, LockType.CREATE, firstToken) + keyLock.cancel() + } + + /** + * Only this module and the reactor one can pin this: reaching the existing-entry branch needs + * an expired entry that nothing sweeps, which means stopping the monitor. The core module has + * no equivalent hook, so the same case stays uncovered there. + */ + @Test + fun `an entry already in the map is reacquired even when the map is at its cap`() = + runBlocking { + // Construct both locks first - each constructor restarts the monitor - then stop it, + // so the expired entry below survives for the rest of the test. + val shortLock = KeyLocalLock(1L) + val longLock = KeyLocalLock(60_000L) + KeyLocalLock.stopMonitoring() + + val key = "cap-existing-${java.util.UUID.randomUUID()}" + assertNotNull(shortLock.tryLock(key, LockType.CREATE)) + delay(20L) // the lock has timed out, and with the monitor stopped nothing removes it + + LocalLockLimit.maxEntries = readLockMap().size.toLong() // exactly full + try { + val reacquired = + assertNotNull( + longLock.tryLock(key, LockType.CREATE), + "an entry already in the map must not be subject to the cap", + ) + // A real lock, not a cap permit: it excludes the next caller and releases cleanly. + assertNull(longLock.tryLock(key, LockType.CREATE)) + assertTrue(longLock.unLock(key, LockType.CREATE, reacquired)) + } finally { + LocalLockLimit.maxEntries = UNLIMITED_LOCK_ENTRIES + } + + shortLock.cancel() + longLock.cancel() + } + private suspend fun doWork() = delay(1000) } diff --git a/core-reactor/src/main/kotlin/com/linecorp/cse/reqshield/reactor/KeyLocalLock.kt b/core-reactor/src/main/kotlin/com/linecorp/cse/reqshield/reactor/KeyLocalLock.kt index b5aa768..71949a2 100644 --- a/core-reactor/src/main/kotlin/com/linecorp/cse/reqshield/reactor/KeyLocalLock.kt +++ b/core-reactor/src/main/kotlin/com/linecorp/cse/reqshield/reactor/KeyLocalLock.kt @@ -16,6 +16,7 @@ package com.linecorp.cse.reqshield.reactor +import com.linecorp.cse.reqshield.support.config.LocalLockLimit import com.linecorp.cse.reqshield.support.constant.ConfigValues.LOCK_KEY_PREFIX import com.linecorp.cse.reqshield.support.constant.ConfigValues.LOCK_MONITOR_INTERVAL_MILLIS import com.linecorp.cse.reqshield.support.utils.nowToEpochTime @@ -56,9 +57,11 @@ class KeyLocalLock( */ val isHeld: AtomicBoolean = AtomicBoolean(false), /** - * Ownership token of the current holder, null when the lock is not held. + * Ownership token of the current holder. * Only the holder that owns this token may release the lock, so a holder whose * lock already expired cannot release the lock of the next holder. + * It is null only while the entry is being built inside compute(): unLock and the monitor + * drop the whole entry instead of clearing the token, so every entry in the map is held. */ @Volatile var token: String? = null, ) @@ -186,6 +189,15 @@ class KeyLocalLock( } existing } else { + // At the cap, hand out a permit that no map entry backs instead of refusing. + // The caller then takes its normal path and writes the cache; making it wait + // would stall it on a holder that does not exist. Only this key's collapsing + // is lost, and the later unLock simply finds nothing to release. + if (LocalLockLimit.rejectsNewEntry(lockMap.mappingCount())) { + acquiredToken.set(nextToken()) + return@compute null + } + // New entry: create and acquire val token = nextToken() val newLock = LockInfo(Semaphore(1), now + lockTimeoutMillis) @@ -209,19 +221,23 @@ class KeyLocalLock( val completeKey = completeKey(key, lockType) val released = AtomicBoolean(false) - // Release inside compute() so that it is atomic with acquisition and cleanup: - // no other thread can reacquire this key while the ownership check runs. + // Release inside compute() so that the ownership check, the release and the removal + // are atomic with acquisition and cleanup: no other thread can reacquire this key + // while the ownership check runs. lockMap.compute(completeKey) { _, existing -> if (existing == null) return@compute null if (existing.token == token && existing.isHeld.compareAndSet(true, false)) { existing.semaphore.release() - existing.token = null released.set(true) - } else { - log.debug("Attempted to unlock key '{}' without holding its current token", completeKey) + // Drop the entry immediately rather than leaving it for the monitor. No + // reference to it escapes this lambda, and the next tryLock() just creates a + // fresh one, so keeping it would only pin the key until its timeout runs out. + return@compute null } - existing // Keep the entry + + log.debug("Attempted to unlock key '{}' without holding its current token", completeKey) + existing } released.get() } diff --git a/core-reactor/src/test/kotlin/com/linecorp/cse/reqshield/reactor/KeyLocalLockTest.kt b/core-reactor/src/test/kotlin/com/linecorp/cse/reqshield/reactor/KeyLocalLockTest.kt index 37df328..c3e71e0 100644 --- a/core-reactor/src/test/kotlin/com/linecorp/cse/reqshield/reactor/KeyLocalLockTest.kt +++ b/core-reactor/src/test/kotlin/com/linecorp/cse/reqshield/reactor/KeyLocalLockTest.kt @@ -17,8 +17,12 @@ package com.linecorp.cse.reqshield.reactor import com.linecorp.cse.reqshield.support.BaseKeyLockTest +import com.linecorp.cse.reqshield.support.config.LocalLockLimit +import com.linecorp.cse.reqshield.support.constant.ConfigValues.LOCK_KEY_PREFIX +import com.linecorp.cse.reqshield.support.constant.ConfigValues.UNLIMITED_LOCK_ENTRIES import org.junit.jupiter.api.AfterEach import org.junit.jupiter.api.Assertions.assertEquals +import org.junit.jupiter.api.Assertions.assertFalse import org.junit.jupiter.api.Assertions.assertNotEquals import org.junit.jupiter.api.Assertions.assertTrue import org.junit.jupiter.api.Test @@ -304,6 +308,116 @@ class KeyLocalLockTest : BaseKeyLockTest { assertEquals(1, successfulAcquisitions.get(), "Only one should acquire the lock") } + /** + * The lock map is private to the companion, so reflection is the only way to observe that + * unLock really drops the entry rather than leaving it for the expiry monitor. + */ + @Suppress("UNCHECKED_CAST") + private fun readLockMap(): Map { + // The companion's private val is compiled as a static field on the outer class. + val field = KeyLocalLock::class.java.getDeclaredField("lockMap") + field.isAccessible = true + return field.get(null) as Map + } + + /** Mirrors KeyLocalLock's private completeKey so the test can look up the same map key. */ + private fun lockMapKeyOf( + key: String, + lockType: LockType, + ) = "$LOCK_KEY_PREFIX${key}_${lockType.name}" + + @Test + fun `unLock removes the map entry right away instead of leaving it until expiry`() { + // A long lock timeout rules the monitor out as the remover: were the entry kept by unLock, + // it would still be in the map for the next 60 seconds. + val keyLock = KeyLocalLock(60_000L) + val key = "unlock-removal-test-${java.util.UUID.randomUUID()}" + val mapKey = lockMapKeyOf(key, LockType.CREATE) + + val token = keyLock.tryLock(key, LockType.CREATE).block() + assertNotNull(token) + assertTrue(readLockMap().containsKey(mapKey), "A held lock must have an entry in the map") + + StepVerifier.create(keyLock.unLock(key, LockType.CREATE, token)).expectNext(true).verifyComplete() + assertFalse(readLockMap().containsKey(mapKey), "unLock must drop the entry, not wait for the monitor") + } + + @Test + fun `at the cap tryLock still grants a permit but stops adding map entries`() { + val keyLock = KeyLocalLock(60_000L) + val first = "cap-test-first-${java.util.UUID.randomUUID()}" + val second = "cap-test-second-${java.util.UUID.randomUUID()}" + + // Taken while still uncapped, so it always succeeds. Its 60s timeout keeps the entry in + // the map for the rest of the test, which is what makes a cap of one deterministic here: + // the map can only grow from this point, never shrink below one. + val firstToken = assertNotNull(keyLock.tryLock(first, LockType.CREATE).block()) + assertTrue(readLockMap().containsKey(lockMapKeyOf(first, LockType.CREATE))) + + LocalLockLimit.maxEntries = 1 + try { + val secondToken = + assertNotNull( + keyLock.tryLock(second, LockType.CREATE).block(), + "past the cap the caller must still get a permit, so it writes the cache " + + "instead of waiting for a holder that does not exist", + ) + assertFalse( + readLockMap().containsKey(lockMapKeyOf(second, LockType.CREATE)), + "a permit handed out past the cap must not add a map entry", + ) + StepVerifier + .create(keyLock.unLock(second, LockType.CREATE, secondToken)) + .expectNext(false) + .verifyComplete() + + // Losing collapsing for that key is exactly what the cap costs. + assertNotNull( + keyLock.tryLock(second, LockType.CREATE).block(), + "past the cap a second caller for the same key is not collapsed either", + ) + } finally { + LocalLockLimit.maxEntries = UNLIMITED_LOCK_ENTRIES + } + + keyLock.unLock(first, LockType.CREATE, firstToken).block() + } + + /** + * Only this module and the coroutine one can pin this: reaching the existing-entry branch + * needs an expired entry that nothing sweeps, which means stopping the monitor. The core + * module has no equivalent hook, so the same case stays uncovered there. + */ + @Test + fun `an entry already in the map is reacquired even when the map is at its cap`() { + // Construct both locks first - each constructor restarts the monitor - then stop it, so + // the expired entry below survives for the rest of the test. + val shortLock = KeyLocalLock(1L) + val longLock = KeyLocalLock(60_000L) + KeyLocalLock.stopMonitoring() + + val key = "cap-existing-${java.util.UUID.randomUUID()}" + assertNotNull(shortLock.tryLock(key, LockType.CREATE).block()) + Thread.sleep(20L) // the lock has timed out, and with the monitor stopped nothing removes it + + LocalLockLimit.maxEntries = readLockMap().size.toLong() // exactly full + try { + val reacquired = + assertNotNull( + longLock.tryLock(key, LockType.CREATE).block(), + "an entry already in the map must not be subject to the cap", + ) + // A real lock, not a cap permit: it excludes the next caller and releases cleanly. + StepVerifier.create(longLock.tryLock(key, LockType.CREATE)).verifyComplete() + StepVerifier + .create(longLock.unLock(key, LockType.CREATE, reacquired)) + .expectNext(true) + .verifyComplete() + } finally { + LocalLockLimit.maxEntries = UNLIMITED_LOCK_ENTRIES + } + } + private fun doWork(): Mono = Mono .delay(Duration.ofSeconds(1)) diff --git a/core-spring-webflux-kotlin-coroutine/src/main/kotlin/com/linecorp/cse/reqshield/spring/webflux/kotlin/coroutine/config/LibAutoConfiguration.kt b/core-spring-webflux-kotlin-coroutine/src/main/kotlin/com/linecorp/cse/reqshield/spring/webflux/kotlin/coroutine/config/LibAutoConfiguration.kt index 9f9a4ee..d84cc55 100644 --- a/core-spring-webflux-kotlin-coroutine/src/main/kotlin/com/linecorp/cse/reqshield/spring/webflux/kotlin/coroutine/config/LibAutoConfiguration.kt +++ b/core-spring-webflux-kotlin-coroutine/src/main/kotlin/com/linecorp/cse/reqshield/spring/webflux/kotlin/coroutine/config/LibAutoConfiguration.kt @@ -17,6 +17,8 @@ package com.linecorp.cse.reqshield.spring.webflux.kotlin.coroutine.config import com.linecorp.cse.reqshield.spring.webflux.kotlin.coroutine.aspect.ReqShieldAspect +import com.linecorp.cse.reqshield.support.config.LocalLockLimit +import com.linecorp.cse.reqshield.support.constant.ConfigValues.MAX_LOCK_ENTRIES_PROPERTY import kotlinx.coroutines.CoroutineExceptionHandler import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers @@ -28,12 +30,24 @@ import org.springframework.context.annotation.Bean import org.springframework.context.annotation.Configuration import org.springframework.context.annotation.EnableAspectJAutoProxy import org.springframework.context.annotation.Import +import org.springframework.core.env.Environment import kotlin.coroutines.CoroutineContext @Configuration @EnableAspectJAutoProxy @Import(ReqShieldAspect::class) -open class LibAutoConfiguration { +open class LibAutoConfiguration( + environment: Environment, +) { + init { + // The lock map is static, so the cap has to be pushed onto it once at startup. Reading it + // from the Environment rather than a system property lets application.yml carry the value; + // the Environment still ranks a -D override above the yml entry. The raw String is handed + // to LocalLockLimit so that a malformed value is ignored with a warning here too, instead + // of failing the context refresh the way Environment's own Long conversion would. + LocalLockLimit.applyConfiguredValue(environment.getProperty(MAX_LOCK_ENTRIES_PROPERTY)) + } + /** * Scope shared by every [com.linecorp.cse.reqshield.kotlin.coroutine.ReqShield] the aspect * creates, used for the fire-and-forget cache writes. diff --git a/core-spring-webflux-kotlin-coroutine/src/test/kotlin/com/linecorp/cse/reqshield/spring/webflux/kotlin/coroutine/config/LibAutoConfigurationLockLimitTest.kt b/core-spring-webflux-kotlin-coroutine/src/test/kotlin/com/linecorp/cse/reqshield/spring/webflux/kotlin/coroutine/config/LibAutoConfigurationLockLimitTest.kt new file mode 100644 index 0000000..d94bb7f --- /dev/null +++ b/core-spring-webflux-kotlin-coroutine/src/test/kotlin/com/linecorp/cse/reqshield/spring/webflux/kotlin/coroutine/config/LibAutoConfigurationLockLimitTest.kt @@ -0,0 +1,88 @@ +/* + * Copyright 2024 LY Corporation + * + * LY Corporation licenses this file to you under the Apache License, + * version 2.0 (the "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at: + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. + */ + +package com.linecorp.cse.reqshield.spring.webflux.kotlin.coroutine.config + +import com.linecorp.cse.reqshield.support.config.LocalLockLimit +import com.linecorp.cse.reqshield.support.constant.ConfigValues.MAX_LOCK_ENTRIES_PROPERTY +import com.linecorp.cse.reqshield.support.constant.ConfigValues.UNLIMITED_LOCK_ENTRIES +import org.junit.jupiter.api.AfterEach +import org.junit.jupiter.api.Assertions.assertEquals +import org.junit.jupiter.api.Test +import org.springframework.core.env.Environment +import org.springframework.core.env.MapPropertySource +import org.springframework.core.env.StandardEnvironment + +/** + * The lock map is static, so [LibAutoConfiguration] is what carries a configured cap onto it. + * These cover the binding itself; that Spring can build the class at all is already proven by the + * integration tests, which register it in a real context. + */ +class LibAutoConfigurationLockLimitTest { + @AfterEach + fun restoreDefault() { + LocalLockLimit.maxEntries = UNLIMITED_LOCK_ENTRIES + } + + private fun environmentWith(vararg properties: Pair): Environment = + StandardEnvironment().apply { + propertySources.addFirst(MapPropertySource("test", mapOf(*properties))) + } + + @Test + fun `binds the lock entry cap from the environment`() { + LibAutoConfiguration(environmentWith(MAX_LOCK_ENTRIES_PROPERTY to "5000")) + + assertEquals(5_000L, LocalLockLimit.maxEntries) + } + + @Test + fun `leaves the cap alone when the property is absent`() { + LocalLockLimit.maxEntries = 42 + + LibAutoConfiguration(StandardEnvironment()) + + assertEquals(42L, LocalLockLimit.maxEntries, "an absent property must not reset a cap set elsewhere") + } + + @Test + fun `an explicit zero leaves the map uncapped`() { + LocalLockLimit.maxEntries = 42 + + LibAutoConfiguration(environmentWith(MAX_LOCK_ENTRIES_PROPERTY to "0")) + + assertEquals(UNLIMITED_LOCK_ENTRIES, LocalLockLimit.maxEntries) + } + + @Test + fun `a yml integer is bound just like a quoted string`() { + // application.yml hands `max-entries: 5000` over as an Integer, not a String. + LibAutoConfiguration(environmentWith(MAX_LOCK_ENTRIES_PROPERTY to 5000)) + + assertEquals(5_000L, LocalLockLimit.maxEntries) + } + + @Test + fun `an unusable cap is ignored instead of bringing the context down`() { + LocalLockLimit.maxEntries = 42 + + LibAutoConfiguration(environmentWith(MAX_LOCK_ENTRIES_PROPERTY to "not-a-number")) + assertEquals(42L, LocalLockLimit.maxEntries, "a malformed value must leave the cap alone") + + LibAutoConfiguration(environmentWith(MAX_LOCK_ENTRIES_PROPERTY to "-1")) + assertEquals(42L, LocalLockLimit.maxEntries, "a negative value must leave the cap alone") + } +} diff --git a/core-spring-webflux/src/main/kotlin/com/linecorp/cse/reqshield/spring/webflux/config/LibAutoConfiguration.kt b/core-spring-webflux/src/main/kotlin/com/linecorp/cse/reqshield/spring/webflux/config/LibAutoConfiguration.kt index c43bbe9..6978632 100644 --- a/core-spring-webflux/src/main/kotlin/com/linecorp/cse/reqshield/spring/webflux/config/LibAutoConfiguration.kt +++ b/core-spring-webflux/src/main/kotlin/com/linecorp/cse/reqshield/spring/webflux/config/LibAutoConfiguration.kt @@ -17,17 +17,31 @@ package com.linecorp.cse.reqshield.spring.webflux.config import com.linecorp.cse.reqshield.spring.webflux.aspect.ReqShieldAspect +import com.linecorp.cse.reqshield.support.config.LocalLockLimit +import com.linecorp.cse.reqshield.support.constant.ConfigValues.MAX_LOCK_ENTRIES_PROPERTY import org.springframework.context.annotation.Bean import org.springframework.context.annotation.Configuration import org.springframework.context.annotation.EnableAspectJAutoProxy import org.springframework.context.annotation.Import +import org.springframework.core.env.Environment import reactor.core.scheduler.Scheduler import reactor.core.scheduler.Schedulers @Configuration @EnableAspectJAutoProxy @Import(ReqShieldAspect::class) -open class LibAutoConfiguration { +open class LibAutoConfiguration( + environment: Environment, +) { + init { + // The lock map is static, so the cap has to be pushed onto it once at startup. Reading it + // from the Environment rather than a system property lets application.yml carry the value; + // the Environment still ranks a -D override above the yml entry. The raw String is handed + // to LocalLockLimit so that a malformed value is ignored with a warning here too, instead + // of failing the context refresh the way Environment's own Long conversion would. + LocalLockLimit.applyConfiguredValue(environment.getProperty(MAX_LOCK_ENTRIES_PROPERTY)) + } + /** * Scheduler shared by every [com.linecorp.cse.reqshield.reactor.ReqShield] the aspect creates, used for * the asynchronous cache writes and for polling the cache while another request holds the lock. diff --git a/core-spring-webflux/src/test/kotlin/com/linecorp/cse/reqshield/spring/webflux/config/LibAutoConfigurationLockLimitTest.kt b/core-spring-webflux/src/test/kotlin/com/linecorp/cse/reqshield/spring/webflux/config/LibAutoConfigurationLockLimitTest.kt new file mode 100644 index 0000000..7874565 --- /dev/null +++ b/core-spring-webflux/src/test/kotlin/com/linecorp/cse/reqshield/spring/webflux/config/LibAutoConfigurationLockLimitTest.kt @@ -0,0 +1,88 @@ +/* + * Copyright 2024 LY Corporation + * + * LY Corporation licenses this file to you under the Apache License, + * version 2.0 (the "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at: + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. + */ + +package com.linecorp.cse.reqshield.spring.webflux.config + +import com.linecorp.cse.reqshield.support.config.LocalLockLimit +import com.linecorp.cse.reqshield.support.constant.ConfigValues.MAX_LOCK_ENTRIES_PROPERTY +import com.linecorp.cse.reqshield.support.constant.ConfigValues.UNLIMITED_LOCK_ENTRIES +import org.junit.jupiter.api.AfterEach +import org.junit.jupiter.api.Assertions.assertEquals +import org.junit.jupiter.api.Test +import org.springframework.core.env.Environment +import org.springframework.core.env.MapPropertySource +import org.springframework.core.env.StandardEnvironment + +/** + * The lock map is static, so [LibAutoConfiguration] is what carries a configured cap onto it. + * These cover the binding itself; that Spring can build the class at all is already proven by the + * integration tests, which register it in a real context. + */ +class LibAutoConfigurationLockLimitTest { + @AfterEach + fun restoreDefault() { + LocalLockLimit.maxEntries = UNLIMITED_LOCK_ENTRIES + } + + private fun environmentWith(vararg properties: Pair): Environment = + StandardEnvironment().apply { + propertySources.addFirst(MapPropertySource("test", mapOf(*properties))) + } + + @Test + fun `binds the lock entry cap from the environment`() { + LibAutoConfiguration(environmentWith(MAX_LOCK_ENTRIES_PROPERTY to "5000")) + + assertEquals(5_000L, LocalLockLimit.maxEntries) + } + + @Test + fun `leaves the cap alone when the property is absent`() { + LocalLockLimit.maxEntries = 42 + + LibAutoConfiguration(StandardEnvironment()) + + assertEquals(42L, LocalLockLimit.maxEntries, "an absent property must not reset a cap set elsewhere") + } + + @Test + fun `an explicit zero leaves the map uncapped`() { + LocalLockLimit.maxEntries = 42 + + LibAutoConfiguration(environmentWith(MAX_LOCK_ENTRIES_PROPERTY to "0")) + + assertEquals(UNLIMITED_LOCK_ENTRIES, LocalLockLimit.maxEntries) + } + + @Test + fun `a yml integer is bound just like a quoted string`() { + // application.yml hands `max-entries: 5000` over as an Integer, not a String. + LibAutoConfiguration(environmentWith(MAX_LOCK_ENTRIES_PROPERTY to 5000)) + + assertEquals(5_000L, LocalLockLimit.maxEntries) + } + + @Test + fun `an unusable cap is ignored instead of bringing the context down`() { + LocalLockLimit.maxEntries = 42 + + LibAutoConfiguration(environmentWith(MAX_LOCK_ENTRIES_PROPERTY to "not-a-number")) + assertEquals(42L, LocalLockLimit.maxEntries, "a malformed value must leave the cap alone") + + LibAutoConfiguration(environmentWith(MAX_LOCK_ENTRIES_PROPERTY to "-1")) + assertEquals(42L, LocalLockLimit.maxEntries, "a negative value must leave the cap alone") + } +} diff --git a/core-spring/src/main/kotlin/com/linecorp/cse/reqshield/spring/config/LibAutoConfiguration.kt b/core-spring/src/main/kotlin/com/linecorp/cse/reqshield/spring/config/LibAutoConfiguration.kt index 7c5af37..27bb656 100644 --- a/core-spring/src/main/kotlin/com/linecorp/cse/reqshield/spring/config/LibAutoConfiguration.kt +++ b/core-spring/src/main/kotlin/com/linecorp/cse/reqshield/spring/config/LibAutoConfiguration.kt @@ -17,10 +17,13 @@ package com.linecorp.cse.reqshield.spring.config import com.linecorp.cse.reqshield.spring.aspect.ReqShieldAspect +import com.linecorp.cse.reqshield.support.config.LocalLockLimit +import com.linecorp.cse.reqshield.support.constant.ConfigValues.MAX_LOCK_ENTRIES_PROPERTY import org.springframework.context.annotation.Bean import org.springframework.context.annotation.Configuration import org.springframework.context.annotation.EnableAspectJAutoProxy import org.springframework.context.annotation.Import +import org.springframework.core.env.Environment import java.util.concurrent.ExecutorService import java.util.concurrent.Executors import java.util.concurrent.atomic.AtomicLong @@ -28,7 +31,18 @@ import java.util.concurrent.atomic.AtomicLong @Configuration @EnableAspectJAutoProxy @Import(ReqShieldAspect::class) -open class LibAutoConfiguration { +open class LibAutoConfiguration( + environment: Environment, +) { + init { + // The lock map is static, so the cap has to be pushed onto it once at startup. Reading it + // from the Environment rather than a system property lets application.yml carry the value; + // the Environment still ranks a -D override above the yml entry. The raw String is handed + // to LocalLockLimit so that a malformed value is ignored with a warning here too, instead + // of failing the context refresh the way Environment's own Long conversion would. + LocalLockLimit.applyConfiguredValue(environment.getProperty(MAX_LOCK_ENTRIES_PROPERTY)) + } + /** * Pool shared by every [com.linecorp.cse.reqshield.ReqShield] the aspect creates, used for the * asynchronous cache writes. diff --git a/core-spring/src/test/kotlin/config/LibAutoConfigurationLockLimitTest.kt b/core-spring/src/test/kotlin/config/LibAutoConfigurationLockLimitTest.kt new file mode 100644 index 0000000..ef282bd --- /dev/null +++ b/core-spring/src/test/kotlin/config/LibAutoConfigurationLockLimitTest.kt @@ -0,0 +1,89 @@ +/* + * Copyright 2024 LY Corporation + * + * LY Corporation licenses this file to you under the Apache License, + * version 2.0 (the "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at: + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. + */ + +package config + +import com.linecorp.cse.reqshield.spring.config.LibAutoConfiguration +import com.linecorp.cse.reqshield.support.config.LocalLockLimit +import com.linecorp.cse.reqshield.support.constant.ConfigValues.MAX_LOCK_ENTRIES_PROPERTY +import com.linecorp.cse.reqshield.support.constant.ConfigValues.UNLIMITED_LOCK_ENTRIES +import org.junit.jupiter.api.AfterEach +import org.junit.jupiter.api.Assertions.assertEquals +import org.junit.jupiter.api.Test +import org.springframework.core.env.Environment +import org.springframework.core.env.MapPropertySource +import org.springframework.core.env.StandardEnvironment + +/** + * The lock map is static, so [LibAutoConfiguration] is what carries a configured cap onto it. + * These cover the binding itself; that Spring can build the class at all is already proven by the + * integration tests, which register it in a real context. + */ +class LibAutoConfigurationLockLimitTest { + @AfterEach + fun restoreDefault() { + LocalLockLimit.maxEntries = UNLIMITED_LOCK_ENTRIES + } + + private fun environmentWith(vararg properties: Pair): Environment = + StandardEnvironment().apply { + propertySources.addFirst(MapPropertySource("test", mapOf(*properties))) + } + + @Test + fun `binds the lock entry cap from the environment`() { + LibAutoConfiguration(environmentWith(MAX_LOCK_ENTRIES_PROPERTY to "5000")) + + assertEquals(5_000L, LocalLockLimit.maxEntries) + } + + @Test + fun `leaves the cap alone when the property is absent`() { + LocalLockLimit.maxEntries = 42 + + LibAutoConfiguration(StandardEnvironment()) + + assertEquals(42L, LocalLockLimit.maxEntries, "an absent property must not reset a cap set elsewhere") + } + + @Test + fun `an explicit zero leaves the map uncapped`() { + LocalLockLimit.maxEntries = 42 + + LibAutoConfiguration(environmentWith(MAX_LOCK_ENTRIES_PROPERTY to "0")) + + assertEquals(UNLIMITED_LOCK_ENTRIES, LocalLockLimit.maxEntries) + } + + @Test + fun `a yml integer is bound just like a quoted string`() { + // application.yml hands `max-entries: 5000` over as an Integer, not a String. + LibAutoConfiguration(environmentWith(MAX_LOCK_ENTRIES_PROPERTY to 5000)) + + assertEquals(5_000L, LocalLockLimit.maxEntries) + } + + @Test + fun `an unusable cap is ignored instead of bringing the context down`() { + LocalLockLimit.maxEntries = 42 + + LibAutoConfiguration(environmentWith(MAX_LOCK_ENTRIES_PROPERTY to "not-a-number")) + assertEquals(42L, LocalLockLimit.maxEntries, "a malformed value must leave the cap alone") + + LibAutoConfiguration(environmentWith(MAX_LOCK_ENTRIES_PROPERTY to "-1")) + assertEquals(42L, LocalLockLimit.maxEntries, "a negative value must leave the cap alone") + } +} diff --git a/core/src/main/kotlin/com/linecorp/cse/reqshield/KeyLocalLock.kt b/core/src/main/kotlin/com/linecorp/cse/reqshield/KeyLocalLock.kt index a8974ae..1191609 100644 --- a/core/src/main/kotlin/com/linecorp/cse/reqshield/KeyLocalLock.kt +++ b/core/src/main/kotlin/com/linecorp/cse/reqshield/KeyLocalLock.kt @@ -16,6 +16,7 @@ package com.linecorp.cse.reqshield +import com.linecorp.cse.reqshield.support.config.LocalLockLimit import com.linecorp.cse.reqshield.support.constant.ConfigValues.LOCK_KEY_PREFIX import com.linecorp.cse.reqshield.support.constant.ConfigValues.LOCK_MONITOR_INTERVAL_MILLIS import com.linecorp.cse.reqshield.support.utils.nowToEpochTime @@ -51,9 +52,11 @@ class KeyLocalLock(private val lockTimeoutMillis: Long) : KeyLock { */ val isHeld: AtomicBoolean = AtomicBoolean(false), /** - * Ownership token of the current holder, null when the lock is not held. + * Ownership token of the current holder. * Only the holder presenting this token may release the lock, which keeps a holder * whose lock expired from releasing the lock of the holder that took it over. + * It is null only while the entry is being built inside compute(): unLock and the monitor + * drop the whole entry instead of clearing the token, so every entry in the map is held. * @Volatile ensures visibility across threads when updated inside compute() and read by monitor. */ @Volatile var token: String? = null, @@ -189,6 +192,15 @@ class KeyLocalLock(private val lockTimeoutMillis: Long) : KeyLock { } existing } else { + // At the cap, hand out a permit that no map entry backs instead of refusing. + // The caller then takes its normal path and writes the cache; making it wait + // would stall it on a holder that does not exist. Only this key's collapsing + // is lost, and the later unLock simply finds nothing to release. + if (LocalLockLimit.rejectsNewEntry(lockMap.mappingCount())) { + acquiredToken.set(nextToken()) + return@compute null + } + // New entry: create and acquire val token = nextToken() val newLock = LockInfo(Semaphore(1), now + lockTimeoutMillis) @@ -210,19 +222,22 @@ class KeyLocalLock(private val lockTimeoutMillis: Long) : KeyLock { val completeKey = buildLockKey(key, lockType) val released = AtomicBoolean(false) - // Release inside compute() so that ownership check and release are atomic with respect to - // tryLock() and the monitor cleanup. Reading the token outside compute() would allow - // "read token == mine -> lock expires and is reacquired by someone else -> release" and - // would therefore release the new owner's lock. + // Release inside compute() so that the ownership check, the release and the removal are + // atomic with respect to tryLock() and the monitor cleanup. Reading the token outside + // compute() would allow "read token == mine -> lock expires and is reacquired by someone + // else -> release" and would therefore release the new owner's lock. lockMap.compute(completeKey) { _, existing -> if (existing == null) return@compute null if (existing.token == token && existing.isHeld.compareAndSet(true, false)) { - existing.token = null existing.semaphore.release() released.set(true) + // Drop the entry immediately rather than leaving it for the monitor. No reference + // to it escapes this lambda, and the next tryLock() just creates a fresh one, so + // keeping it would only pin the key until its lockTimeoutMillis runs out. + return@compute null } - existing // Keep the entry, the monitor removes it once expired + existing } if (!released.get()) { diff --git a/core/src/test/kotlin/com/linecorp/cse/reqshield/KeyLocalLockTest.kt b/core/src/test/kotlin/com/linecorp/cse/reqshield/KeyLocalLockTest.kt index 68eacf9..43278ff 100644 --- a/core/src/test/kotlin/com/linecorp/cse/reqshield/KeyLocalLockTest.kt +++ b/core/src/test/kotlin/com/linecorp/cse/reqshield/KeyLocalLockTest.kt @@ -18,7 +18,9 @@ package com.linecorp.cse.reqshield import com.linecorp.cse.reqshield.support.BaseKeyLockTest import com.linecorp.cse.reqshield.support.BaseReqShieldTest.Companion.AWAIT_TIMEOUT +import com.linecorp.cse.reqshield.support.config.LocalLockLimit import com.linecorp.cse.reqshield.support.constant.ConfigValues.LOCK_MONITOR_INTERVAL_MILLIS +import com.linecorp.cse.reqshield.support.constant.ConfigValues.UNLIMITED_LOCK_ENTRIES import org.awaitility.Awaitility.await import org.junit.jupiter.api.Assertions.assertEquals import org.junit.jupiter.api.Assertions.assertFalse @@ -664,6 +666,65 @@ class KeyLocalLockTest : BaseKeyLockTest { keyLock.shutdown() } + @Test + fun `unLock removes the map entry right away instead of leaving it until expiry`() { + // A long lock timeout rules the monitor out as the remover: were the entry kept by unLock, + // it would still be in the map for the next 60 seconds. + val keyLock = KeyLocalLock(60_000L) + val key = "unlock-removal-test-${java.util.UUID.randomUUID()}" + val mapKey = buildLockKeyForTest(key, LockType.CREATE) + + val token = assertNotNull(keyLock.tryLock(key, LockType.CREATE)) + assertTrue(readLockMap().containsKey(mapKey), "A held lock must have an entry in the map") + + assertTrue(keyLock.unLock(key, LockType.CREATE, token)) + assertFalse(readLockMap().containsKey(mapKey), "unLock must drop the entry, not wait for the monitor") + + keyLock.shutdown() + } + + @Test + fun `at the cap tryLock still grants a permit but stops adding map entries`() { + val keyLock = KeyLocalLock(60_000L) + val first = "cap-test-first-${java.util.UUID.randomUUID()}" + val second = "cap-test-second-${java.util.UUID.randomUUID()}" + + // Taken while still uncapped, so it always succeeds. Its 60s timeout keeps the entry in + // the map for the rest of the test, which is what makes a cap of one deterministic here: + // the map can only grow from this point, never shrink below one. + val firstToken = assertNotNull(keyLock.tryLock(first, LockType.CREATE)) + assertTrue(readLockMap().containsKey(buildLockKeyForTest(first, LockType.CREATE))) + + LocalLockLimit.maxEntries = 1 + try { + val secondToken = + assertNotNull( + keyLock.tryLock(second, LockType.CREATE), + "past the cap the caller must still get a permit, so it writes the cache " + + "instead of waiting for a holder that does not exist", + ) + assertFalse( + readLockMap().containsKey(buildLockKeyForTest(second, LockType.CREATE)), + "a permit handed out past the cap must not add a map entry", + ) + assertFalse( + keyLock.unLock(second, LockType.CREATE, secondToken), + "the permit is backed by no entry, so releasing it finds nothing", + ) + + // Losing collapsing for that key is exactly what the cap costs. + assertNotNull( + keyLock.tryLock(second, LockType.CREATE), + "past the cap a second caller for the same key is not collapsed either", + ) + } finally { + LocalLockLimit.maxEntries = UNLIMITED_LOCK_ENTRIES + } + + keyLock.unLock(first, LockType.CREATE, firstToken) + keyLock.shutdown() + } + /** Mirrors KeyLocalLock's private buildLockKey so the test can look up the same map key. */ private fun buildLockKeyForTest( key: String, diff --git a/core/src/test/kotlin/com/linecorp/cse/reqshield/ReqShieldLockLimitIntegrationTest.kt b/core/src/test/kotlin/com/linecorp/cse/reqshield/ReqShieldLockLimitIntegrationTest.kt new file mode 100644 index 0000000..68a6b7e --- /dev/null +++ b/core/src/test/kotlin/com/linecorp/cse/reqshield/ReqShieldLockLimitIntegrationTest.kt @@ -0,0 +1,143 @@ +/* + * Copyright 2024 LY Corporation + * + * LY Corporation licenses this file to you under the Apache License, + * version 2.0 (the "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at: + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. + */ + +package com.linecorp.cse.reqshield + +import com.linecorp.cse.reqshield.config.ReqShieldConfiguration +import com.linecorp.cse.reqshield.support.config.LocalLockLimit +import com.linecorp.cse.reqshield.support.constant.ConfigValues.GET_CACHE_INTERVAL_MILLIS +import com.linecorp.cse.reqshield.support.constant.ConfigValues.UNLIMITED_LOCK_ENTRIES +import com.linecorp.cse.reqshield.support.model.Product +import com.linecorp.cse.reqshield.support.model.ReqShieldData +import org.awaitility.Awaitility.await +import org.junit.jupiter.api.AfterEach +import org.junit.jupiter.api.Assertions.assertEquals +import org.junit.jupiter.api.Assertions.assertTrue +import org.junit.jupiter.api.Test +import java.time.Duration +import java.util.UUID +import java.util.concurrent.Callable +import java.util.concurrent.ConcurrentHashMap +import java.util.concurrent.atomic.AtomicInteger +import kotlin.system.measureTimeMillis +import kotlin.test.assertNotNull + +/** + * Exercises a real [KeyLocalLock] through [ReqShield] with the lock map capped. + * + * The unit tests only prove what `tryLock` returns; what actually matters is what the request + * does with that answer. A refusal would send the caller into the lost-the-lock path, where it + * polls for a holder that does not exist and then returns **without writing the cache** - so the + * key would never become cacheable while the cap held. These tests pin the opposite. + */ +class ReqShieldLockLimitIntegrationTest { + private val maxAttemptGetCache = 20 + private val pollingBudgetMillis = maxAttemptGetCache * GET_CACHE_INTERVAL_MILLIS + + private val cache = ConcurrentHashMap>() + private val supplierCalls = AtomicInteger(0) + private lateinit var saturator: KeyLocalLock + private lateinit var saturatorToken: String + private lateinit var saturatorKey: String + + private fun reqShield(): ReqShield = + ReqShield( + ReqShieldConfiguration( + setCacheFunction = { key, data, _ -> + cache[key] = data + true + }, + getCacheFunction = { cache[it] }, + maxAttemptGetCache = maxAttemptGetCache, + ), + ) + + private fun supplier(name: String): Callable = + Callable { + supplierCalls.incrementAndGet() + Product("id-$name", name) + } + + /** + * Holds one real lock and caps the map at that single entry, so every other key is past the + * cap. Taking the lock while still uncapped makes this deterministic: the map can only grow + * from here, so it never drops back below the cap during a test. + */ + private fun saturateLockMap() { + saturatorKey = "saturator-${UUID.randomUUID()}" + saturator = KeyLocalLock(60_000L) + saturatorToken = assertNotNull(saturator.tryLock(saturatorKey, LockType.CREATE)) + LocalLockLimit.maxEntries = 1 + } + + @AfterEach + fun releaseSaturator() { + LocalLockLimit.maxEntries = UNLIMITED_LOCK_ENTRIES + if (::saturator.isInitialized) { + saturator.unLock(saturatorKey, LockType.CREATE, saturatorToken) + saturator.shutdown() + } + } + + @Test + fun `a cache miss past the cap populates the cache without waiting for a holder`() { + saturateLockMap() + val key = "capped-miss-${UUID.randomUUID()}" + + val elapsed = + measureTimeMillis { + val data = reqShield().getAndSetReqShieldData(key, supplier("first"), 10_000) + assertEquals("first", data.value?.name) + } + + assertTrue( + elapsed < pollingBudgetMillis, + "the request must not spend the ${pollingBudgetMillis}ms polling budget waiting for a " + + "holder that the cap prevented from existing, but took ${elapsed}ms", + ) + await().atMost(Duration.ofSeconds(5)).untilAsserted { + assertNotNull(cache[key], "a request past the cap must still write the cache") + } + assertEquals(1, supplierCalls.get()) + } + + @Test + fun `the value written past the cap is served from the cache on the next request`() { + saturateLockMap() + val key = "capped-reuse-${UUID.randomUUID()}" + + reqShield().getAndSetReqShieldData(key, supplier("first"), 10_000) + await().atMost(Duration.ofSeconds(5)).untilAsserted { assertNotNull(cache[key]) } + + // Still capped: the second request must be a plain cache hit, not another supplier call. + val data = reqShield().getAndSetReqShieldData(key, supplier("second"), 10_000) + + assertEquals("first", data.value?.name, "the second request must be served from the cache") + assertEquals(1, supplierCalls.get(), "a cached key must not reach the supplier again") + } + + @Test + fun `an uncapped map is unaffected`() { + LocalLockLimit.maxEntries = UNLIMITED_LOCK_ENTRIES + val key = "uncapped-${UUID.randomUUID()}" + + val data = reqShield().getAndSetReqShieldData(key, supplier("first"), 10_000) + + assertEquals("first", data.value?.name) + await().atMost(Duration.ofSeconds(5)).untilAsserted { assertNotNull(cache[key]) } + assertEquals(1, supplierCalls.get()) + } +} diff --git a/support/src/main/kotlin/com/linecorp/cse/reqshield/support/config/LocalLockLimit.kt b/support/src/main/kotlin/com/linecorp/cse/reqshield/support/config/LocalLockLimit.kt new file mode 100644 index 0000000..1dbc3c9 --- /dev/null +++ b/support/src/main/kotlin/com/linecorp/cse/reqshield/support/config/LocalLockLimit.kt @@ -0,0 +1,124 @@ +/* + * Copyright 2024 LY Corporation + * + * LY Corporation licenses this file to you under the Apache License, + * version 2.0 (the "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at: + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. + */ + +package com.linecorp.cse.reqshield.support.config + +import com.linecorp.cse.reqshield.support.constant.ConfigValues.MAX_LOCK_ENTRIES_PROPERTY +import com.linecorp.cse.reqshield.support.constant.ConfigValues.UNLIMITED_LOCK_ENTRIES +import org.slf4j.LoggerFactory +import java.util.concurrent.atomic.AtomicLong + +private val log = LoggerFactory.getLogger(LocalLockLimit::class.java) + +/** + * Cap on how many entries a local lock map may hold. + * + * An entry exists only while a lock is held, so the map's size tracks how many keys are being + * locked *at the same time*, plus any lock whose holder never released it and that has not yet + * timed out. The cap therefore bounds concurrent lock ownership, not request cardinality. + * + * Once the map is full, the next new key is handed a permit that no map entry backs. The caller + * proceeds exactly as if it had taken the lock - it calls the supplier and writes the cache - so + * the cap costs request collapsing for that key and nothing else. Making the caller wait instead + * would be worse: there is no holder to wait for, so it would stall for the whole polling budget + * and then return without ever populating the cache. + * + * Uncapped by default. Set [MAX_LOCK_ENTRIES_PROPERTY] as a system property, or let the Spring + * modules pass the same key through [applyConfiguredValue], which also picks it up from + * application.yml. + * + * The cap applies per lock map, and each core module owns one, so an application that somehow used + * two of them would get the cap applied to each map separately. + */ +object LocalLockLimit { + /** + * Maximum number of entries a local lock map may hold; [UNLIMITED_LOCK_ENTRIES] disables the + * cap. Assigning a negative value fails fast, which is what a programmatic caller wants; a + * value that arrives as unparsed configuration is filtered by [parseMaxEntries] first, because + * neither a class initializer nor an application context should die over a tuning knob typo. + */ + @Volatile + var maxEntries: Long = parseMaxEntries(System.getProperty(MAX_LOCK_ENTRIES_PROPERTY)) ?: UNLIMITED_LOCK_ENTRIES + set(value) { + require(value >= UNLIMITED_LOCK_ENTRIES) { + "$MAX_LOCK_ENTRIES_PROPERTY must not be negative, but was $value" + } + field = value + } + + private const val REJECTION_LOG_INTERVAL = 1000L + + private val rejectionCount = AtomicLong(0) + + /** + * Applies a cap that arrived as configuration, the same way the system property is applied: + * an absent value leaves the current cap alone, and an unusable one is reported and ignored. + * The Spring modules route [MAX_LOCK_ENTRIES_PROPERTY] through here so that a typo behaves + * identically whether it reached the library through application.yml or through -D. + */ + fun applyConfiguredValue(rawValue: String?) { + parseMaxEntries(rawValue)?.let { maxEntries = it } + } + + /** + * Whether a lock map holding [currentSize] entries must refuse to take a new one. + * + * [currentSize] is an estimate on a concurrent map and the check is not atomic with the + * insertion it guards, so the cap is a soft one: concurrent callers can push the map a little + * past it. That is fine for a footprint guard. + * + * Logs the first refusal and every [REJECTION_LOG_INTERVAL]th after it, so a sustained + * overflow reports itself without flooding the log. + */ + fun rejectsNewEntry(currentSize: Long): Boolean { + val limit = maxEntries + if (limit == UNLIMITED_LOCK_ENTRIES || currentSize < limit) return false + + val rejections = rejectionCount.incrementAndGet() + if (rejections == 1L || rejections % REJECTION_LOG_INTERVAL == 0L) { + log.warn( + "Local lock map is at its cap of {} entries, so new keys are no longer collapsed " + + "({} refusals so far). Raise {} or shorten lockTimeoutMillis if this is unexpected.", + limit, + rejections, + MAX_LOCK_ENTRIES_PROPERTY, + ) + } + return true + } + + /** + * Parses the configured form of the cap, returning null when there is nothing usable to apply + * - either because it is absent or because it cannot be read as a non-negative number. Kept + * separate from the [maxEntries] setter so that neither the class initializer nor a Spring + * context refresh can be brought down by a malformed value. + */ + internal fun parseMaxEntries(configured: String?): Long? { + if (configured == null) return null + + val parsed = configured.trim().toLongOrNull() + if (parsed == null || parsed < UNLIMITED_LOCK_ENTRIES) { + log.warn( + "Ignoring {}='{}': expected a non-negative number, leaving the current cap unchanged", + MAX_LOCK_ENTRIES_PROPERTY, + configured, + ) + return null + } + + return parsed + } +} diff --git a/support/src/main/kotlin/com/linecorp/cse/reqshield/support/constant/ConfigValues.kt b/support/src/main/kotlin/com/linecorp/cse/reqshield/support/constant/ConfigValues.kt index 140f2c5..51b4fee 100644 --- a/support/src/main/kotlin/com/linecorp/cse/reqshield/support/constant/ConfigValues.kt +++ b/support/src/main/kotlin/com/linecorp/cse/reqshield/support/constant/ConfigValues.kt @@ -34,4 +34,14 @@ object ConfigValues { /** Prefix applied to every lock key so lock entries can never collide with cache entries. */ const val LOCK_KEY_PREFIX = "reqshield:lock:" + + /** + * Caps how many entries the local lock map may hold. The core modules read it from the system + * properties; the Spring modules read the same key from the Environment, which also covers + * application.yml and REQ_SHIELD_LOCK_MAX_ENTRIES. + */ + const val MAX_LOCK_ENTRIES_PROPERTY = "req-shield.lock.max-entries" + + /** Value of [MAX_LOCK_ENTRIES_PROPERTY] that leaves the local lock map uncapped. */ + const val UNLIMITED_LOCK_ENTRIES = 0L } diff --git a/support/src/test/kotlin/com/linecorp/cse/reqshield/support/config/LocalLockLimitTest.kt b/support/src/test/kotlin/com/linecorp/cse/reqshield/support/config/LocalLockLimitTest.kt new file mode 100644 index 0000000..2be821f --- /dev/null +++ b/support/src/test/kotlin/com/linecorp/cse/reqshield/support/config/LocalLockLimitTest.kt @@ -0,0 +1,107 @@ +/* + * Copyright 2024 LY Corporation + * + * LY Corporation licenses this file to you under the Apache License, + * version 2.0 (the "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at: + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. + */ + +package com.linecorp.cse.reqshield.support.config + +import com.linecorp.cse.reqshield.support.constant.ConfigValues.UNLIMITED_LOCK_ENTRIES +import org.junit.jupiter.api.AfterEach +import org.junit.jupiter.api.Assertions.assertEquals +import org.junit.jupiter.api.Assertions.assertFalse +import org.junit.jupiter.api.Assertions.assertTrue +import org.junit.jupiter.api.Test +import org.junit.jupiter.api.assertThrows +import kotlin.test.assertNull + +class LocalLockLimitTest { + @AfterEach + fun restoreDefault() { + // The limit is a process-wide setting, so a test that changes it must put it back. + LocalLockLimit.maxEntries = UNLIMITED_LOCK_ENTRIES + } + + @Test + fun `an uncapped map never refuses a new entry`() { + LocalLockLimit.maxEntries = UNLIMITED_LOCK_ENTRIES + + assertFalse(LocalLockLimit.rejectsNewEntry(0)) + assertFalse(LocalLockLimit.rejectsNewEntry(Long.MAX_VALUE)) + } + + @Test + fun `a capped map refuses a new entry only once it is full`() { + LocalLockLimit.maxEntries = 3 + + assertFalse(LocalLockLimit.rejectsNewEntry(0), "an empty map has room") + assertFalse(LocalLockLimit.rejectsNewEntry(2), "the last free slot is still a slot") + assertTrue(LocalLockLimit.rejectsNewEntry(3), "a full map must refuse") + assertTrue(LocalLockLimit.rejectsNewEntry(4), "an over-full map must keep refusing") + } + + @Test + fun `a cap of one collapses nothing beyond the first key`() { + LocalLockLimit.maxEntries = 1 + + assertFalse(LocalLockLimit.rejectsNewEntry(0)) + assertTrue(LocalLockLimit.rejectsNewEntry(1)) + } + + @Test + fun `a negative cap is rejected rather than silently disabling the limit`() { + val failure = assertThrows { LocalLockLimit.maxEntries = -1 } + + assertTrue(failure.message!!.contains("must not be negative"), failure.message) + assertEquals(UNLIMITED_LOCK_ENTRIES, LocalLockLimit.maxEntries, "the rejected value must not be applied") + } + + @Test + fun `an absent value yields nothing to apply`() { + assertNull(LocalLockLimit.parseMaxEntries(null)) + } + + @Test + fun `a configured value is read as a number, surrounding whitespace included`() { + assertEquals(5_000L, LocalLockLimit.parseMaxEntries("5000")) + assertEquals(5_000L, LocalLockLimit.parseMaxEntries(" 5000 ")) + assertEquals(UNLIMITED_LOCK_ENTRIES, LocalLockLimit.parseMaxEntries("0")) + } + + @Test + fun `an unusable value yields nothing to apply instead of failing`() { + assertNull(LocalLockLimit.parseMaxEntries("not-a-number")) + assertNull(LocalLockLimit.parseMaxEntries("-1"), "a negative cap is configuration noise, not a cap of zero") + assertNull(LocalLockLimit.parseMaxEntries("")) + } + + @Test + fun `applying a configured value leaves the current cap alone unless the value is usable`() { + LocalLockLimit.maxEntries = 42 + + LocalLockLimit.applyConfiguredValue(null) + assertEquals(42L, LocalLockLimit.maxEntries, "an absent value must not reset a cap set elsewhere") + + LocalLockLimit.applyConfiguredValue("not-a-number") + assertEquals(42L, LocalLockLimit.maxEntries, "an unusable value must not reset a cap set elsewhere") + + LocalLockLimit.applyConfiguredValue("-7") + assertEquals(42L, LocalLockLimit.maxEntries, "a negative value must not reset a cap set elsewhere") + + LocalLockLimit.applyConfiguredValue("7") + assertEquals(7L, LocalLockLimit.maxEntries, "a usable value must be applied") + + LocalLockLimit.applyConfiguredValue("0") + assertEquals(UNLIMITED_LOCK_ENTRIES, LocalLockLimit.maxEntries, "an explicit zero must uncap the map") + } +}