FR: Add number_of_retries option to set() so that it fails when offline.

[mohshraim]

[REQUIRED] Describe your environment

• Operating System version: web chrome
• Firebase SDK version: 4.9.1
• Firebase Product: firestore

[REQUIRED] Describe the problem

Using firestore on web Javascript, i need to work only if user connected to internet so i didn't enable
persistence. --> firebase.firestore().enablePersistence() ...
if i disconnect the network then add a new item to my DB collection the console show many connection error from firestore but wait until i reconnect and then confirm the success of add operation...

expected behaviour; as i didnt enable the persistence feature the (set or update ) should fire the catch and notify me for connection..

maybe it can be as feature request to add options to wait for reconnect or through error on offline, adding this will provide more control to application if we want to enablePersistence..

var db = firebase.firestore();

function addOneItem() {
    var item = {
      description: "more details about item",
      name: "orange juice web item",
      sellPrice: 10
    };

    db.collection('item').doc('1-1').set(item)
    .then(function() {
        console.log("Document successfully written!");
    })
    .catch(function(error) {
        console.error("Error writing document: ", error);
    });
}

thanks

[mikelehen]

Thanks for the report. This is currently expected behavior. It is common to experience temporary network glitches (even while your app is "online") and so Firestore will transparently retry the write operation until it completes.

We do this regardless of whether persistence is enabled. Enabling persistence will additionally persist the write operation in IndexedDb so if you close and re-open the app (or refresh the page in a normal web app), the write operation will be restored and we'll again attempt to send it to the backend.

Note that get() calls behave differently since the user is presumably waiting for the data to be returned and so waiting indefinitely while offline is not suitable. So get() calls should fail (and call your catch handler) if the client is offline.

Can you explain your use case a bit more and what UX you are trying to provide? I wonder if failing the write is actually what you want or if some other indicator of "offline-ness" would be more suitable. Thanks!

[mohshraim]

@mikelehen

I wonder if failing the write is actually what you want or if some other indicator of "offline-ness" would be more suitable. Thanks!

This exactly what we looking for... to achieve a better user experience in our targeted apps we should give user feedback of successful save operations..

app.showLoadingIndicator('Saving Data');

db.collection('item').doc('1-1').set(item)
.then(function() {
    app.hideLoadingIndicator();
    app.navigateToNextPage();
    console.log("Document successfully written!");
})
.catch(function(error) {
    app.hideLoadingIndicator();
    app.showError(error.message);
    console.error("Error writing document: ", error);
});

as you can see from code...
->>if we eliminate the -save indicator progress step- and user close the webpage or the mobile app with hybrid JS (while he was offline); then user will lose his save operation as we didn't feed him back of disconnected state (falling back to catch)...
--> and if we just stay on this way and user was offline then saving indicator will stay forever!!!

I would prefer add some options to control the behaviour on set(), update() and maybe batch.commit(); giving us more control
->if sdk will keep try the operation
->or fall back to catch on offline state after the first try.

THANKS

[mikelehen]

The "save indicator" is exactly how we expect people to deal with this sort of situation and provide an optimal user experience, so they can provide feedback to the user that the write hasn't completed yet. You could optionally also have a timer so that if the save indicator doesn't go away after some time period (e.g. 10 seconds) you show a more aggressive notification (e.g. "WARNING: Your writes have not yet been committed to the server. Please ensure your device is online else you risk losing your work.")

If this isn't suitable for your case, I'd be curious to hear more about your use case including under what circumstances you would want the write to fail given that networks are inherently unreliable (temporary network issues are the norm, especially on mobile devices). Would you want an immediate failure? Or would you want a certain number of retries or a timeout or something?

Thanks!

[mohshraim]

@mikelehen
adding timers not a good practice or not fitting with Promises/A+ that we are working on.
giving user a feedback of his operation within max 2-3 seconds,
this give user more control and let him decide to retry or check his connection state or just stop what he was saving...
and if user close the web app while saving; now he knows that his data not saved yet..
so he decide exactly when to close his app according to UX and saving indicator...

adding options to control the behaviour of update retries will enable us to manage the UX..
my suggested options is:
1- number_of_retries: starting by 0 if i want immediate failure after first try. this the important one
2- retries_timeout: the time between retires ... this not that important as the sdk handle the retries timing issue.

