Discussion of withCol migration

[ShridharGoel]

Edit by @Arthur-Milchior .
This issue is here for reviewers to discuss the details of the migration. If you just want to contribute, you can go on #12495 to see how to do it. Feel free to ignore this issue, or on the contrary, to read it entirely to see how we reached an agreement.

---

Original post:

This was suggested by @Arthur-Milchior over Discord.

Replace usages of getCol with the new implementation: withCol.

This can be started by searching for CollectionHelper.instance.getCol and replacing one by one. Would be good to have different PRs for each different usage.

Note that in ReminderService, it's used like this:

colHelper = CollectionHelper.instance
col = colHelper.getCol(context)

Mentioning this so that this one is also easy to find.

This task would require working with coroutines since withCol is a suspend method. It's implementation can be found in CollectionManager.

[dae]

For some background on why such a change is recommended, see #11849

Because withCol is a coroutine, it needs to be called in a coroutine context - either inside another suspend function, or by launching an asynchronous task, eg

val col = colHelper.getCol()
val someValue = col.doSomething()
showSnackbar(`got $someValue`)

might become

launchCatchingTask {
  val someValue = withCol { doSomething() }
  showSnackbar(`got $someValue`)
}

Note how code that depends on the result of some collection routine needs to be placed inside the launched task, which is
run asynchronously. Also note how UI code must be outside of the withCol call.

Also note that a lot of getCol() calls are implicit - AnkiActivity defines a col property that delegates to CollectionHelper:

    override val col: Collection
        get() = CollectionHelper.instance.getCol(this)!!

This means any time you see code accessing col, it's likely using getCol() and needs updating.

[Arthur-Milchior]

Thanks a lot for creating the issue and adding explanation.
I add a "good first issue" tag as a test. I believe many such changes can be easy.
If it does not work I’ll remove it.

Note that some objects such as card, note, deck manager, keep a copy of it’s collection. We should use this copy. Indeed this is the main way we ensure that if we have a card froma collection, the changes we make are saved to the same collection.

Admittesly since there is a single collection today, there is no risk.

[dae]

Sadly the current API passes a reference to the collection into each Card/Note object, meaning that calls to things like card.note() result in a DB query. To ensure this is correctly guarded, any methods that invoke a DB query should be invoked inside a withCol block, eg:

val note = withCol { card.note() }

In the long term, upstream should avoid passing a collection handle into cards/notes, so that collection references are explicit.

[Arthur-Milchior]

I tried to create a page that explains to someone who is new to withCol everything they need to know, without needing to look at various PR and issues to get context. Which also explains how to do the migration.
One of the goal is that each of you, who know this field correctly, can edit the page. So that a new contributor don't see a list of people disagreeing with each other and fear that the issue is very complex. Instead, they'll see a currently up-to-date document listing best practice.
So, I'd love if you could review it, both of you, it's on https://github.com/ankidroid/Anki-Android/wiki/Accessing-the-collection
, if I got something wrong, correct it, or just let me know and I'll edit it. And once it's okay to show it to contributors who want to help us and discover, I'll create a new issue from scratch, that don't have all of this discussion, and just redirect to the wiki.

Does it seems good to you?

[Arthur-Milchior]

Quoting #11849

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() }

It seems that @david-allison has some objection with this particular example (that I reused). In this case because decks already caches the set of decks and hence the number of decks. And so this should not require an async call unless maybe we are really at the first time we fetch the deck manager.

Sadly, discussion was on discord, so I try to recap it here; first because I know you are not there @dae ; and also to keep track of it for future.

