Entities can be loaded half-way while being updated

[seadowg]

Blocked by #6012

It's currently possible to load entities in a follow-up form while they're being updated (by match exactly or previously download only form updates) which can result in various problems like partial entity lists.

To fix this, automatic form updates (exactly match server or previously downloaded only) should not be able to run during form entry. In the case this does happen, the form update should try to run again with exponential back off rather than wait for the next scheduled run (like we do with auto send failures).

If form entry is started during an update, the form should not load until form entry has finished. Ideally we'd flag this in the progress dialog shown while loading a form, but it might be easier to do this as a follow-up down the road.

Notes

It's likely that this can be implemented using the existing forms ChangeLock. The form updates already respect this so we just have to make sure we correctly lock/unlock this during form entry (as well as wait for the lock when loading a form).

[seadowg]

@getodk/testers are you able to investigate this and see if you can create problems by updating forms (with changes to entities) during form entry on master?

[seadowg]

@getodk/testers any update of this?

[dbemke]

If an entity update/follow-up form is opened the entities in the opened form don't change after match exactly (adding some entities). The thing I'm going to explore are drafts after match exactly cause the changes in entities appear there. I open a follow-up form wait for match exactly, save the form as draft. When I open the draft there are new entities/ changed entities... so "blank" was saved without changes but after opening the draft changes appear.

[dbemke]

Savepoints in update forms+ updating entities result in a blank list with only the entity being updated

Steps to reproduce the problem

1. Download a project with Trees update form to Collect.
2. Open Trees update form and select a tree to update and start filling the form.
3. While in the form go to project settings and change the theme of the app (to create a savepoint).
4. Go to Central to Trees registration form and submit a new tree.
5. Wait ~15min for match exactly or manually download the update to Trees update form (in Collect).
6. Go to Trees update form and recover the savepoint

Should I file a separate issue or it's connected?
(the master version 1bd1af1)

[dbemke]

Deleting the entity while it’s being updated in a device with match exactly

Steps to reproduce the problem

1. In Central create a project with children registration and child update forms.
   child_update.xlsx
   children_registration(1).xlsx

2. Download the project to Collect.

3. Open the child update form, select a child to update and fill questions (but don’t close the form).

4. In Central delete the child.

5. In Collect wait ~15min for match exactly.

6. While the child update form is opened swipe forwards and backwards and check if the values/properties of the child are present.
   

[seadowg]

After doing some research into SQLite, I can add another problem to the list here. I'll post my full notes below, but the TLDR is that we can't write to the entities DB concurrently meaning that an offline create/update would block if it tried to run at the same time as we were updating entities from the server (because EntitiesRepository#save is one transaction currently). The good news is that with a few small modifications, we can allow concurrent reads during a transaction (by enabling write ahead logging).

Full notes here:

• According to this SO calls to a shared SqliteDatabase object (via a shared SqliteOpenHelper) are serialised: "The SqliteDatabase object uses java locks to keep access serialized. So, if 100 threads have one db instance, calls to the actual on-disk database are serialized.".
• The official docs suggest that writing and reading in parallel is not possible in the section on write ahead logging: https://developer.android.com/reference/android/database/sqlite/SQLiteDatabase.html#enableWriteAheadLogging%28%29.
• Running an experiment with a transaction that uses a Thread.sleep shows us that reading (using the same SqliteOpenHelper) from another thread is blocked until the transaction is finished.
• Calling setWriteAheadLoggingEnabled with true on the SqliteOpenHelper before running the experiment allows the DB to be read while the transaction is "running". Modifying the experiment to try writing multiple transactions demonstrates that enabling WAL does not allow multiple writes however: the second one will block until the first has finished.
• Switching the first transaction to non exclusive does not seem to help the above problem. This makes sense after reading SQLite's own docs on WAL: "Because writers do nothing that would interfere with the actions of readers, writers and readers can run at the same time. However, since there is only one WAL file, there can only be one writer at a time."
• In cases where a long DB transaction in the background (running in WorkManager for example) could block an action in the foreground, it seems like it makes sense to use a lock of some kind to prevent the background work from running.

[seadowg]

We're going to go with blocking form updates during form entry (with back off). I'll update the issue description.

[seadowg]

@lognaturel @grzesiek2010 let me know if the updated issue description makes sense! If you agree, I think this is ready to go.

[seadowg]

I did a quick spike on this just to see how feasible it was. I ended up adding lock/unlock methods to ChangeLock and then using them in FormEntryActivity and `FormEntryViewModel like so:

FormFillingActivity

@Override
public void onCreate(Bundle savedInstanceState) {
    Collect.getInstance().getComponent().inject(this);

    if (savedInstanceState == null) {
        sessionId = formSessionRepository.create();
        changeLockProvider.create(projectsDataService.getCurrentProject().getUuid()).getFormsLock().lock();
    } else {
        sessionId = savedInstanceState.getString(KEY_SESSION_ID);
    }

FormEntryViewModel

public void exit() {
    formSessionRepository.clear(sessionId);
    ReferenceManager.instance().reset();

    AppDependencyComponent component = Collect.getInstance().getComponent();
    component.changeLockProvider().create(component.currentProjectProvider().getCurrentProject().getUuid()).getFormsLock().unlock();
}

These examples are obviously not particularly "clean", but it did appear to work and didn't cause any tests to fail - as far as I could tell the "try again when locked" part of the behaviour is already implemented for the background tasks (but this is worth double-checking of course). I think taking a direction like this would work, but it would definitely be better to have the lock/unlock calls somewhere better. It might be worth reworking FormSessionRepository to be a ...DataService and do it there. Alternatively, it could happen in FormEntryViewModel and we could also move the session creation in there (it feels natural for these to be together). I think the latter could potentially be better as we'll likely move to a single Activity for form entry in the future (with the hierarchy just being another Fragment) and that would make a data service unnecessary.

I didn't dive into blocking loading if an update is happening. Form loading is currently very tangled up in FormLoaderTask and I didn't have time to look at how we might incorporate acquiring a lock. I'd imagine in the abstract we'll be able to do something along the lines of trying to acquire a lock and then spinning until we can (while allowing cancellation).

I also gave some thought to testing: it feels like checking that an update can't start during form entry will be easy enough from an integration level (we already test background tasks from there), but I think the other way around is pretty tricky (and might involve some refactoring to make unit testing around loading more feasible).