Skip to content

Commit cfb4ae0

Browse files
matt-ramotarmatt-ramotar
authored
Fix MutableStore.write() ignoring SourceOfTruth write failures (#727)
Signed-off-by: Matt Ramotar <matt.ramotar@uber.com>
1 parent d4e94b4 commit cfb4ae0

6 files changed

Lines changed: 167 additions & 35 deletions

File tree

‎store/src/commonMain/kotlin/org/mobilenativefoundation/store/store5/impl/RealMutableStore.kt‎

Lines changed: 23 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ import org.mobilenativefoundation.store.store5.impl.extensions.now
2424
import org.mobilenativefoundation.store.store5.internal.concurrent.ThreadSafety
2525
import org.mobilenativefoundation.store.store5.internal.definition.WriteRequestQueue
2626
import org.mobilenativefoundation.store.store5.internal.result.EagerConflictResolutionResult
27+
import org.mobilenativefoundation.store.store5.internal.result.StoreDelegateWriteResult
2728

2829
@OptIn(ExperimentalStoreApi::class)
2930
internal class RealMutableStore<Key : Any, Network : Any, Output : Any, Local : Any>(
@@ -85,25 +86,30 @@ internal class RealMutableStore<Key : Any, Network : Any, Output : Any, Local :
8586
val storeWriteResponse =
8687
try {
8788
// Always write to local first.
88-
delegate.write(writeRequest.key, writeRequest.value)
89-
90-
// Try to sync to network.
91-
val updaterResult = tryUpdateServer(writeRequest)
92-
93-
// Convert UpdaterResult -> StoreWriteResponse.
94-
when (updaterResult) {
95-
is UpdaterResult.Error.Exception -> StoreWriteResponse.Error.Exception(updaterResult.error)
96-
is UpdaterResult.Error.Message -> StoreWriteResponse.Error.Message(updaterResult.message)
97-
is UpdaterResult.Success.Typed<*> -> {
98-
val typedValue = updaterResult.value as? Response
99-
if (typedValue == null) {
100-
StoreWriteResponse.Success.Untyped(updaterResult.value)
101-
} else {
102-
StoreWriteResponse.Success.Typed(updaterResult.value)
89+
// Only proceed to network if local write succeeded.
90+
when (val delegateWriteResult = delegate.write(writeRequest.key, writeRequest.value)) {
91+
is StoreDelegateWriteResult.Error.Exception -> {
92+
StoreWriteResponse.Error.Exception(delegateWriteResult.error)
93+
}
94+
is StoreDelegateWriteResult.Error.Message -> {
95+
StoreWriteResponse.Error.Message(delegateWriteResult.error)
96+
}
97+
is StoreDelegateWriteResult.Success -> {
98+
// Try to sync to network.
99+
when (val updaterResult = tryUpdateServer(writeRequest)) {
100+
is UpdaterResult.Error.Exception -> StoreWriteResponse.Error.Exception(updaterResult.error)
101+
is UpdaterResult.Error.Message -> StoreWriteResponse.Error.Message(updaterResult.message)
102+
is UpdaterResult.Success.Typed<*> -> {
103+
val typedValue = updaterResult.value as? Response
104+
if (typedValue == null) {
105+
StoreWriteResponse.Success.Untyped(updaterResult.value)
106+
} else {
107+
StoreWriteResponse.Success.Typed(updaterResult.value)
108+
}
109+
}
110+
is UpdaterResult.Success.Untyped -> StoreWriteResponse.Success.Untyped(updaterResult.value)
103111
}
104112
}
105-
106-
is UpdaterResult.Success.Untyped -> StoreWriteResponse.Success.Untyped(updaterResult.value)
107113
}
108114
} catch (throwable: Throwable) {
109115
StoreWriteResponse.Error.Exception(throwable)

‎store/src/commonMain/kotlin/org/mobilenativefoundation/store/store5/impl/RealStore.kt‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -332,9 +332,13 @@ internal class RealStore<Key : Any, Network : Any, Output : Any, Local : Any>(
332332
value: Output,
333333
): StoreDelegateWriteResult =
334334
try {
335-
memCache?.put(key, value)
336-
sourceOfTruth?.write(key, converter.fromOutputToLocal(value))
337-
StoreDelegateWriteResult.Success
335+
val writeException = sourceOfTruth?.write(key, converter.fromOutputToLocal(value))
336+
if (writeException != null) {
337+
StoreDelegateWriteResult.Error.Exception(writeException)
338+
} else {
339+
memCache?.put(key, value)
340+
StoreDelegateWriteResult.Success
341+
}
338342
} catch (error: Throwable) {
339343
StoreDelegateWriteResult.Error.Exception(error)
340344
}

‎store/src/commonMain/kotlin/org/mobilenativefoundation/store/store5/impl/SourceOfTruthWithBarrier.kt‎

Lines changed: 24 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -144,11 +144,18 @@ internal class SourceOfTruthWithBarrier<Key : Any, Network : Any, Output : Any,
144144
}
145145
}
146146

147+
/**
148+
* Writes a value to the underlying [SourceOfTruth] and returns any error that occurred.
149+
*
150+
* @return The [SourceOfTruth.WriteException] if the write failed, or null if successful.
151+
* Callers like [RealStore.write] can check this to determine if the write succeeded.
152+
* The barrier mechanism also notifies readers of the error via [BarrierMsg.Open.writeError].
153+
*/
147154
@Suppress("UNCHECKED_CAST")
148155
suspend fun write(
149156
key: Key,
150157
value: Local,
151-
) {
158+
): SourceOfTruth.WriteException? {
152159
val barrier = barriers.acquire(key)
153160
try {
154161
barrier.emit(BarrierMsg.Blocked(versionCounter.incrementAndGet()))
@@ -164,24 +171,27 @@ internal class SourceOfTruthWithBarrier<Key : Any, Network : Any, Output : Any,
164171
}
165172
}
166173

174+
// Avoid double-wrapping if the error is already a WriteException.
175+
val writeException =
176+
writeError?.let {
177+
writeError as? SourceOfTruth.WriteException
178+
?: SourceOfTruth.WriteException(
179+
key = key,
180+
value = value,
181+
cause = writeError,
182+
)
183+
}
184+
167185
barrier.emit(
168186
BarrierMsg.Open(
169187
version = versionCounter.incrementAndGet(),
170-
writeError =
171-
writeError?.let {
172-
SourceOfTruth.WriteException(
173-
key = key,
174-
value = value,
175-
cause = writeError,
176-
)
177-
},
188+
writeError = writeException,
178189
),
179190
)
180-
if (writeError is CancellationException) {
181-
// only throw if it failed because of cancelation.
182-
// otherwise, we take care of letting downstream know that there was a write error
183-
throw writeError
184-
}
191+
192+
// Return the error so callers know the operation failed.
193+
// The barrier message above notifies readers of the error.
194+
return writeException
185195
} finally {
186196
barriers.release(key, barrier)
187197
}

‎store/src/commonTest/kotlin/org/mobilenativefoundation/store/store5/mutablestore/RealMutableStoreTest.kt‎

Lines changed: 110 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import kotlinx.coroutines.flow.toList
1111
import kotlinx.coroutines.test.runTest
1212
import org.mobilenativefoundation.store.core5.ExperimentalStoreApi
1313
import org.mobilenativefoundation.store.store5.FetcherResult
14+
import org.mobilenativefoundation.store.store5.SourceOfTruth
1415
import org.mobilenativefoundation.store.store5.StoreReadRequest
1516
import org.mobilenativefoundation.store.store5.StoreReadResponse
1617
import org.mobilenativefoundation.store.store5.StoreWriteRequest
@@ -297,6 +298,115 @@ class RealMutableStoreTest {
297298
assertNotNull(testBookkeeper.getLastFailedSync("exceptionKey"))
298299
}
299300

301+
@Test
302+
fun write_givenSourceOfTruthFailure_whenCalled_thenSurfacesWriteError() =
303+
runTest {
304+
// Given
305+
val key = "key"
306+
val errorMessage = "write error"
307+
testSourceOfTruth.throwOnWrite(key) {
308+
IllegalStateException(errorMessage)
309+
}
310+
val request =
311+
StoreWriteRequest.of<String, Note, Unit>(
312+
key = key,
313+
value = Note(key, "content"),
314+
created = 3333L,
315+
onCompletions = null,
316+
)
317+
318+
// When
319+
val response = mutableStore.write(request)
320+
321+
// Then
322+
val errorResponse = assertIs<StoreWriteResponse.Error.Exception>(response)
323+
val writeException = assertIs<SourceOfTruth.WriteException>(errorResponse.error)
324+
val cause = assertIs<IllegalStateException>(writeException.cause)
325+
assertEquals(errorMessage, cause.message)
326+
}
327+
328+
@Test
329+
fun write_givenSourceOfTruthFailure_whenCalled_thenNetworkSyncNotAttempted() =
330+
runTest {
331+
// Given
332+
val key = "key"
333+
testUpdater.postCallCount = 0
334+
testSourceOfTruth.throwOnWrite(key) { IllegalStateException("SOT failure") }
335+
336+
val request =
337+
StoreWriteRequest.of<String, Note, Unit>(
338+
key = key,
339+
value = Note(key, "content"),
340+
created = 4444L,
341+
onCompletions = null,
342+
)
343+
344+
// When
345+
val response = mutableStore.write(request)
346+
347+
// Then
348+
assertIs<StoreWriteResponse.Error.Exception>(response)
349+
assertEquals(0, testUpdater.postCallCount, "Network updater should not be called when SOT write fails")
350+
}
351+
352+
@Test
353+
fun write_givenSourceOfTruthFailure_whenCalled_thenMemCacheNotUpdated() =
354+
runTest {
355+
// Given
356+
val key = "key"
357+
testSourceOfTruth.throwOnWrite(key) { IllegalStateException("SOT failure") }
358+
359+
val request =
360+
StoreWriteRequest.of<String, Note, Unit>(
361+
key = key,
362+
value = Note(key, "content"),
363+
created = 6666L,
364+
onCompletions = null,
365+
)
366+
367+
// When
368+
val response = mutableStore.write(request)
369+
370+
// Then
371+
assertIs<StoreWriteResponse.Error.Exception>(response)
372+
assertNull(delegateStore.latestOrNull(key), "Value should not be in cache after SOT write failure")
373+
}
374+
375+
@Test
376+
fun write_givenNoSourceOfTruth_whenCalled_thenSucceeds() =
377+
runTest {
378+
// Given
379+
val storeWithoutSot =
380+
testStore(
381+
fetcher = testFetcher,
382+
sourceOfTruth = null,
383+
converter = testConverter,
384+
validator = testValidator,
385+
memoryCache = testCache,
386+
)
387+
val mutableStoreWithoutSot =
388+
RealMutableStore(
389+
delegate = storeWithoutSot,
390+
updater = testUpdater,
391+
bookkeeper = testBookkeeper,
392+
logger = testLogger,
393+
)
394+
395+
val request =
396+
StoreWriteRequest.of<String, Note, Unit>(
397+
key = "noSotKey",
398+
value = Note("id", "content"),
399+
created = 5555L,
400+
onCompletions = null,
401+
)
402+
403+
// When
404+
val response = mutableStoreWithoutSot.write(request)
405+
406+
// Then
407+
assertIs<StoreWriteResponse.Success>(response)
408+
}
409+
300410
@Test
301411
fun clearAll_givenSomeKeys_whenCalled_thenDelegateIsCleared() =
302412
runTest {

‎store/src/commonTest/kotlin/org/mobilenativefoundation/store/store5/mutablestore/util/TestStore.kt‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ internal fun <Key : Any, Network : Any, Output : Any, Local : Any> testStore(
1414
dispatcher: CoroutineDispatcher = Dispatchers.Default,
1515
scope: CoroutineScope = CoroutineScope(dispatcher),
1616
fetcher: Fetcher<Key, Network> = TestFetcher(),
17-
sourceOfTruth: SourceOfTruth<Key, Local, Output> = TestSourceOfTruth(),
17+
sourceOfTruth: SourceOfTruth<Key, Local, Output>? = TestSourceOfTruth(),
1818
converter: Converter<Network, Local, Output> = TestConverter(),
1919
validator: Validator<Output> = TestValidator(),
2020
memoryCache: Cache<Key, Output> = TestCache(),

‎store/src/commonTest/kotlin/org/mobilenativefoundation/store/store5/mutablestore/util/TestUpdater.kt‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,11 +8,13 @@ class TestUpdater<Key : Any, Output : Any, Response : Any> : Updater<Key, Output
88
var exception: Throwable? = null
99
var errorMessage: String? = null
1010
var successValue: Response? = null
11+
var postCallCount: Int = 0
1112

1213
override suspend fun post(
1314
key: Key,
1415
value: Output,
1516
): UpdaterResult {
17+
postCallCount++
1618
exception?.let { return UpdaterResult.Error.Exception(it) }
1719
errorMessage?.let { return UpdaterResult.Error.Message(it) }
1820
successValue?.let { return UpdaterResult.Success.Typed(it) }

0 commit comments

Comments
 (0)