wbrawner/thunderbird-android
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Change the Storage class to be immutable

Browse source 
Also make sure the in-memory state and the database are modified together while a lock is being held.
This commit is contained in:
cketti 2022-02-11 02:54:39 +01:00
parent ec04a1c827
commit ffa0ebb5bb
9 changed files with 142 additions and 189 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

20
app/core/src/main/java/com/fsck/k9/Preferences.kt
View file

@ -32,6 +32,7 @@ class Preferences internal constructor(
private val backgroundDispatcher: CoroutineDispatcher = Dispatchers.IO
) : AccountManager {
private val accountLock = Any()
private val storageLock = Any()
@GuardedBy("accountLock")
private var accountsMap: MutableMap<String, Account>? = null
@ -44,15 +45,22 @@ class Preferences internal constructor(
private val accountsChangeListeners = CopyOnWriteArraySet<AccountsChangeListener>()
private val accountRemovedListeners = CopyOnWriteArraySet<AccountRemovedListener>()
val storage = Storage()
@GuardedBy("storageLock")
private var currentStorage: Storage? = null
init {
val persistedStorageValues = storagePersister.loadValues()
storage.replaceAll(persistedStorageValues)
}
val storage: Storage
get() = synchronized(storageLock) {
currentStorage ?: storagePersister.loadValues().also { newStorage ->
currentStorage = newStorage
}
}
fun createStorageEditor(): StorageEditor {
return storagePersister.createStorageEditor(storage)
return storagePersister.createStorageEditor { updater ->
synchronized(storageLock) {
currentStorage = updater(storage)
}
}
}
@RestrictTo(RestrictTo.Scope.TESTS)

25
app/core/src/main/java/com/fsck/k9/preferences/Storage.java
View file

@ -2,30 +2,31 @@ package com.fsck.k9.preferences;
import java.util.Collections;
import java.util.HashMap;
import java.util.Map;
import timber.log.Timber;
public class Storage {
private volatile Map<String, String> storage = Collections.emptyMap();
private final Map<String, String> values;
public Storage() { }
public Storage(Map<String, String> values) {
this.values = Collections.unmodifiableMap(values);
}
public boolean isEmpty() {
return storage.isEmpty();
return values.isEmpty();
}
public boolean contains(String key) {
return storage.containsKey(key);
return values.containsKey(key);
}
public Map<String, String> getAll() {
return storage;
return values;
}
public boolean getBoolean(String key, boolean defValue) {
String val = storage.get(key);
String val = values.get(key);
if (val == null) {
return defValue;
}
@ -33,7 +34,7 @@ public class Storage {
}
public int getInt(String key, int defValue) {
String val = storage.get(key);
String val = values.get(key);
if (val == null) {
return defValue;
}
@ -46,7 +47,7 @@ public class Storage {
}
public long getLong(String key, long defValue) {
String val = storage.get(key);
String val = values.get(key);
if (val == null) {
return defValue;
}
@ -59,14 +60,10 @@ public class Storage {
}
public String getString(String key, String defValue) {
String val = storage.get(key);
String val = values.get(key);
if (val == null) {
return defValue;
}
return val;
}
public void replaceAll(Map<String, String> workingStorage) {
storage = new HashMap<>(workingStorage);
}
}

11
app/core/src/main/java/com/fsck/k9/preferences/StoragePersister.kt
View file

@ -1,10 +1,11 @@
package com.fsck.k9.preferences
import androidx.annotation.CheckResult
interface StoragePersister {
@CheckResult
fun loadValues(): Map<String, String>
fun loadValues(): Storage
fun createStorageEditor(storage: Storage): StorageEditor
fun createStorageEditor(storageUpdater: StorageUpdater): StorageEditor
}
fun interface StorageUpdater {
fun updateStorage(updater: (currentStorage: Storage) -> Storage)
}

79
app/core/src/test/java/com/fsck/k9/preferences/StorageTest.kt
View file

@ -1,79 +0,0 @@
package com.fsck.k9.preferences
import org.junit.Assert.assertEquals
import org.junit.Assert.assertFalse
import org.junit.Assert.assertTrue
import org.junit.Test
private const val TEST_STRING_KEY = "s"
private const val TEST_STRING_VALUE = "y"
private const val TEST_INT_KEY = "i"
private const val TEST_INT_VALUE = "4"
private const val TEST_STRING_DEFAULT = "z"
private const val TEST_INT_DEFAULT = 2
private val TEST_MAP = mapOf(
TEST_STRING_KEY to TEST_STRING_VALUE,
TEST_INT_KEY to TEST_INT_VALUE
)
class StorageTest {
internal var storage = Storage()
@Test
fun isEmpty() {
assertTrue(storage.isEmpty)
}
@Test
fun isNotEmpty() {
storage.replaceAll(TEST_MAP)
assertFalse(storage.isEmpty)
}
@Test
fun contains() {
storage.replaceAll(TEST_MAP)
assertTrue(storage.contains(TEST_STRING_KEY))
}
@Test
fun getString() {
storage.replaceAll(TEST_MAP)
assertEquals(TEST_STRING_VALUE, storage.getString(TEST_STRING_KEY, TEST_STRING_DEFAULT))
}
@Test
fun getString_default() {
assertTrue(storage.isEmpty)
assertEquals(TEST_STRING_DEFAULT, storage.getString(TEST_STRING_KEY, TEST_STRING_DEFAULT))
}
@Test
fun getAll() {
storage.replaceAll(TEST_MAP)
assertFalse(storage.isEmpty)
assertEquals(TEST_MAP, storage.all)
}
@Test
fun replaceAll() {
storage.replaceAll(TEST_MAP)
storage.replaceAll(emptyMap())
assertTrue(storage.isEmpty)
}
@Test
fun getInteger() {
storage.replaceAll(TEST_MAP)
assertEquals(Integer.parseInt(TEST_INT_VALUE), storage.getInt(TEST_INT_KEY, TEST_INT_DEFAULT))
}
@Test
fun getInteger_stringValue() {
storage.replaceAll(TEST_MAP)
// TODO is this good behavior?
assertEquals(TEST_INT_DEFAULT, storage.getInt(TEST_STRING_KEY, TEST_INT_DEFAULT))
}
}

23
app/storage/src/main/java/com/fsck/k9/preferences/K9StorageEditor.java
View file

@ -15,18 +15,16 @@ import timber.log.Timber;
public class K9StorageEditor implements StorageEditor {
private Storage storage;
private StorageUpdater storageUpdater;
private K9StoragePersister storagePersister;
private Map<String, String> changes = new HashMap<>();
private List<String> removals = new ArrayList<>();
private Map<String, String> snapshot = new HashMap<>();
public K9StorageEditor(Storage storage, K9StoragePersister storagePersister) {
this.storage = storage;
public K9StorageEditor(StorageUpdater storageUpdater, K9StoragePersister storagePersister) {
this.storageUpdater = storageUpdater;
this.storagePersister = storagePersister;
snapshot.putAll(storage.getAll());
}
@Override
@ -47,7 +45,7 @@ public class K9StorageEditor implements StorageEditor {
@Override
public boolean commit() {
try {
commitChanges();
storageUpdater.updateStorage(this::commitChanges);
return true;
} catch (Exception e) {
Timber.e(e, "Failed to save preferences");
@ -55,13 +53,16 @@ public class K9StorageEditor implements StorageEditor {
}
}
private void commitChanges() {
private Storage commitChanges(Storage storage) {
long startTime = SystemClock.elapsedRealtime();
Timber.i("Committing preference changes");
Map<String, String> newValues = new HashMap<>();
Map<String, String> oldValues = storage.getAll();
StoragePersistOperationCallback committer = new StoragePersistOperationCallback() {
@Override
public void beforePersistTransaction(Map<String, String> workingStorage) {
workingStorage.putAll(storage.getAll());
workingStorage.putAll(oldValues);
}
@Override
@ -72,7 +73,7 @@ public class K9StorageEditor implements StorageEditor {
for (Entry<String, String> entry : changes.entrySet()) {
String key = entry.getKey();
String newValue = entry.getValue();
String oldValue = snapshot.get(key);
String oldValue = oldValues.get(key);
if (removals.contains(key) || !newValue.equals(oldValue)) {
ops.put(key, newValue);
}
@ -81,12 +82,14 @@ public class K9StorageEditor implements StorageEditor {
@Override
public void onPersistTransactionSuccess(Map<String, String> workingStorage) {
storage.replaceAll(workingStorage);
newValues.putAll(workingStorage);
}
};
storagePersister.doInTransaction(committer);
long endTime = SystemClock.elapsedRealtime();
Timber.i("Preferences commit took %d ms", endTime - startTime);
return new Storage(newValues);
}
@Override

13
app/storage/src/main/java/com/fsck/k9/preferences/K9StoragePersister.java
View file

@ -10,8 +10,8 @@ import android.database.Cursor;
import android.database.sqlite.SQLiteDatabase;
import android.database.sqlite.SQLiteStatement;
import android.os.SystemClock;
import androidx.annotation.CheckResult;
import androidx.annotation.NonNull;
import com.fsck.k9.helper.Utility;
import com.fsck.k9.preferences.migrations.StorageMigrations;
import com.fsck.k9.preferences.migrations.StorageMigrationsHelper;
@ -88,9 +88,10 @@ public class K9StoragePersister implements StoragePersister {
}
}
@NonNull
@Override
public StorageEditor createStorageEditor(Storage storage) {
return new K9StorageEditor(storage, this);
public StorageEditor createStorageEditor(@NonNull StorageUpdater storageUpdater) {
return new K9StorageEditor(storageUpdater, this);
}
static class StoragePersistOperations {
@ -136,14 +137,14 @@ public class K9StoragePersister implements StoragePersister {
void onPersistTransactionSuccess(Map<String, String> workingStorage);
}
@NonNull
@Override
@CheckResult
public Map<String, String> loadValues() {
public Storage loadValues() {
long startTime = SystemClock.elapsedRealtime();
Timber.i("Loading preferences from DB into Storage");
try (SQLiteDatabase database = openDB()) {
return readAllValues(database);
return new Storage(readAllValues(database));
} finally {
long endTime = SystemClock.elapsedRealtime();
Timber.i("Preferences load took %d ms", endTime - startTime);

138
app/storage/src/test/java/com/fsck/k9/preferences/StorageEditorTest.kt
View file

@ -1,47 +1,39 @@
package com.fsck.k9.preferences
import com.fsck.k9.preferences.K9StoragePersister.StoragePersistOperationCallback
import com.fsck.k9.preferences.K9StoragePersister.StoragePersistOperations
import com.fsck.k9.storage.K9RobolectricTest
import org.junit.Assert.assertEquals
import org.junit.Assert.assertFalse
import org.junit.Assert.assertTrue
import org.junit.Before
import com.google.common.truth.Truth.assertThat
import org.junit.Test
import org.mockito.ArgumentMatchers.any
import org.mockito.ArgumentMatchers.eq
import org.mockito.Mock
import org.mockito.Mockito.times
import org.mockito.Mockito.verify
import org.mockito.MockitoAnnotations
import org.mockito.kotlin.doAnswer
import org.mockito.kotlin.doThrow
import org.mockito.kotlin.mock
import org.mockito.kotlin.stubbing
import org.mockito.kotlin.verify
import org.mockito.kotlin.verifyNoMoreInteractions
import org.mockito.kotlin.whenever
class StorageEditorTest : K9RobolectricTest() {
@Mock private lateinit var storage: Storage
@Mock private lateinit var storagePersister: K9StoragePersister
@Mock private lateinit var storagePersisterOps: K9StoragePersister.StoragePersistOperations
private lateinit var editor: K9StorageEditor
private val storage: Storage = Storage(mapOf("storage-key" to "storage-value"))
private val storageUpdater = TestStorageUpdater(storage)
private val storagePersister = mock<K9StoragePersister>()
private val storagePersisterOps = mock<StoragePersistOperations>()
private val editor = K9StorageEditor(storageUpdater, storagePersister)
private val workingMap = mutableMapOf<String, String>()
private val storageMap = mapOf(
"storage-key" to "storage-value"
)
@Before
fun setUp() {
MockitoAnnotations.initMocks(this)
whenever(storage.all).thenReturn(storageMap)
editor = K9StorageEditor(storage, storagePersister)
verify(storage).all
}
private val newValues: Map<String, String>
get() = storageUpdater.newStorage!!.all
@Test
fun commit_exception() {
whenever(storagePersister.doInTransaction(any())).thenThrow(RuntimeException())
stubbing(storagePersister) {
on { doInTransaction(any()) } doThrow RuntimeException()
}
val success = editor.commit()
assertFalse(success)
assertThat(success).isFalse()
}
@Test
@ -50,7 +42,9 @@ class StorageEditorTest : K9RobolectricTest() {
val success = editor.commit()
assertTrue(success)
assertThat(success).isTrue()
assertThat(newValues).isEqualTo(storage.all)
verifyNoMoreInteractions(storagePersisterOps)
}
@ -61,7 +55,8 @@ class StorageEditorTest : K9RobolectricTest() {
editor.putBoolean("x", true)
val success = editor.commit()
assertTrue(success)
assertThat(success).isTrue()
assertThat(newValues).isEqualTo(mapOf("storage-key" to "storage-value", "x" to "true"))
verify(storagePersisterOps).put("x", "true")
verifyNoMoreInteractions(storagePersisterOps)
}
@ -73,7 +68,8 @@ class StorageEditorTest : K9RobolectricTest() {
editor.putInt("x", 123)
val success = editor.commit()
assertTrue(success)
assertThat(success).isTrue()
assertThat(newValues).isEqualTo(mapOf("storage-key" to "storage-value", "x" to "123"))
verify(storagePersisterOps).put("x", "123")
verifyNoMoreInteractions(storagePersisterOps)
}
@ -85,7 +81,8 @@ class StorageEditorTest : K9RobolectricTest() {
editor.putLong("x", 1234)
val success = editor.commit()
assertTrue(success)
assertThat(success).isTrue()
assertThat(newValues).isEqualTo(mapOf("storage-key" to "storage-value", "x" to "1234"))
verify(storagePersisterOps).put("x", "1234")
verifyNoMoreInteractions(storagePersisterOps)
}
@ -97,7 +94,8 @@ class StorageEditorTest : K9RobolectricTest() {
editor.putString("x", "y")
val success = editor.commit()
assertTrue(success)
assertThat(success).isTrue()
assertThat(newValues).isEqualTo(mapOf("storage-key" to "storage-value", "x" to "y"))
verify(storagePersisterOps).put("x", "y")
verifyNoMoreInteractions(storagePersisterOps)
}
@ -109,7 +107,8 @@ class StorageEditorTest : K9RobolectricTest() {
editor.putString("storage-key", "storage-value")
val success = editor.commit()
assertTrue(success)
assertThat(success).isTrue()
assertThat(newValues).isEqualTo(mapOf("storage-key" to "storage-value"))
verifyNoMoreInteractions(storagePersisterOps)
}
@ -120,7 +119,8 @@ class StorageEditorTest : K9RobolectricTest() {
editor.putString("storage-key", "other-value")
val success = editor.commit()
assertTrue(success)
assertThat(success).isTrue()
assertThat(newValues).isEqualTo(mapOf("storage-key" to "other-value"))
verify(storagePersisterOps).put("storage-key", "other-value")
verifyNoMoreInteractions(storagePersisterOps)
}
@ -133,51 +133,71 @@ class StorageEditorTest : K9RobolectricTest() {
editor.putString("storage-key", "storage-value")
val success = editor.commit()
assertTrue(success)
assertThat(success).isTrue()
assertThat(newValues).isEqualTo(mapOf("storage-key" to "storage-value"))
verify(storagePersisterOps).remove("storage-key")
verify(storagePersisterOps).put("storage-key", "storage-value")
verifyNoMoreInteractions(storagePersisterOps)
}
@Test
fun `remove key that doesn't exist`() {
prepareStoragePersisterMock()
editor.remove("x")
val success = editor.commit()
assertThat(success).isTrue()
assertThat(newValues).isEqualTo(mapOf("storage-key" to "storage-value"))
verify(storagePersisterOps).remove("x")
verifyNoMoreInteractions(storagePersisterOps)
}
@Test
fun remove() {
prepareStoragePersisterMock()
editor.remove("x")
editor.remove("storage-key")
val success = editor.commit()
assertTrue(success)
verify(storagePersisterOps).remove("x")
assertThat(success).isTrue()
assertThat(newValues).isEmpty()
verify(storagePersisterOps).remove("storage-key")
verifyNoMoreInteractions(storagePersisterOps)
}
private fun prepareStoragePersisterMock() {
whenever(storagePersisterOps.put(any(), any())).then {
val key = it.getArgument<String>(0)
val value = it.getArgument<String>(0)
stubbing(storagePersisterOps) {
on { put(any(), any()) } doAnswer {
val key = it.getArgument<String>(0)
val value = it.getArgument<String>(1)
workingMap[key] = value
}
workingMap[key] = value
Unit
}
whenever(storagePersisterOps.remove(any())).then {
val key = it.getArgument<String>(0)
val value = it.getArgument<String>(0)
on { remove(any()) } doAnswer {
val key = it.getArgument<String>(0)
workingMap.remove(key)
workingMap[key] = value
Unit
Unit
}
}
whenever(storagePersister.doInTransaction(any())).then {
val operationCallback = it.getArgument<K9StoragePersister.StoragePersistOperationCallback>(0)
operationCallback.beforePersistTransaction(workingMap)
verify(storage, times(2)).all
assertEquals(workingMap, storageMap)
stubbing(storagePersister) {
on { doInTransaction(any()) } doAnswer {
val operationCallback = it.getArgument<StoragePersistOperationCallback>(0)
operationCallback.persist(storagePersisterOps)
verify(storagePersister).doInTransaction(any())
operationCallback.onPersistTransactionSuccess(workingMap)
verify(storage).replaceAll(eq(workingMap))
operationCallback.beforePersistTransaction(workingMap)
operationCallback.persist(storagePersisterOps)
operationCallback.onPersistTransactionSuccess(workingMap)
}
}
}
}
class TestStorageUpdater(private val currentStorage: Storage) : StorageUpdater {
var newStorage: Storage? = null
override fun updateStorage(updater: (currentStorage: Storage) -> Storage) {
newStorage = updater(currentStorage)
}
}

2
app/storage/src/test/java/com/fsck/k9/preferences/StoragePersisterTest.kt
View file

@ -46,7 +46,7 @@ class StoragePersisterTest : K9RobolectricTest() {
storagePersister.doInTransaction(operationCallback)
val values = storagePersister.loadValues()
val values = storagePersister.loadValues().all
assertEquals(1, values.size)
assertEquals("y", values["x"])
}

20
app/testing/src/main/java/com/fsck/k9/preferences/InMemoryStoragePersister.kt
View file

@ -5,14 +5,15 @@ import android.content.SharedPreferences
class InMemoryStoragePersister : StoragePersister {
private val values = mutableMapOf<String, Any?>()
override fun loadValues(): Map<String, String> {
return values.mapValues { (_, value) -> value?.toString() ?: "" }
override fun loadValues(): Storage {
return Storage(values.mapValues { (_, value) -> value?.toString() ?: "" })
}
override fun createStorageEditor(storage: Storage): StorageEditor = InMemoryStorageEditor(storage)
override fun createStorageEditor(storageUpdater: StorageUpdater): StorageEditor {
return InMemoryStorageEditor(storageUpdater)
}
private inner class InMemoryStorageEditor(private val storage: Storage) : StorageEditor {
private val snapshot = storage.all.toMutableMap()
private inner class InMemoryStorageEditor(private val storageUpdater: StorageUpdater) : StorageEditor {
private val removals = mutableSetOf<String>()
private val changes = mutableMapOf<String, String>()
private var alreadyCommitted = false
@ -52,12 +53,13 @@ class InMemoryStoragePersister : StoragePersister {
if (alreadyCommitted) throw AssertionError("StorageEditor.commit() called more than once")
alreadyCommitted = true
removals.forEach { snapshot.remove(it) }
snapshot.putAll(changes)
storage.replaceAll(snapshot)
storageUpdater.updateStorage(::writeValues)
return true
}
private fun writeValues(currentStorage: Storage): Storage {
return Storage(currentStorage.all - removals + changes)
}
}
}