From 4067167f4f4cb224b4849a05725c980846c5b2b4 Mon Sep 17 00:00:00 2001 From: UnknownJoe796 Date: Wed, 29 Jul 2026 16:39:29 -0600 Subject: [PATCH] Make settings resolution thread-safe Concurrent first-touch of a setting could resolve it more than once. Transformed results are now published as an immutable snapshot: reads are lock-free (once a setting is resolved, an immutable Map makes it visible for good via a @Volatile field), and writes replace the whole map under a single lock (copy-on-write) -- cheap, since each setting resolves at most once, and in production every setting is pre-resolved single-threaded during ready(). A single reentrant lock guards transformation because a setting's getter may resolve its own dependencies by calling get() again on the same thread; per-setting locks could deadlock on mutually-dependent settings instead. (cherry picked from commit ed1d009b83c351a94ca810deca294baaee6c32d9) --- .../settings/ServerSettings.kt | 43 ++++++++++++---- .../ServerSettingThreadSafetyTest.kt | 51 +++++++++++++++++++ .../settings/ServerSettingsTest.kt | 21 ++++++++ 3 files changed, 106 insertions(+), 9 deletions(-) diff --git a/core/src/main/kotlin/com/lightningkite/lightningserver/settings/ServerSettings.kt b/core/src/main/kotlin/com/lightningkite/lightningserver/settings/ServerSettings.kt index 4687c88bc..ab07e4ced 100644 --- a/core/src/main/kotlin/com/lightningkite/lightningserver/settings/ServerSettings.kt +++ b/core/src/main/kotlin/com/lightningkite/lightningserver/settings/ServerSettings.kt @@ -89,7 +89,20 @@ public class ServerSettings private constructor( private var usingDefaults: Boolean = false private val serializable: MapRegistry, Any?> = MapRegistry() - private val goal: MapRegistry, Any?> = MapRegistry() + + // Transformed results, published as an immutable snapshot. Reads are lock-free: once a setting is resolved, + // its value is visible here for good (the @Volatile guarantees other threads see the new snapshot). Writes + // replace the whole map under [resolveLock] (copy-on-write) — cheap because each setting is resolved at most + // once, and in production every setting is pre-resolved single-threaded during ready(). An immutable Map + // (not a ConcurrentHashMap) is used so a null result is stored naturally, with no sentinel. + @Volatile + private var goal: Map, Any?> = emptyMap() + + // Guards resolution so each setting is transformed exactly once, even if several threads first request it + // at once. A single reentrant lock — not per-setting — is deliberate: a setting's getter may resolve its + // dependencies by calling get() again on the same thread, which re-enters this lock harmlessly, whereas + // per-setting locks could deadlock between two mutually-dependent settings. + private val resolveLock = Any() // Track settings currently being resolved to detect circular dependencies at runtime private val resolving: ThreadLocal>>? = @@ -137,7 +150,8 @@ public class ServerSettings private constructor( */ public infix fun ServerSetting<*, RESULT>.setStatic(value: RESULT) { if (ready) throw IllegalStateException("Settings are marked as ready.") - goal.register(this, value as Any?) + // Pre-ready configuration is single-threaded, so a plain copy-on-write assignment is enough. + goal = goal + (this to value) } /** @@ -250,7 +264,16 @@ public class ServerSettings private constructor( public fun get(key: ServerSetting): RESULT { if (!ready) throw IllegalStateException("Settings not ready yet.") - return goal.getOrRegister(key) { + // Lock-free fast path: once a setting is resolved its value lives in [goal] forever, read without locking. + // containsKey (not a null check) because a resolved value may legitimately be null. + goal.let { if (it.containsKey(key)) return it[key] as RESULT } + + // Slow path: resolve under [resolveLock] so each setting is transformed at most once, even when several + // threads first request it concurrently. The per-thread [resolving] set still detects circular deps. + synchronized(resolveLock) { + // Re-check now that we hold the lock; another thread may have resolved it while we waited. + goal.let { if (it.containsKey(key)) return it[key] as RESULT } + // Check for circular dependency during resolution val currentlyResolving = resolving?.get() if (currentlyResolving != null && key in currentlyResolving) { @@ -258,7 +281,7 @@ public class ServerSettings private constructor( } currentlyResolving?.add(key) - try { + val result = try { overrides[key]?.let { defer -> serializable[key] ?.let { key.get(it as SERIALIZABLE) } // specified in settings file @@ -282,7 +305,11 @@ public class ServerSettings private constructor( } finally { currentlyResolving?.remove(key) } - } as RESULT + // Publish by re-reading [goal] (never a snapshot captured before resolution): the getter above may + // have recursively resolved dependencies and published them, and those additions must be preserved. + goal = goal + (key to result) + return result as RESULT + } } // For some dumb fucking reason kotlin made this internal, and it's what getOrElse should do in the first place! @@ -328,7 +355,8 @@ public class ServerSettings private constructor( overrides: Map, Runtime<*>> = this.overrides, ) = ServerSettings(settings, overrides).also { it.serializable.include(this.serializable) - it.goal.include(this.goal) + // Safe to share the reference: [goal] is immutable, and copy-on-write writes replace it rather than mutate. + it.goal = this.goal } public operator fun plus(requirement: ServerSetting<*, *>): ServerSettings = copy(settings + requirement) @@ -347,9 +375,6 @@ public class ServerSettings private constructor( * 2. The missing settings check in `ready()` doesn't distinguish between truly required settings * and optional settings with no default. The error message could be more helpful. * - * 3. The `get()` function performs lazy transformation but the caching isn't thread-safe. - * If called concurrently during initial ready phase, could transform the same setting twice. - * (Though this is documented in ServerSetting.kt TODO, worth noting here too.) * * 4. `readyUsingDefaults()` bypasses all validation with a warning, but doesn't actually * enforce that defaults exist for all settings. Could still throw during get(). diff --git a/core/src/test/kotlin/com/lightningkite/lightningserver/definition/ServerSettingThreadSafetyTest.kt b/core/src/test/kotlin/com/lightningkite/lightningserver/definition/ServerSettingThreadSafetyTest.kt index 92c224b5c..c30da21ba 100644 --- a/core/src/test/kotlin/com/lightningkite/lightningserver/definition/ServerSettingThreadSafetyTest.kt +++ b/core/src/test/kotlin/com/lightningkite/lightningserver/definition/ServerSettingThreadSafetyTest.kt @@ -1,6 +1,9 @@ package com.lightningkite.lightningserver.definition +import com.lightningkite.lightningserver.runtime.ServerRuntime +import com.lightningkite.lightningserver.settings.ServerSettings import kotlinx.coroutines.* +import kotlinx.serialization.builtins.serializer import org.junit.Test import java.util.concurrent.atomic.AtomicInteger import kotlin.test.assertEquals @@ -59,4 +62,52 @@ class ServerSettingThreadSafetyTest { // The computation should only happen once despite 100 concurrent accesses assertEquals(1, executionCount.get(), "Runtime should only execute once, got ${executionCount.get()}") } + + /** + * Hammers [ServerSettings.get] from many threads for several settings simultaneously and asserts that + * each setting's transformer runs exactly once. Guards against the double-transform race that existed + * when the goal registry was populated without synchronization. + */ + @Test + fun `ServerSettings get transforms each setting exactly once under concurrent first-access`() = runBlocking { + val counters = (0 until 8).map { AtomicInteger(0) } + val settings = counters.mapIndexed { i, counter -> + ServerSetting("setting$i", 0, Int.serializer()) { + counter.incrementAndGet() + Thread.sleep(20) // Widen the race window so an unsynchronized cache would double-transform. + "result-$i" + } + } + val serverSettings = ServerSettings(settings) + serverSettings.readyUsingDefaults() + + val runtime = stubRuntime() + + // Every coroutine resolves every setting; without per-setting synchronization the transformers race. + (1..100).map { + async(Dispatchers.Default) { + with(runtime) { settings.map { serverSettings.get(it) } } + } + }.awaitAll() + + counters.forEachIndexed { i, counter -> + assertEquals(1, counter.get(), "Transformer for setting$i should run exactly once, got ${counter.get()}") + } + } + + /** Minimal [ServerRuntime] stub whose only usable capability is acting as the settings/transformation context. */ + private fun stubRuntime(): ServerRuntime = object : ServerRuntime { + override val server get() = throw NotImplementedError() + override val settings get() = throw NotImplementedError() + override val internalSerialization get() = throw NotImplementedError() + override val externalSerialization get() = throw NotImplementedError() + override val serverId get() = "" + override val serverVersion get() = "" + override val projectName get() = "" + override val sharedResources get() = throw NotImplementedError() + override suspend fun Task.invoke(input: T) = throw NotImplementedError() + override suspend fun sendWebSocketSubscriptionMessage( + event: com.lightningkite.lightningserver.websockets.WebSocketSubscriptionMessage, + ) = throw NotImplementedError() + } } diff --git a/core/src/test/kotlin/com/lightningkite/lightningserver/settings/ServerSettingsTest.kt b/core/src/test/kotlin/com/lightningkite/lightningserver/settings/ServerSettingsTest.kt index 53325b29b..77e3321e4 100644 --- a/core/src/test/kotlin/com/lightningkite/lightningserver/settings/ServerSettingsTest.kt +++ b/core/src/test/kotlin/com/lightningkite/lightningserver/settings/ServerSettingsTest.kt @@ -172,4 +172,25 @@ class ServerSettingsTest { assertEquals("A", b()) // should defer to 'a' as configured in the server } } + + @Test + fun `resolving a setting caches its reentrantly-resolved dependencies`() { + // Guards get()'s copy-on-write publish: resolving `derived` reentrantly resolves `base`, and publishing + // derived must not clobber base's freshly-cached entry. If it did, `base` would be transformed twice. + val baseTransforms = java.util.concurrent.atomic.AtomicInteger(0) + object : ServerBuilder() { + val base = setting("base", "x", getter = { baseTransforms.incrementAndGet(); it }) + val derived = setting("derived", "y") + + init { + derived bind base + } + }.let { server -> + server.test({}) { + assertEquals("x", server.derived()) // reentrantly resolves and caches base + assertEquals("x", server.base()) // must hit the cache, not re-transform + assertEquals(1, baseTransforms.get(), "base should be transformed exactly once") + } + } + } }