Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Offline Mode Improvements #2755

Closed
nVitius opened this issue Mar 17, 2020 · 9 comments · Fixed by #3575
Closed

Offline Mode Improvements #2755

nVitius opened this issue Mar 17, 2020 · 9 comments · Fixed by #3575
Labels
api: firestore

Comments

@nVitius
Copy link

nVitius commented Mar 17, 2020

I wanted to propose a few improvements that might be made to the SDK's Firestore Offline Persistence functionality.

To provide a little context, I work for a small organization using Firebase across 5 separate web applications (also wrapped as native applications through WKWebView and Android WebView) servicing about 2000 users. One of the main driving factors behind our decision to implement Firebase was the ease of implementing offline data access; it is very important to us.

Currently, there is an issue across most devices/browsers where Firestore can lose connection unexpectedly to IndexedDB and cannot recover without a restart ( #2711 #2710 #1533 #1926 #2581 ).

A comment from @schmidt-sebastian on the latest issue:

You should see this a lot less with SDK versions 7.2.1 and later. It does however seem that we were not able to completely solve this. If you do encounter this issue with the latest Firestore SDK, you will unfortunately have to reload your PWA/webpage or create a new Firestore instance.

We are exploring ways to re-architect our SDK to be more robust agains IndexedDb failures in general, but this will take us a bit of time.

I understand that this issue is being worked on and it is rather difficult to address as it might require re-writing a large part of the SDK. That said, for a product that is no longer in beta, it is unacceptable ti have issues that cause data loss and are unrecoverable (even if it happens "a lot less"). For our applications, we use a session recording software that allows us to recover data for a user when this happens based on their logs. But for a lot of people that are not aware this is happening (there is no warnings on the documentation that offline persistence is broken) this means permanent data loss.

Of course, there is always the option to disable offline persistence. But I do not think that is an option for everyone. For us at least, it is one of the major selling points of our apps.

While a fix is being worked on, these are some improvements I think could be made to the system that might not require a lot of work and would make everyone's lives easier.

  1. Error Handling
    Currently, the only way to handle this error is to catch it at the Window level and then reload the page. The issue with this is that there is no way for me to know which of my .set() calls failed. When/if I reload the app, that data is still lost. Ideally, the error would (also?) be thrown to the original caller so that the application could easily identify which specific queries have failed. This would allow one to store the data manually and retry saving it when the app is reloaded.

  2. Trigger Failure Manually
    Given that this issue is very difficult to reproduce consistently in a real-world scenario, I think it is important to add tools to the SDK so that developers can test their solutions inside of their actual apps instead of hoping that it will work when the failure happens in produdtion.

  3. Notify Client When Going Offline
    We use onSnapshot calls to keep our clients up to date with data that might be inserted from another device. Obviously, these do not work when the user is offline. I would like to be able to notify my users that they are not receiving updates from other sources when they are offline. Right now, there is no way for me to know if Firebase is operating in offline mode, so I cannot notify my users of this.

  4. Enable/Disable Offline Persistence
    Currently, offline persistence is disabled by default on the JS SDK. The only way to enable it is right after initializing the application. Perhaps that makes sense, but I would also like the ability to disable it at any point. A possible use case could be to allow a user to disable offline mode if they don't want to accidentally enter data that might be lost. Alternatively, they might leave it on if all they want is to access cached data for viewing only.

The last two points might not be directly related to the IndexedDB issue, but I still believe they are valid concerns/suggestions regarding the existing functionality for offline persistence.
@google-oss-bot
Copy link
Contributor

I found a few problems with this issue:

  • I couldn't figure out how to label this issue, so I've labeled it for a human to triage. Hang tight.
  • This issue does not seem to follow the issue template. Make sure you provide all the required information.

@wu-hui
Copy link
Contributor

wu-hui commented Mar 19, 2020

Hi nVitius,

Thanks a lot for your suggestions!

  1. Better error handling. This is something we already discussed internally, and is on our to-do list. Unfortunately we cannot tell when this will be worked on yet.

  2. Trigger failure manually. This is interesting, once we have better error handling. However, this is more like a testing facilitation, and I am not sure this should be SDK's role to provide it. You might be able to do it with a mock object, to test you App can handle errors properly.

  3. Offline notification. This is one of the requested features already. We are aware of the need. But again, I cannot tell when we can work on this yet.

  4. Config offline persistence on the fly. This might be very hard to achieve, given how our SDK is structured now, it requires changing a significant portion of the SDK. We can revisit if more people are requesting for this, but as far as I am aware, this is the first request for this.

Overall, I agree with you that the SDK has a wrong assumption of how reliable IndexedDB is across all browsers, and there is definitely some improvements to be made!

@wu-hui
Copy link
Contributor

wu-hui commented Mar 19, 2020

Closing it because this is not an actual issue, we can continue to discuss regardless.

@wu-hui wu-hui closed this as completed Mar 19, 2020
@nVitius
Copy link
Author

nVitius commented Mar 19, 2020

Thanks for the reply, @wu-hui.

I don't think there has been any doubt about the issues regarding the SDK's assumptions around IDB transactions for at least a couple of months now.

I understand that you might not have the freedom to share the team's roadmap and provide us with an estimate on when this might be accomplished. But I really believe that the Firebase team should consider being more open with their "internal discussions" on an open source project.

There may not be any contractual obligations from Firebase to fix any issues, but we are paying customers. I don't use Firebase for fun; it is part of my job. In turn, I have paying customers being affected by the reliability of our applications.

Like I said in my first post, issues that cause permanent data-loss from a data store product are unacceptable, especially from one no longer in beta. These are not just "improvements". Currently, there are is no way for a developer to completely mitigate this flaw aside from completely disabling, what I would consider, a core feature of the SDK.

It is a little frustrating to hear "I cannot tell when this will be worked on" even when trying to identify ways we might allow for developers to handle these errors. Not to mention the fact that we don't have an estimate on a fix for the original issue or any visibility on if the work is being done. As far as I can tell, there hasn't even been an acknowledgment from the team regarding the severity of the issue.

@nVitius
Copy link
Author

nVitius commented Apr 2, 2020

@wu-hui I wanted to follow up on this again.

I already mentioned this, but there is currently no way, save for turning off the feature entirely (not an option for everyone), for a developer to completely mitigate data loss from this failure. It's a big problem for our organization, and I'm sure it is a problem for more users. Unfortunately, closed issues don't get much traffic.

If you cannot share the roadmap for this fix, then please at the very least confirm that it is being worked on. My concern here is that this problem is being seen as an "improvement" rather than a critical flaw by the Firebase team.

In the case that the fix will take a long time to complete, we need to have tools provided by the SDK to recover from these errors. Reloading the page to re-establish the connection is an unacceptable solution and can easily cause data loss.

Pinging other members of the firebase team to increase visibility on this issue:
@schmidt-sebastian @hsubox76 @mikelehen @Feiyang1 @hiranya911 @bojeil-google @depoll

@schmidt-sebastian
Copy link
Contributor

I am working on this feature as we speak. It is among my top priorities, but as all of our lives have been been disrupted this might take me longer than it normally would.

@nVitius
Copy link
Author

nVitius commented Apr 2, 2020

@schmidt-sebastian Thank you so much for updating us on that. That's really all I wanted to know; that it was actively being worked on.

@schmidt-sebastian
Copy link
Contributor

First of many PRs just landed: #2879
This retries Multi-Tab operations that fail to access IndexedDB.

@schmidt-sebastian schmidt-sebastian mentioned this issue Apr 18, 2020
Merged
@firebase firebase locked and limited conversation to collaborators Apr 19, 2020
@schmidt-sebastian
Copy link
Contributor

I am re-opening the issue to allow tracking of progress for our IndexedDB recovery, which addresses one of the concerns raised in the Issue disruption. I will close this issue again when we have code in place that handles all IndexedDB failure scenarios. So far, the PRs that have been opened are:

The only user-visible change from these PRs is that set(), update(), delete() and add() operations return a rejected Promise now if IndexedDB is unavailable. Similar, snapshot listeners and get() operations can now fail if we are not able to write to IndexedDB during their initialization. Keep in mind that these are all cases where we previously crashed the client. If you see an error like this in your code, it is likely that you are performing operations in the background that should be deferred until the tab is once again in the foreground.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
api: firestore
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Make shutdown idempotent firebase/firebase-js-sdk
5 participants
@nVitius @Feiyang1 @schmidt-sebastian @google-oss-bot @wu-hui