Allow the app to work without storage in extreme cases where it fails completely

[marcaaron]

Problem

There are documented cases where IndexedDB appears to completely fail to spin up space for the app to use. The app won't work at all when this happens. Most recently this happened to @davidcardoza here (sorry, internal).

The error we saw in the JS console was :

Internal error opening backing store for indexedDB.open

We then get stuck in a spot where since the IndexedDB store doesn't work basically nothing works.

Solution

While we aim to have the app work offline and provide a good return experience with some data and request persistence. If the app can't save anything to storage at all it should still work. If we run into a serious error like this - we should gracefully degrade the experience.

Bonus quest

Actually, figure out why exactly all the IndexedDB errors happen...

[marcaaron]

@chrispader should I assign you to this issue? I think it should be fairly straightforward of a change.

[chrispader]

Yes, you can assign me! 👍

[marcaaron]

🤙

[melvin-bot]

Eep! 4 days overdue now. Issues have feelings too...

[chrispader]

Just finished the E2E encryption library, so i have more time to work on this now! Looking at this this week 👍

[melvin-bot]

@chrispader Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[marcaaron]

@chrispader any thoughts?

[melvin-bot]

@chrispader Still overdue 6 days?! Let's take care of this!

[melvin-bot]

@chrispader Now this issue is 8 days overdue. Are you sure this should be a Daily? Feel free to change it!

[chrispader]

Sorry for the delay again 🥴 I can look into this over the weekend, though if this has high priority, you might want assign it to somebody else @marcaaron

Still pretty busy with other tasks unfortunately

[melvin-bot]

@chrispader Whoops! This issue is 2 days overdue. Let's get this updated quick!

[melvin-bot]

@chrispader Eep! 4 days overdue now. Issues have feelings too...

[chrispader]

Going to look into this tmrw! For real!

[melvin-bot]

@chrispader Whoops! This issue is 2 days overdue. Let's get this updated quick!

[chrispader]

Most recently this happened to @davidcardoza here (sorry, internal).

@marcaaron is there any other information about this specific incident or any other reproduction steps?

No worries if not, i should still be able to adapt the app for this issue without actually getting any errors from IndexedDB

[chrispader]

I have a couple of thoughts and proposals for this problem after my initial investigation:

The error that @davidcardoza experienced is presumably thrown by createStore function call in the web storage provider implementation IDBKeyVal. On native, this could also happen similarly in the the SQLiteStorage provider.

