From 6c4917922a40cb23ecb7dbf5fdbccfcb7eae844c Mon Sep 17 00:00:00 2001 From: vahid torkaman Date: Tue, 14 Oct 2025 16:52:59 +0200 Subject: [PATCH 1/3] apply read write lock for file disk storage --- .../confidence/cache/FileDiskStorage.kt | 37 +++++++++++++------ 1 file changed, 25 insertions(+), 12 deletions(-) diff --git a/Confidence/src/main/java/com/spotify/confidence/cache/FileDiskStorage.kt b/Confidence/src/main/java/com/spotify/confidence/cache/FileDiskStorage.kt index 5218d1b6..38d4cc6e 100644 --- a/Confidence/src/main/java/com/spotify/confidence/cache/FileDiskStorage.kt +++ b/Confidence/src/main/java/com/spotify/confidence/cache/FileDiskStorage.kt @@ -6,6 +6,9 @@ import com.spotify.confidence.apply.ApplyInstance import kotlinx.serialization.encodeToString import kotlinx.serialization.json.Json import java.io.File +import java.util.concurrent.locks.ReentrantReadWriteLock +import kotlin.concurrent.read +import kotlin.concurrent.write internal const val FLAGS_FILE_NAME = "confidence_flags_cache.json" internal const val APPLY_FILE_NAME = "confidence_apply_cache.json" @@ -14,13 +17,16 @@ internal class FileDiskStorage internal constructor( private val flagsFile: File, private val applyFile: File ) : DiskStorage { + private val lock = ReentrantReadWriteLock() override fun store(flagResolution: FlagResolution) { write(Json.encodeToString(flagResolution)) } override fun clear() { - flagsFile.delete() + lock.write { + flagsFile.delete() + } } override fun writeApplyData(applyData: Map>) { @@ -42,22 +48,29 @@ internal class FileDiskStorage internal constructor( } } - private fun write(data: String) { + private fun write(data: String) = lock.write { flagsFile.writeText(data) } - override fun read(): FlagResolution { - if (!flagsFile.exists()) return FlagResolution.EMPTY - val fileText: String = flagsFile.bufferedReader().use { it.readText() } - return if (fileText.isEmpty()) { - FlagResolution.EMPTY - } else { - try { - Json.decodeFromString(fileText) - } catch (e: Throwable) { - flagsFile.delete() + override fun read(): FlagResolution = lock.read { + try { + if (!flagsFile.exists()) return FlagResolution.EMPTY + val fileText: String = flagsFile.bufferedReader().use { it.readText() } + return if (fileText.isEmpty()) { FlagResolution.EMPTY + } else { + try { + Json.decodeFromString(fileText) + } catch (e: Throwable) { + // Delete corrupted file - safe to do while holding read lock + // since we're the only thread accessing it + flagsFile.delete() + FlagResolution.EMPTY + } } + } catch (e: java.io.FileNotFoundException) { + // File was deleted between exists check and read + return FlagResolution.EMPTY } } From 26a27ec531eff8c007d4629e667c5e718b45e5bf Mon Sep 17 00:00:00 2001 From: vahid torkaman Date: Tue, 14 Oct 2025 17:14:46 +0200 Subject: [PATCH 2/3] add test --- .../confidence/cache/FileDiskStorage.kt | 5 - .../spotify/confidence/FileDiskStorageTest.kt | 139 ++++++++++++++++++ 2 files changed, 139 insertions(+), 5 deletions(-) diff --git a/Confidence/src/main/java/com/spotify/confidence/cache/FileDiskStorage.kt b/Confidence/src/main/java/com/spotify/confidence/cache/FileDiskStorage.kt index 38d4cc6e..fdf3f14b 100644 --- a/Confidence/src/main/java/com/spotify/confidence/cache/FileDiskStorage.kt +++ b/Confidence/src/main/java/com/spotify/confidence/cache/FileDiskStorage.kt @@ -53,7 +53,6 @@ internal class FileDiskStorage internal constructor( } override fun read(): FlagResolution = lock.read { - try { if (!flagsFile.exists()) return FlagResolution.EMPTY val fileText: String = flagsFile.bufferedReader().use { it.readText() } return if (fileText.isEmpty()) { @@ -68,10 +67,6 @@ internal class FileDiskStorage internal constructor( FlagResolution.EMPTY } } - } catch (e: java.io.FileNotFoundException) { - // File was deleted between exists check and read - return FlagResolution.EMPTY - } } companion object { diff --git a/Confidence/src/test/java/com/spotify/confidence/FileDiskStorageTest.kt b/Confidence/src/test/java/com/spotify/confidence/FileDiskStorageTest.kt index d1827644..d800d910 100644 --- a/Confidence/src/test/java/com/spotify/confidence/FileDiskStorageTest.kt +++ b/Confidence/src/test/java/com/spotify/confidence/FileDiskStorageTest.kt @@ -9,6 +9,9 @@ import org.junit.Test import java.io.File import java.nio.file.Files import java.util.Date +import java.util.concurrent.CountDownLatch +import java.util.concurrent.CyclicBarrier +import kotlin.concurrent.thread class FileDiskStorageTest { private lateinit var diskStorage: FileDiskStorage @@ -64,4 +67,140 @@ class FileDiskStorageTest { val read = diskStorage.read() Assert.assertEquals(FlagResolution.EMPTY, read) } + + @Test + fun testConcurrentReadersAndWriters() { + // This test reproduces the original crash scenario: + // Multiple threads reading while multiple threads are writing simultaneously + + // Given: Initial data + val initialData = FlagResolution( + mapOf("key" to ConfidenceValue.String("initial")), + listOf(), + "token-initial" + ) + diskStorage.store(initialData) + + // When: Multiple reader threads AND multiple writer threads run concurrently + val readerCount = 10 + val writerCount = 5 + val iterations = 200 + val exceptions = mutableListOf() + val barrier = CyclicBarrier(readerCount + writerCount) + val threads = mutableListOf() + + // Reader threads - continuously read + repeat(readerCount) { + threads.add(thread { + try { + barrier.await() // Start all threads at the same time + repeat(iterations) { + diskStorage.read() // This used to throw FileNotFoundException + } + } catch (e: Throwable) { + synchronized(exceptions) { + exceptions.add(e) + } + } + }) + } + + // Writer threads - continuously write + repeat(writerCount) { writerId -> + threads.add(thread { + try { + barrier.await() // Start all threads at the same time + repeat(iterations) { iteration -> + val testData = FlagResolution( + mapOf("writer" to ConfidenceValue.String("$writerId-$iteration")), + listOf(), + "token-$writerId-$iteration" + ) + diskStorage.store(testData) + } + } catch (e: Throwable) { + synchronized(exceptions) { + exceptions.add(e) + } + } + }) + } + + // Then: No FileNotFoundException or other exceptions should occur + threads.forEach { it.join(30000) } + Assert.assertTrue( + "Expected no exceptions but got: ${exceptions.map { "${it.javaClass.simpleName}: ${it.message}" }}", + exceptions.isEmpty() + ) + + // And: File should contain valid data from one of the writers + val finalResult = diskStorage.read() + Assert.assertNotEquals(FlagResolution.EMPTY, finalResult) + } + + @Test + fun testFileDeletedExternallyDuringRead() { + // Given: A file with valid data + val testData = FlagResolution( + mapOf("key1" to ConfidenceValue.String("value1")), + listOf(), + "token123" + ) + diskStorage.store(testData) + + // When: File is deleted externally during read attempts + val readThreadCount = 5 + val latch = CountDownLatch(1) + val exceptions = mutableListOf() + + val threads = List(readThreadCount) { + thread { + try { + latch.await() + // Repeatedly try to read, some will succeed, some will find file missing + repeat(10) { + diskStorage.read() // Should handle FileNotFoundException gracefully + Thread.sleep(1) + } + } catch (e: Throwable) { + synchronized(exceptions) { + exceptions.add(e) + } + } + } + } + + // Delete file externally (simulating external process/crash) + thread { + latch.countDown() + Thread.sleep(5) // Let some reads start + flagsFile.delete() + } + + // Then: No FileNotFoundException should propagate + threads.forEach { it.join(5000) } + Assert.assertTrue( + "Expected no exceptions but got: ${exceptions.map { it.javaClass.simpleName + ": " + it.message }}", + exceptions.isEmpty() + ) + } + + @Test + fun testReadAfterClearReturnsEmpty() { + // Given: A file with data + val testData = FlagResolution( + mapOf("key1" to ConfidenceValue.String("value1")), + listOf(), + "token123" + ) + diskStorage.store(testData) + Assert.assertEquals(testData, diskStorage.read()) + + // When: File is cleared + diskStorage.clear() + + // Then: Read returns empty + val result = diskStorage.read() + Assert.assertEquals(FlagResolution.EMPTY, result) + } } \ No newline at end of file From 40255eab90472064b7f5c95e964dd711aa6f0958 Mon Sep 17 00:00:00 2001 From: vahid torkaman Date: Tue, 14 Oct 2025 17:21:15 +0200 Subject: [PATCH 3/3] run ktlint format --- .../confidence/cache/FileDiskStorage.kt | 24 ++++----- .../spotify/confidence/FileDiskStorageTest.kt | 54 ++++++++++--------- 2 files changed, 41 insertions(+), 37 deletions(-) diff --git a/Confidence/src/main/java/com/spotify/confidence/cache/FileDiskStorage.kt b/Confidence/src/main/java/com/spotify/confidence/cache/FileDiskStorage.kt index fdf3f14b..0e643acc 100644 --- a/Confidence/src/main/java/com/spotify/confidence/cache/FileDiskStorage.kt +++ b/Confidence/src/main/java/com/spotify/confidence/cache/FileDiskStorage.kt @@ -53,20 +53,20 @@ internal class FileDiskStorage internal constructor( } override fun read(): FlagResolution = lock.read { - if (!flagsFile.exists()) return FlagResolution.EMPTY - val fileText: String = flagsFile.bufferedReader().use { it.readText() } - return if (fileText.isEmpty()) { + if (!flagsFile.exists()) return FlagResolution.EMPTY + val fileText: String = flagsFile.bufferedReader().use { it.readText() } + return if (fileText.isEmpty()) { + FlagResolution.EMPTY + } else { + try { + Json.decodeFromString(fileText) + } catch (e: Throwable) { + // Delete corrupted file - safe to do while holding read lock + // since we're the only thread accessing it + flagsFile.delete() FlagResolution.EMPTY - } else { - try { - Json.decodeFromString(fileText) - } catch (e: Throwable) { - // Delete corrupted file - safe to do while holding read lock - // since we're the only thread accessing it - flagsFile.delete() - FlagResolution.EMPTY - } } + } } companion object { diff --git a/Confidence/src/test/java/com/spotify/confidence/FileDiskStorageTest.kt b/Confidence/src/test/java/com/spotify/confidence/FileDiskStorageTest.kt index d800d910..bdb7c9fe 100644 --- a/Confidence/src/test/java/com/spotify/confidence/FileDiskStorageTest.kt +++ b/Confidence/src/test/java/com/spotify/confidence/FileDiskStorageTest.kt @@ -91,39 +91,43 @@ class FileDiskStorageTest { // Reader threads - continuously read repeat(readerCount) { - threads.add(thread { - try { - barrier.await() // Start all threads at the same time - repeat(iterations) { - diskStorage.read() // This used to throw FileNotFoundException - } - } catch (e: Throwable) { - synchronized(exceptions) { - exceptions.add(e) + threads.add( + thread { + try { + barrier.await() // Start all threads at the same time + repeat(iterations) { + diskStorage.read() // This used to throw FileNotFoundException + } + } catch (e: Throwable) { + synchronized(exceptions) { + exceptions.add(e) + } } } - }) + ) } // Writer threads - continuously write repeat(writerCount) { writerId -> - threads.add(thread { - try { - barrier.await() // Start all threads at the same time - repeat(iterations) { iteration -> - val testData = FlagResolution( - mapOf("writer" to ConfidenceValue.String("$writerId-$iteration")), - listOf(), - "token-$writerId-$iteration" - ) - diskStorage.store(testData) - } - } catch (e: Throwable) { - synchronized(exceptions) { - exceptions.add(e) + threads.add( + thread { + try { + barrier.await() // Start all threads at the same time + repeat(iterations) { iteration -> + val testData = FlagResolution( + mapOf("writer" to ConfidenceValue.String("$writerId-$iteration")), + listOf(), + "token-$writerId-$iteration" + ) + diskStorage.store(testData) + } + } catch (e: Throwable) { + synchronized(exceptions) { + exceptions.add(e) + } } } - }) + ) } // Then: No FileNotFoundException or other exceptions should occur