the amazing option i wish we can have
3- success_target or target_db: (local, server) .
if local; i just want the success callback when sdk write on IndexDB, that the app UX doesn't care about instant update of server db.
if server; i want the success callback only when save on server DB and prefered not to have any copy for this on IndexDB until success callback from server. -> on this option will be combined with first option 1- number_of_retries.

i hope that i explain the cases which i think any developer looking for.

Thanks.

[mohshraim]

@mikelehen

Do you evaluate the need of number_of_retries options to be added??
at current scenario if our application require only online operation we can't give user any feedback about the operation result..

I don't know how explain, but small scenario..
-> user click save button.. saving indicator wait for 10 second then alert user about failed operation for connection issue.
-> user try again and click save button ... while new saving indicator appear to user.. then internet connection back..
-> result will be tow new record for user save operation (BUG)..

i think we need more control for the save operations..

Thanks..

[mohshraim]

Dear @mikelehen

Do we have any chance to solve this problem?
if not; Do you suggest to go with normal REST request?

Thanks

[mikelehen]

We're interested in exploring features along these lines, though we want to capture more use cases before deciding what the API should look like. In particular I'm wary of adding timeouts and retry options since this is largely contrary to what the Firebase SDK is attempting to provide. It's explicitly designed to handle offline and network interruptions, etc. rather than just being a wrapper over the REST API. I've added your feedback to an internal tracking bug, but we're probably going to hold off on implementing any changes until we get more feedback.

In the meantime if the REST API seems like a better fit to you, you're more than welcome to use it. It will give you full control over retries and timeouts, etc. at the expense of seamless handling for offline / network interruptions.

[mohshraim]

@mikelehen

It's explicitly designed to handle offline and network interruptions, etc.

• This is the first reason for choosing firestore not realtimeDB as no offline existence of data on JS sdk.

• Second : This exactly let us consider this behaviour as bug as we cant feed client with his update operation AND SPECIALLY IF WE DISABLE PERSISTENCE

• Moreover this lead us to request a feature of controlling the success of adding doc to db; if not a number of retries should be same as the feature you are working on get() operation when add a new option for source {default, server , cache}..
  --> if default -- will stay on the current behaviour.
  --> if server -- return the success or fail
  --> if cache or local -- return the success or fail instantly and developer should monitor the metadata if still havePendingChanges.

I hope that you push this case to more firebase users and get a feedback.
meanwhile will try to ignore REST as much as we can,, that we don't want to reinvent the wheel.

Thanks for your following up..

[mikelehen]

@mohshraim Based on having used Firestore for a while longer, do you still feel this is something you would find useful?

[mohshraim]

@mikelehen
Thanks for your follow up.

I think the feature i request in the Subject is not that useful as you mentioned for my current project as i use persistence, and we will start another project soon that disable the persistence which i think not having this feature will lead to bug on our project when user offline.

Will explain two scenario, one exist and the second for our next project.

** First Scenario with enable persistence. **

• This to enable our clients to have full access on cached data while offline.
• My main problem that i did a few work arounds for is as following.
  • User save a doc while he is offline, and as save done as promise, then i should wait for promise result to feed back user with operation result, so with offline case neither success nor fail will be catched. so we can't provide user any feed back about his operation.
  • My currently work around:
    • i make a custom save function (as promise) that will listen to the targeted document (saved document) changes before call the save operation on firestore instance.
    • in case of catch error from firestore save will forward (return) this error. (this only rare cases for us on development environment only should not appear on production, like permission)
    • in case listener detect changes for targeted document save function promise will success.
    • will use this success operation handle the application while this success doesn't mean it is saved on server,
      • so we handle this with special color for UX according to metadata.hasPendingWrite.
    • so now our user know when it is saved locally and when its committed to server 👍
  • Generally i don't like this work around
  • Preferred solution; option on SDK save operation to return success when developer determine {cache success , or server success}. and sure the current behaviour is default.

** Second Scenario with Disable persistence. **

• I am not sure if you already made any changes to this, but according to my last testing from long time as following:
• user load website while online and everything is perfect.
• user went offline for any reason while he try to save (add) some data.
• save promise will not return any result (success or fail), so we forced to put timer to indicate fail operation after 10-15 second.
• user will try again to save (add) the same data; this will make duplicate records if internet back before user refresh his page. (this our bug).
• Work around; when disable persistence always use firestore Transaction as it will give us a real indication of connection and will not try again by it self after fail for connection.
• Preferred Solution; determine the max timeout before operation fail and removed from SDK pending operations. so we don't forced to use Transaction.

