Don't run AccountsUpdatedListener while account is being renamed

Don't run AccountsUpdatedListener while account is being renamed (closes bitfireAT/davx5#135)

2 files changed, 36 insertions, 2 deletions

diff --git a/app/src/main/java/at/bitfire/davdroid/syncadapter/AccountsUpdatedListener.kt b/app/src/main/java/at/bitfire/davdroid/syncadapter/AccountsUpdatedListener.kt
index 68804cb7..03caa3f1 100644
--- a/app/src/main/java/at/bitfire/davdroid/syncadapter/AccountsUpdatedListener.kt
+++ b/app/src/main/java/at/bitfire/davdroid/syncadapter/AccountsUpdatedListener.kt
@@ -23,6 +23,7 @@ import dagger.hilt.components.SingletonComponent
 import kotlinx.coroutines.CoroutineScope
 import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.launch
+import java.util.concurrent.Semaphore
 import java.util.logging.Level
 import javax.inject.Singleton
 
@@ -44,6 +45,13 @@ class AccountsUpdatedListener private constructor(
         fun appDatabase(): AppDatabase
     }
 
+    /**
+     * This mutex (semaphore with 1 permit) will be acquired by [onAccountsUpdated]. So if you
+     * want to postpone [onAccountsUpdated] execution because you're modifying accounts non-transactionally,
+     * you can acquire the mutex by yourself. Don't forget to release it as soon as you're done.
+     */
+    val mutex = Semaphore(1)
+
 
     fun listen() {
         val accountManager = AccountManager.get(context)
@@ -56,13 +64,21 @@ class AccountsUpdatedListener private constructor(
      *
      * 1. Remove related address book accounts.
      * 2. Remove related service entries from the [AppDatabase].
+     *
+     * Before the accounts are cleaned up, the [mutex] is acquired.
+     * After the accounts are cleaned up, the [mutex] is released.
      */
     @AnyThread
     override fun onAccountsUpdated(accounts: Array<out Account>) {
         /* onAccountsUpdated may be called from the main thread, but cleanupAccounts
            requires disk (database) access. So we launch it in a separate thread. */
         CoroutineScope(Dispatchers.Default).launch {
-            cleanupAccounts(context, accounts)
+            try {
+                mutex.acquire()
+                cleanupAccounts(context, accounts)
+            } finally {
+                mutex.release()
+            }
         }
     }
 
diff --git a/app/src/main/java/at/bitfire/davdroid/ui/account/RenameAccountFragment.kt b/app/src/main/java/at/bitfire/davdroid/ui/account/RenameAccountFragment.kt
index ca8c3985..b45c2c5e 100644
--- a/app/src/main/java/at/bitfire/davdroid/ui/account/RenameAccountFragment.kt
+++ b/app/src/main/java/at/bitfire/davdroid/ui/account/RenameAccountFragment.kt
@@ -36,6 +36,7 @@ import at.bitfire.davdroid.log.Logger
 import at.bitfire.davdroid.resource.LocalAddressBook
 import at.bitfire.davdroid.resource.LocalTaskList
 import at.bitfire.davdroid.settings.AccountSettings
+import at.bitfire.davdroid.syncadapter.AccountsUpdatedListener
 import at.bitfire.ical4android.TaskProvider
 import com.google.android.material.dialog.MaterialAlertDialogBuilder
 import dagger.hilt.android.AndroidEntryPoint
@@ -105,6 +106,7 @@ class RenameAccountFragment: DialogFragment() {
     @HiltViewModel
     class Model @Inject constructor(
         @ApplicationContext val context: Context,
+        val accountsUpdatedListener: AccountsUpdatedListener,
         val db: AppDatabase
     ): ViewModel() {
 
@@ -129,11 +131,27 @@ class RenameAccountFragment: DialogFragment() {
 
             val accountManager = AccountManager.get(context)
             try {
+                /* https://github.com/bitfireAT/davx5/issues/135
+                Take AccountsUpdatedListenerLock so that the AccountsUpdateListener doesn't run while we rename the account
+                because this can cause problems when:
+                1. The account is renamed.
+                2. The AccountsUpdateListener is called BEFORE the services table is updated.
+                   → AccountsUpdateListener removes the "orphaned" services because they belong to the old account which doesn't exist anymore
+                3. Now the services would be renamed, but they're not here anymore. */
+                accountsUpdatedListener.mutex.acquire()
+
                 accountManager.renameAccount(oldAccount, newName, {
                     if (it.result?.name == newName /* success */)
                         viewModelScope.launch(Dispatchers.Default + NonCancellable) {
                             onAccountRenamed(accountManager, oldAccount, newName, syncIntervals)
-                        }
+
+                            // release AccountsUpdatedListener mutex at the end of this async coroutine
+                            accountsUpdatedListener.mutex.release()
+                        } else
+                            // release AccountsUpdatedListener mutex now
+                            accountsUpdatedListener.mutex.release()
+
+
                 }, null)
             } catch (e: Exception) {
                 Logger.log.log(Level.WARNING, "Couldn't rename account", e)