Which begs the question, do we consider that we can still references to decks and models, or are we always forced to take them through the collection. I admit I don't have an opinion here. On the one hand I see the point about async. On the other, if the collection was closed, reusing its previous decks makes no sense. Plus, doing change to a deck that can't be flushed (since it's collection its closed), or worse, which would flush over the current deck of the collection, seems a bad idea. On the third hand, it seems credible that in a lot of use case, either there is no write, or anyway, we really don't expect the collection to be closed/changed during the activity (i.e. anything that is not the deck picker should not lead to closing a collection)

val note = withCol { card.note() }

It seems David and myself understood in different way.
My understanding is that you should actually do val note = withCol { card.note(this) }, and rewrite note so that it takes the collection as argument. David understood that this means that note() should uses withCol itself in order to get the queue when it needs it. (Which would require note() to become a suspend function)

While I can see why we would want the access to col to be near its actual use (in this case, in note()), the trouble is that it would lead to two withCol in the same stack trace. And since withCol is the queue, the innermost withCol would wait for the outermost one to ends; and the outermost one would wait for the innermost one to return, which would create a deadlock. (Generally speaking, it'll be hard to guarantee we never have two withCol in the same stack by the way)

In any case, we should decide whether we want most functions that requires the col to become suspend function, or whether we want them to take col as a parameter. Or whether we want a third option. For example having withColRead and withColWrite, and authorizing any number of read or a single right, but not both. (i.e. same than with reference and mutable references in Rust).

I'll let David correct me, I'm sure I was not entirely faithful to his original opinion. But at least, I'm trying to ensure we have a non-discord trace and maybe could get Damien's opinion.

[dae]

My understanding is that you should actually do val note = withCol { card.note(this) }

Sorry for the confusion; your interpretation is what I meant.

Note that CollectionManager is in the GUI anki module, not in libanki. This means all the code in libanki can remain as normal functions and remains easy to reason about (1). On the GUI side, you call withCol to await a handle to the collection, and then you can perform whatever synchronous operations you want on it before returning control to any other callers.

I would caution against trying to omit a call to withCol if you think something is cached - it may not be, or may be changed in the future to be fetched lazily, and the person changing it will have no easy way of knowing that other parts of the codebase are assuming that the item is cached. And in fact in DecksV16, that's already the case - decks.count() calls into the backend for its result.

Which begs the question, do we consider that we can still references to decks and models, or are we always forced to take them through the collection.

I think storing a reference to decks/models is going to cause the same sort of problems as things like card.note() - it makes it too easy to accidentally access the collection in an unprotected manner without realising you're doing so. I'd recommend that code using withCol never returns a handle to the collection, either directly, or indirectly via something like Decks/Models.

One thing that may be worth mentioning on that page: the programmer should attempt to move as much collection access into a single withCol as possible, ideally reading+transforming+writing in a single block. This ensures no interleaving can occur, and minimizes the overhead of the asynchronous call.

(1) I incorrectly placed ChangeManager in libanki; it and the suspend fun in SoundKt should eventually be moved over to the anki module.

[dae]

Another thing that should probably be documented is the potential to introduce unit test flakes. When a routine is converted from directly accessing the collection to using launchCatchingTask, unit tests that expect the routine to have completed by the time it returns may start to flake when the launched task fails to complete before the unit test proceeds. This is usually pretty easy to fix - by wrapping the failing tests in runTest(), the launched task will be executed eagerly. eg:

diff --git a/AnkiDroid/src/test/java/com/ichi2/anki/CardBrowserTest.kt b/AnkiDroid/src/test/java/com/ichi2/anki/CardBrowserTest.kt
index 4869d7e8d..3e8a5d59c 100644
--- a/AnkiDroid/src/test/java/com/ichi2/anki/CardBrowserTest.kt
+++ b/AnkiDroid/src/test/java/com/ichi2/anki/CardBrowserTest.kt
@@ -125,7 +125,7 @@ class CardBrowserTest : RobolectricTest() {
     }

     @Test
-    fun browserDoesNotFailWhenSelectingANonExistingCard() {
+    fun browserDoesNotFailWhenSelectingANonExistingCard() = runTest {
         // #5900
         val browser = getBrowserWithNotes(6)
         // Sometimes an async operation deletes a card, we clear the data and rerender it to simulate this

[Arthur-Milchior]

I edited the original post so that it reflects that this issue is for discussion between reviewers and keeping track of the decision, and created #12495 for contributors to discuss migration itself (in the hope that new contributors can help without the need to understand the whole discussion and disagreement details)

[oakkitten]

My two uneducated cents.

In many cases withCol will work fine, but in others you want to get the data in sync. You may have a FragmentManager trying to recreate a dialog now, or you can have a RecyclerView that is going to be visible on the next frame and its adapter needs data. If you can't get data in sync, you have to work around these issues, maybe trying to delay the display of the dialog, or showing a loading animation on top of the RecyclerView, and this can get complex fast, and error-prone.

It might be better, at least for some collection queries, to use as much immutable objects as possible, and allow retrieving them in sync. As I understand, the collection is rarely not open, so it cases when it's not open, blocking main sounds fine. I suppose fully async approach might be fine when you build everything with it in mind. Given the current state of AnkiDroid, I'm not so sure.

P.S. Why not collection.getNote(card)? Both card.note(collection) and card.note() are weird ways to spell it.

[Arthur-Milchior]

And I just:

• removed the part about caching (since it seems it's a bad idea)
• added a note about runTest

Thanks a lot @dae for the review and correction.

[dae]

Thanks for making those edits @Arthur-Milchior . Regarding #12495, it might be worth holding off on requesting such changes at the moment. While I think it does make sense to encourage usage of withCol in new code/when the code has to be updated anyway (eg @divyansh-dxn's migration of tasks to coroutines), as @BrayanDSO mentioned on #12496 (comment), the future switchover to the new schema is going to render a fair bit of the current legacy code obsolete, and it would be a shame to spend time on code that is not long for this world.

@oakkitten I agree that some APIs that assume a synchronous reply can make things a bit harder, but I think you're overstating things a bit. For a RecyclerView for example, if the behaviour is anything like its iOS counterpart, you could presumably return a blank row synchronously, and then populate it with data asynchronously. onCreateOptionsMenu() required a bit more work to avoid flicker, but I'd argue the new approach is actually easier to read and reason about, and less error-prone with the state fetching and applying of that state to the GUI being separated.

As I understand, the collection is rarely not open, so it cases when it's not open, blocking main sounds fine.

You never know when another task may already be running, or some disk I/O will stall due to some background process or slow sd card. In the description of #11849 I included the following snippet:

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

That was a real 2.6s hang on displaying the decks screen that I saw when running on an emulator on a quite-fast dev machine. The problem with a "sometimes synchronous" approach is that if any of those synchronous calls blocks, you've hung the UI.

P.S. Why not collection.getNote(card)? Both card.note(collection) and card.note() are weird ways to spell it.

col.getNote(noteId) already exists. col.getNoteOfCard(card) would be a reasonable alternative, or the method could just be dropped in favour of col.getNote(card.nid).

[oakkitten]

For a RecyclerView for example, if the behaviour is anything like its iOS counterpart, you could presumably return a blank row synchronously, and then populate it with data asynchronously.

You could do that, and RecyclerView will animate the change for you if you do it right, but:

• It works best if the rows are the same size before and after fetching data. Otherwise, you will see resize animation
• In the case when the RecyclerView doesn't span the height of its container, you'll will see its size suddenly change with new data, and you will have to animate that too
• Since in most cases you get the data nearly right away, you'll see the transition start at once. This kind of animation might just not look good
• Re-populating the RecyclerView is just more work. RecyclerView is hard to get wrong, but it's also not exactly easy to get right!

As I understand, the collection is rarely not open, so it cases when it's not open, blocking main sounds fine.

You never know when another task may already be running, or some disk I/O will stall due to some background process or slow sd card.

I think the trick here might be to have immutable data. This way, the only thing you ever have to wait for is the collection being open. For instance, if you want to display the list of decks, you may have something like this (not real code):

private val collection: Collection? // E.g. null if not open

private ensureCollectionUnsafe(): Collection {
    return collection ?: openCollection().also { collection = it }
}

fun getDeckNames(): List<String> {
    return ensureCollectionUnsafe().decks.names()
}

If decks is an immutable object, getDeckNames() is safe to run at any time, even though it obtains the collection in an unsafe way. If the collection is open, this method will return instantly. If not, it will block the UI while the collection is getting open. If you create a bunch of methods such as getDeckNames(), getAllTags(), getSyncStatus(), etc, that never reveal the collection object, I think this just might work well?

Another option is perhaps using an event bus/reactive approach. The backend might post sticky events with data such as sync status, deck lists, tags, etc, and the UI can subscribe to those events. You will still have to handle the possibility of a missing event, but this should happen super rarely and you could just forgo animation in these cases.

[dae]

This way, the only thing you ever have to wait for is the collection being open.

But that's not true - any subsequent operation performed on the collection may also take time to complete, as the data may need to be read from storage, or another operation may already be running. When you talk about immutability, I'm guessing you're talking about caching the objects in memory? It's not feasible to store the entire collection in memory, and even if it were, there would be an initial stall while that data is read.

Your argument seems to be that synchronous access from the UI thread is easier, and I don't disagree with that - but doing so will cause the UI to hang or stutter when an I/O operation doesn't return quickly.

[oakkitten]

Oh, I was under impression that actually getting the list of decks doesn't perform any queries, I see that this is only true for the legacy backend. In that case, yes, I guess what I'm talking about is caching certain information about the collection.

I think caching even a little may go a long way. For instance, for the main activity you could cache (or post a sticky event with) the list of decks names only. You would need to rebuild such a list very rarely, but it would allow instantly displaying decks, and new/due numbers can be fetched asynchronously and animated in easily. And this could be used for the deck picker dialog.

Of course, cache invalidation is famously said to be one of the two hard things in computer science, and it is, but just maybe, if done carefully, it can save us a lot of trouble with the UI.

Off the top of my head, some of the things that I would want cached:

• Deck names
• Tags
• Model names
• Custom flag names
• Custom searches

[mikehardy]

Every item in that list is potentially unbounded

[oakkitten]

You've got to have the full list of decks to easily display it, so if it takes too much RAM it will cause a problem either way. Maybe caching loses if you've got millions of decks and millions of tags at the same time—but if the user decides that they need all of that, that's really on them 😛

Anyway, if you only cache names, for 1000 decks names of 100 characters each that's what, 100KB + whatever the overhead of a String. Not sure if this will cause issues IRL...

[dae]

I don't think caching is really relevant when discussing the usefulness of asynchronous I/O, since you can't cache everything, and since I/O can stall before the data is cached. Also bear in mind that SQLite has a page cache, so reading things like deck names (and all the other info such as collapsed state and cards done that day that is actually required to render the deck tree) often does not involve actually requesting the data from disk.

[oakkitten]

I was saying that perhaps caching just a few select things would be very helpful, but if SQLite page cache means that I can get the list of decks fast and in sync, that's great! 😃

[github-actions]

Hello 👋, this issue has been opened for more than 2 months with no activity on it. If the issue is still here, please keep in mind that we need community support and help to fix it! Just comment something like still searching for solutions and if you found one, please open a pull request! You have 7 days until this gets closed automatically