Skip to content

Commit

Permalink
Introduce CollectionManager for preventing concurrent access
Browse files Browse the repository at this point in the history
Currently there are many instances in the AnkiDroid codebase where
the collection is accessed a) on the UI thread, blocking the UI, and
b) in unsafe ways (eg by checking to see if the collection is open,
and then assuming it will remain open for the rest of a method call.
"Fix full download crashing in new schema case" (159a108) demonstrates
a few of these cases.

This PR is an attempt at addressing those issues. It introduces
a `withCol` function that is intended to be used instead of
CollectionHelper.getCol(). For example, code that previously looked
like:

        val col = CollectionHelper.getInstance().getCol(this)
        val count = col.decks.count()

Can now be used like this:

        val count = withCol { decks.count() }

The block is run on a background thread, and other withCol calls made
in parallel will be queued up and executed sequentially. Because of
the exclusive access, routines can safely close and reopen the
collection inside the block without fear of affecting other callers.

It's not practical to update all the legacy code to use withCol
immediately - too much work is required, and coroutines are not
accessible from Java code. The intention here is that this new path is
gradually bought into. Legacy code can continue calling CollectionHelper.
getCol(), which internally delegates to CollectionManager.

Two caveats to be aware of:
- Legacy callers will wait for other pending operations to complete
before they receive the collection handle, but because they retain it,
subsequent access is not guaranteed to be exclusive.
- Because getCol() and colIsOpen() are often used on the main thread,
they will block the UI if a background operation is already running.
Logging has been added to help diagnose this, eg messages like:

E/CollectionManager: blocked main thread for 2626ms:
    com.ichi2.anki.DeckPicker.onCreateOptionsMenu(DeckPicker.kt:624)

Other changes:

- simplified CoroutineHelpers
- added TR function for accessing translations without a col reference
- onCreateOptionsMenu() needed refactoring to avoid blocking the UI.
- The blocking colIsOpen() call in onPrepareOptionsMenu() had to be
removed. I can not reproduce the issue it reports, and the code checks
for col in onCreateOptionsMenu().
- The subscribers in ChangeManager need to be cleared out at the start
of each test, or two tests are flaky when run with the new schema.
  • Loading branch information
dae committed Aug 6, 2022
1 parent 16b108e commit 5503652
Show file tree
Hide file tree
Showing 18 changed files with 590 additions and 326 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -853,8 +853,8 @@ abstract class AbstractFlashcardViewer :
if (BackendFactory.defaultLegacySchema) {
legacyUndo()
} else {
return launchCatchingCollectionTask { col ->
if (!backendUndoAndShowPopup(col)) {
return launchCatchingTask {
if (!backendUndoAndShowPopup()) {
legacyUndo()
}
}
Expand Down
24 changes: 11 additions & 13 deletions AnkiDroid/src/main/java/com/ichi2/anki/BackendBackups.kt
Original file line number Diff line number Diff line change
Expand Up @@ -18,47 +18,45 @@

package com.ichi2.anki

import com.ichi2.libanki.CollectionV16
import com.ichi2.anki.CollectionManager.withCol
import com.ichi2.libanki.awaitBackupCompletion
import com.ichi2.libanki.createBackup
import kotlinx.coroutines.*

fun DeckPicker.performBackupInBackground() {
launchCatchingCollectionTask { col ->
launchCatchingTask {
// Wait a second to allow the deck list to finish loading first, or it
// will hang until the first stage of the backup completes.
delay(1000)
createBackup(col, false)
createBackup(force = false)
}
}

fun DeckPicker.importColpkg(colpkgPath: String) {
launchCatchingTask {
val helper = CollectionHelper.getInstance()
val backend = helper.getOrCreateBackend(baseContext)
runInBackgroundWithProgress(
backend,
withProgress(
extractProgress = {
if (progress.hasImporting()) {
text = progress.importing
}
},
) {
helper.importColpkg(baseContext, colpkgPath)
CollectionManager.importColpkg(colpkgPath)
}

invalidateOptionsMenu()
updateDeckList()
}
}

private suspend fun createBackup(col: CollectionV16, force: Boolean) {
runInBackground {
private suspend fun createBackup(force: Boolean) {
withCol {
// this two-step approach releases the backend lock after the initial copy
col.createBackup(
BackupManager.getBackupDirectoryFromCollection(col.path),
newBackend.createBackup(
BackupManager.getBackupDirectoryFromCollection(this.path),
force,
waitForCompletion = false
)
col.awaitBackupCompletion()
newBackend.awaitBackupCompletion()
}
}
17 changes: 9 additions & 8 deletions AnkiDroid/src/main/java/com/ichi2/anki/BackendImporting.kt
Original file line number Diff line number Diff line change
Expand Up @@ -19,24 +19,24 @@
package com.ichi2.anki

import anki.import_export.ImportResponse
import com.ichi2.anki.CollectionManager.withCol
import com.ichi2.libanki.exportAnkiPackage
import com.ichi2.libanki.importAnkiPackage
import com.ichi2.libanki.undoableOp
import net.ankiweb.rsdroid.Translations

fun DeckPicker.importApkgs(apkgPaths: List<String>) {
launchCatchingCollectionTask { col ->
launchCatchingTask {
for (apkgPath in apkgPaths) {
val report = runInBackgroundWithProgress(
col.backend,
val report = withProgress(
extractProgress = {
if (progress.hasImporting()) {
text = progress.importing
}
},
) {
undoableOp {
col.importAnkiPackage(apkgPath)
importAnkiPackage(apkgPath)
}
}
showSimpleMessageDialog(summarizeReport(col.tr, report))
Expand Down Expand Up @@ -69,16 +69,17 @@ fun DeckPicker.exportApkg(
withMedia: Boolean,
deckId: Long?
) {
launchCatchingCollectionTask { col ->
runInBackgroundWithProgress(
col.backend,
launchCatchingTask {
withProgress(
extractProgress = {
if (progress.hasExporting()) {
text = progress.exporting
}
},
) {
col.exportAnkiPackage(apkgPath, withScheduling, withMedia, deckId)
withCol {
newBackend.exportAnkiPackage(apkgPath, withScheduling, withMedia, deckId)
}
}
}
}
7 changes: 3 additions & 4 deletions AnkiDroid/src/main/java/com/ichi2/anki/BackendUndo.kt
Original file line number Diff line number Diff line change
Expand Up @@ -17,17 +17,16 @@
package com.ichi2.anki

import com.ichi2.anki.UIUtils.showSimpleSnackbar
import com.ichi2.libanki.CollectionV16
import com.ichi2.libanki.undoNew
import com.ichi2.libanki.undoableOp
import com.ichi2.utils.BlocksSchemaUpgrade
import net.ankiweb.rsdroid.BackendException

suspend fun AnkiActivity.backendUndoAndShowPopup(col: CollectionV16): Boolean {
suspend fun AnkiActivity.backendUndoAndShowPopup(): Boolean {
return try {
val changes = runInBackgroundWithProgress() {
val changes = withProgress() {
undoableOp {
col.undoNew()
undoNew()
}
}
showSimpleSnackbar(
Expand Down
4 changes: 2 additions & 2 deletions AnkiDroid/src/main/java/com/ichi2/anki/CardBrowser.kt
Original file line number Diff line number Diff line change
Expand Up @@ -1282,8 +1282,8 @@ open class CardBrowser :
if (BackendFactory.defaultLegacySchema) {
Undo().runWithHandler(mUndoHandler)
} else {
launchCatchingCollectionTask { col ->
if (!backendUndoAndShowPopup(col)) {
launchCatchingTask {
if (!backendUndoAndShowPopup()) {
Undo().runWithHandler(mUndoHandler)
}
}
Expand Down
74 changes: 6 additions & 68 deletions AnkiDroid/src/main/java/com/ichi2/anki/CollectionHelper.java
Original file line number Diff line number Diff line change
Expand Up @@ -51,15 +51,9 @@
* Singleton which opens, stores, and closes the reference to the Collection.
*/
public class CollectionHelper {

// Collection instance belonging to sInstance
private Collection mCollection;
// Name of anki2 file
public static final String COLLECTION_FILENAME = "collection.anki2";

// A backend instance is reused after collection close.
private @Nullable Backend mBackend;

/**
* The preference key for the path to the current AnkiDroid directory
* <br>
Expand Down Expand Up @@ -143,54 +137,19 @@ public static CollectionHelper getInstance() {
*/
private Collection openCollection(Context context, String path) {
Timber.i("Begin openCollection: %s", path);
Backend backend = getOrCreateBackend(context);
Backend backend = BackendFactory.getBackend(context);
Collection collection = Storage.collection(context, path, false, true, backend);
Timber.i("End openCollection: %s", path);
return collection;
}

synchronized @NonNull Backend getOrCreateBackend(Context context) {
if (mBackend == null) {
mBackend = BackendFactory.getBackend(context);
}
return mBackend;
}


/**
* Close the currently cached backend and discard it. Useful when enabling the V16 scheduler in the
* dev preferences, or if the active language changes. The collection should be closed before calling
* this.
*/
public synchronized void discardBackend() {
if (mBackend != null) {
mBackend.close();
mBackend = null;
}
}

/**
* Get the single instance of the {@link Collection}, creating it if necessary (lazy initialization).
* @param _context is no longer used, as the global AnkidroidApp instance is used instead
* @return instance of the Collection
*/
public synchronized Collection getCol(Context _context) {
// Open collection
Context context = AnkiDroidApp.getInstance();
if (!colIsOpen()) {
String path = getCollectionPath(context);
// Check that the directory has been created and initialized
try {
initializeAnkiDroidDirectory(getParentDirectory(path));
// Path to collection, cached for the reopenCollection() method
} catch (StorageAccessException e) {
Timber.e(e, "Could not initialize AnkiDroid directory");
return null;
}
// Open the database
mCollection = openCollection(context, path);
}
return mCollection;
public synchronized Collection getCol(Context context) {
return CollectionManager.getColUnsafe();
}

/**
Expand Down Expand Up @@ -260,29 +219,15 @@ public synchronized Collection getColSafe(Context context) {
*/
public synchronized void closeCollection(boolean save, String reason) {
Timber.i("closeCollection: %s", reason);
if (mCollection != null) {
mCollection.close(save);
}
CollectionManager.closeCollectionBlocking(save);
}

/**
* Replace the collection with the provided colpkg file if it is valid.
*/
public synchronized void importColpkg(Context context, String colpkgPath) {
Backend backend = getOrCreateBackend(context);
if (mCollection != null) {
mCollection.close(true);
}
String colPath = getCollectionPath(context);
importCollectionPackage(backend, colPath, colpkgPath);
getCol(context);
}

/**
* @return Whether or not {@link Collection} and its child database are open.
*/
public boolean colIsOpen() {
return mCollection != null && !mCollection.isDbClosed();
return CollectionManager.isOpenUnsafe();
}

/**
Expand Down Expand Up @@ -621,13 +566,6 @@ public enum CollectionOpenFailure {

@VisibleForTesting(otherwise = VisibleForTesting.NONE)
public void setColForTests(Collection col) {
if (col == null) {
try {
mCollection.close();
} catch (Exception exc) {
// may not be open
}
}
this.mCollection = col;
CollectionManager.setColForTests(col);
}
}
Loading

0 comments on commit 5503652

Please sign in to comment.