Sorry for long description but i have to explain in details..
And big thanks for Amazing Firestore.
Moh'd Shraim

[mikelehen]

Thanks @mohshraim!

Can you explain what the "cache success" promise is for? When you do a set() operation, it'll always be committed to the cache immediately, so I'm thinking it doesn't really make sense to return a Promise for that.

Our current recommendation for the second case (whether you have persistence enabled or not) is to just do the set(), and then use the hasPendingWrites metadata to show some UI indicating it hasn't been committed to the server yet, e.g. "Saving... Please make sure you are online." That way the user doesn't have to wait 10-15 seconds to know if it's working or have to hit save multiple times, etc. But we can consider adding a timeout in the future. It sounds like that's the mechanism you would prefer.

Thanks again for the feedback.

[mohshraim]

@mikelehen

Can you explain what the "cache success" promise is for? When you do a set() operation

it'll always be committed to the cache immediately

There is always a chance for Error while saving on IndexedDB one of them 939. and as firestore sdk on this case -as sample- if error occur when we save sdk doesn't return error,,

For previous; we can't rely that each save will successfully committed to the cache.. so as mentioned before we add listener to doc before call save on SDK and keep listen for this listener if any change occur we make it as (cache success saved).

The second case, as your suggestion then we have also to add listener for doc to make sure its saved, so FR for timeout will be amazing...

Thanks for your follow up

[mohshraim]

@mikelehen
Following another error when use .set(doc) will not go to catch

firebase-firestore.js:1 Uncaught (in promise) Error: Function DocumentReference.set() called with invalid data. Unsupported field value: undefined
sure we should handle it in our code but always we have some cases we can't make sure that .set() operations succeed ..

Thanks

[mikelehen]

@mohshraim Thanks. For #939, I think this is sort of outside the scope of this conversation because the SDK is hitting an internal error due to a bug in Chrome. So the entire SDK ends up in a bad state, and having a "cache success" Promise doesn't solve that.

The "called with invalid data" error is intentionally a synchronous exception (rather than returning a rejected Promise). It indicates a programming error and so we don't expect developers to be programmatically catching it (but you can with try ... catch if you really want to).

[mohshraim]

@mikelehen
I have a question :)
According to #520 (comment)

I can enclose my (set,update) statements with try{ } catch(){ } finally(){}
if reach finally (without or before) catch then i can make sure this doc saved locally??
Thanks

[mikelehen]

For the purposes of writing your app, you can safely assume so, yes.

Technically writes to IndexedDb are asynchronous and so it's possible the client could crash or the user could immediately close the app or something, preventing our write from completing locally. But realistically, the IndexedDb operation should complete in under 10s of milliseconds and so it will be written locally before the user has any chance to do anything.

So we 100% recommend that people just "set it and forget it". If you need a stronger guarantee than that, then you probably want to wait for it to be written to the server anyway and should just wait for the Promise completion.

[danielx]

+1 to this feature.

Our use case with some major simplifications

We use Firestore to maintain state in our app to see if an user have a specific resource open or not.

This is done by having the user update a document every x seconds with a timestamp.

If the user goes offline they are still able to view and update the resource and we save the changes locally (we do not use Firestore due to document size limitations).

With the current retry until successful strategy the user could have closed the resource while offline and once they come online they will send unnecessary updates of their state that is no longer true.

Therefore we would very much like an option to set max retries, a timeout, or perhaps cancel a pending set/update.

[raviqqe]

Note that get() calls behave differently since the user is presumably waiting for the data to be returned and so waiting indefinitely while offline is not suitable.

In my intuition, I expect both get() and set() methods do retries and current inconsistent behavior between them is confusing for me. And that lets developers create their own logics to save documents between online and offline situations for them to create PWA's which work in totally offline situations because we cannot await doc.set() in the same way when the apps are online.

So, If I was an implementer of the API, I would just disable retries of both read and write operations if apps are offline and persistence is enabled as even written changes are eventually synchronized with remote stores anyway.

Another point of discussion is if IndexDB persistence should behave like a cache or a local store. Currently, it behaves like a cache as its write operations never complete offline. But I think it should behave like a local store ...