These errors will essentially be thrown at bundle start time (when Onyx get's imported the first time), since we're already trying to establish the storage instance by then.

Questions

To which degree should the app still work reliably? There are certainly a lot of screens/components/flows that rely on the data that's set with Onyx.set or Onyx.merge to be written to the storage or at least the cache.

Is the idea behind this issue to essentially just omit storage persistence in general and only rely on cache for these extreme cases? If so, we there are some approaches we could go for:

Possible solutions

Optimistically creating storage instances

We can try to create/connect to the storage instance - idb-keyval store on web and SQLiteStorage DB on native - at bundle start time, but not throw an error. When functions like Onyx.set/Onyx.merge/Onyx.get later try to access data from storage and the storage instance couldn't be created, we'll simply return the value from cache.

All of this could be done inside react-native-onyx without the need to adapt anything in the app really.

Initial Onyx initialisation

In order to safely provide a backup solution for when the storage initialisation is not working, we could add a "initialisation phase" in Onyx, which will create the local storage instance and if failing, this will give us the chance to omit the storage and only rely on cache or potentially disable certain features partially or completely...

In this context, i was thinking of adding something a function like initOnyx (there's already Onyx.init though).

Error handling directly in the app

I think this might be the least favourable and most difficult approach, since we use Onyx in lots of places and functions like Onyx.set/Onyx.merge will already handle the errors from the storage layer, which could lead to Onyx trying to evict from storage or further throwing and logging errors when getting data

Curious about your thoughts @marcaaron... I think Option 1 should be pretty straight forward!

[melvin-bot]

@chrispader Still overdue 6 days?! Let's take care of this!

[chrispader]

cc @marcaaron @tgolen

[chrispader]

I like the overall thought, but can you explain "create a temporary new provider, that just does nothing at all." more? I'm not sure I understand what this would be or why it's necessary.

With this design, instead of adding a second MemoryOnlyProvider as fallback which actually stores/retrieves data (and therefore will double the memory footprint) i can just create a storage provider with the same interface, which will just be () => Promise.resolve() for every operation.

This way, the Onyx layer will still succeed because we act as if everything works fine, but in reality, it's only relying on cache...

I would need to check though, if this is currently doable with all operations in Onyx.js

[chrispader]

I like the overall thought, but can you explain "create a temporary new provider, that just does nothing at all." more? I'm not sure I understand what this would be or why it's necessary.

With this design, instead of adding a second MemoryOnlyProvider as fallback which actually stores/retrieves data (and therefore will double the memory footprint) i can just create a storage provider with the same interface, which will just be () => Promise.resolve() for every operation.

This way, the Onyx layer will still succeed because we act as if everything works fine, but in reality, it's only relying on cache...

I would need to check though, if this is currently doable with all operations in Onyx.js

i added the NoopProvider here just for showing what i mean: https://github.com/margelo/react-native-onyx/blob/%40chrispader/memory-only-provider/lib/storage/providers/NoopProvider.js

[marcaaron]

The ultimate idea - and the reason for this new design - is to completely replace OnyxCache with this new MemoryOnlyProvider as the first/top-priority storage.

While having the cache replaced by a "provider" could be interesting, it doesn't seem to significantly improve the current situation. Too much complexity could also make the code less accessible. So, I'd encourage us to just focus on addressing the issue without assuming that we may need more than that.

That said, if the rest of the team is on board, I don't want to create a roadblock here. Just wanted to share my reservations as I'm still having trouble fully supporting the extra changes.

Here's what I'd do:

• Create a provider that does nothing - basically like a mock provider, but without an internal cache
• Leave the cache as it is
• When storage fails in a way where it can't be recovered switch the storage provider to the provider that noops
• Don't chain providers together in an array. Just switch them out when a unrecoverable error happens.

[melvin-bot]

@chrispader Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[melvin-bot]

@chrispader Still overdue 6 days?! Let's take care of this!

[melvin-bot]

@chrispader Now this issue is 8 days overdue. Are you sure this should be a Daily? Feel free to change it!

[chrispader]

Updated the PR to only contain logic to fallback to the NoopProvider and left an explanation here: Expensify/react-native-onyx#439 (comment)

[marcaaron]

Left a few comments. Seeing as this issue has been unresolved for a while - I would love to maybe revisit a high level analysis of the "problem" (as experienced by users) that we are trying to solve and then a proposal on how it will be fixed (or what other changes to expect) so we can maybe all agree on what we are doing here before spending time on additional reviews?

[marcaaron]

e.g. unpacking this statement for a second:

The error that @davidcardoza experienced is presumably thrown by createStore function call in the web storage provider implementation IDBKeyVal

Are we sure that's where the error gets thrown?

[chrispader]

Are we sure that's where the error gets thrown?

No we aren't 100%, you're right. I didn't find lots of substantial proof/reason for this error. Anyway, i updated the PR to also handle errors during runtime (after initialization)

[melvin-bot]

@chrispader Huh... This is 4 days overdue. Who can take care of this?

[melvin-bot]

@chrispader 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

[melvin-bot]

@chrispader 8 days overdue is a lot. Should this be a Weekly issue? If so, feel free to change it!

[melvin-bot]

@chrispader 12 days overdue now... This issue's end is nigh!

[melvin-bot]

This issue has not been updated in over 14 days. @chrispader eroding to Weekly issue.

[melvin-bot]

This issue has not been updated in over 15 days. @chrispader eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[chrispader]

@tgolen @marcaaron i think we can close this out since the split-up PRs by @kirillzyusko have been merged?

[tgolen]

I think you're right! I'll close it out.