Degrade IndexedDBStore back to memory only on failure

[jryans]

IndexedDB may fail at any moment with QuoteExceededError when space is low or
other random issues we can't control. Since IndexedDBStore is just a cache for
improving performance, we can give up on it if it fails.

This causes IndexedDBStore to degrade in place back to using memory only. This
allow (for example) login to complete even if IndexedDB is exploding.

Hopefully improves element-hq/element-web#7769

[bwindels]

The idea here is that the next calls to the store still go to func, e.g. the idb store method and encounter a closed db, fail, and end up in the catch below degrading to the memory store?

I suppose that would work, but seems a bit convoluted.

Just an idea, but have you considered using a Proxy (seems well supported) as a meta store to switch between two implementations of the store if one fails?

[jryans]

The first time any of the IDB methods marked degradable fails, we'll enter the catch block that one time and revert back to being just a MemoryStore.

Not sure if this is clear from the review, but IndexedDBStore already extends from MemoryStore, so it's not like we're totally transforming into some other unrelated object. We're basically "peeling off" the child layer of IndexedDBStore and becoming the parent MemoryStore only.

Future method calls to the IDB store if it degrades will then only run code from MemoryStore. The state of the object is already set up for this to work since it extends from MemoryStore.

I've gotten burned by Proxy adding perf overhead in the past, so it didn't come to mind until now... It could work, but I am not sure it would really that much more elegant?

const MetaStore = new Proxy(idbStoreInstance, {
    get(target, prop, receiver) {
         // If `prop` is on the list of methods to degrade, 
         // wrap that in another proxy...
         if (prop != "storeClientOptions" && etc.) {
             return target[prop];
         }
         return new Proxy(target[prop], {
              apply(target, thisArg, argList) {
                   try {
                       // Call the normal path
                       return target.call(thisArg, ...argList);
                   } catch (e) {
                       // Instead maybe delete and fallback
                       return MemoryStore.prototype[prop].call(...)
                   }
              },
         });
    },
});

Probably we'd need to sprinkle some async / await into the above as well... The "proxies on proxies" usually makes me avoid it.

Are there parts of the current approach I could improve through comments, etc.?

[bwindels]

Oh right, I see now. Sorry for being slow on this.

One more thing then: the only method in IndexedDBStore that seems to forward the call to MemoryStore seems to be deleteAllData, so the memory store state would be empty for the part that the idb store overrides? The first example I can think of would be getSavedSyncToken, but feels like there would be more code that calls the store and assume once the app is syncing, the store is not empty? Maybe I'm missing something and the memory store is being updated from somewhere else...

W.r.t. commenting, maybe just briefly mentioning above Object.setPrototypeOf(this, MemoryStore.prototype); that we're changing the class of this instance to the parent class so future calls get permanently redirected would have helped understand the code faster.

[bwindels]

Right, after looking at the store code again, I still can't spot where we're updating the memory store from the indexeddbstore mutators. But it doesn't seem like it matters as all the read methods overriden (like you mention) in indexeddbstore only seem to be called during startup, initial sync, ... so they shouldn't be called again while the app is running.

So I guess this is fine. Maybe adjust your comment where it says that the mutators update the memory state if you agree with the above assessment, as it could be a bit misleading...

[jryans]

I added a comment to explain why it should be safe to fallback to the parent store type.

IndexedDBStore actually leaves many of the methods from MemoryStore as-is without overriding them at all. For basic sync data like rooms and users, the memory state is used to answer things like getRoom even in the IDB store. When saving, the IDB calls syncToDatabase on the backend which uses the sync accumulator to save and totally ignores the data in the store itself.

You asked about getSavedSyncToken. This one is only used on startup when loading a sync from IDB. Since we are degrading to a memory store, this would return null after degrading, which seems fine. Let's say we managed to read an old cached sync from IDB, but then it degrades at some later time. SyncApi.prototype._syncFromCache is given the saved sync data, which calls _processSyncResponse which write to the in-memory part of the store via storeUser etc. The in-memory side of IndexedDBStore is always being updated even when IDB fully operational. If IDB fails later, we'll just continue with this state we have, so the store won't be empty.

However, the extra methods for things like OOB members, etc. don't currently maintain memory state. Let's examine all the mutator methods:

Mutator Methods	IDB maintains memory state?
setSyncToken	Yes (not overridden in IDB)
storeGroup	Yes (not overridden in IDB)
storeRoom	Yes (not overridden in IDB)
removeRoom	Yes (not overridden in IDB)
storeUser	Yes (not overridden in IDB)
storeEvents	Yes (not overridden in IDB)
storeFilter	Yes (not overridden in IDB)
setFilterIdByName	Yes (not overridden in IDB)
storeAccountDataEvents	Yes (not overridden in IDB)
setSyncData	Yes (no memory state for this)
setOutOfBandMembers	No, needs to call memory also
clearOutOfBandMembers	No, needs to call memory also
storeClientOptions	No, needs to call memory also

So, it's good that we examined each method here, as it reveals a few to fix! (clearOutOfBandMembers was I guess never implemented for MemoryStore, so that's an extra bug I guess).

I have now fixed these new bugs.

[bwindels]

Great, thanks for your detailed look! I must indeed have forgotten to implement clearOutOfBandMembers for MemoryStore when working on LL.

Looks good now!

[jryans]

I think you left for the day... I'll re-request review, as I'd like to double-check that my last comment and change make sense to you.

[bwindels]

lgtm!