quickstart-unity icon indicating copy to clipboard operation
quickstart-unity copied to clipboard

FR: Firestore.SetAsync() should taken a Cancellation Token similar to Storage methods.

Open jonahgoldsaito opened this issue 4 years ago • 4 comments

Feature proposal

  • Firebase Component: Firestore

Firestore should have a flavor of the SetAsync() call that can accept a cancellationToken, ideally similar to Storage's counterparts, like PutFileAsync().

SetAsync( object documentData, SetOptions options, CancellationToken cancelToken )

Though a Firestore Get/Set/Update operation is usually quite quick, I've had times where there is some waiting because of spotty internet connections, or which appear to have failed silently (no isFault or cancellation). In these cases, the wait time can potentially be quite long, and a user (or a timeout I set up) should be able to cancel out.

A side note is that within the quickstart-unity project, the Firestore example is set up with the same structure as Storage, with a cancellationTokenSource. But within the Storage example, the cancellationTokenSource.Cancel() is connected to the Task generated by the Storage API call, but in Firestore there is a similar cancellationTokenSource but without anything in the SetAsync() to connect it up with. I can't find where it has any effect. And I wouldn't be at all surprised if I'm missing something clever 😅

jonahgoldsaito avatar Dec 23 '20 01:12 jonahgoldsaito

Thank you for this feature request, @jonahgoldsaito. I'm actually working on adding CancellationToken parameters to some methods in the Firestore Unity SDK right now; however, methods like SetAsync() are not in scope. The reason is that these operations do not natively support cancellation and so providing a CancellationToken argument incorrectly suggests that these operations could be cancelled when in reality it would provide no such benefit.

The 4 methods that are planned to gain a CancellationToken parameter include:

  1. FirebaseFirestore.RunTransactionAsync()
  2. FirebaseFirestore.ListenForSnapshotsInSync()
  3. DocumentReference.Listen()
  4. Query.Listen()

As for the "dangling" CancellationTokenSource in the Firestore quickstart app https://github.com/firebase/quickstart-unity/blob/241b0e5f527fb0b2088709a16e747d080853ca89/firestore/testapp/Assets/Firebase/Sample/Firestore/UIHandler.cs#L58 you're almost certainly correct that it was copied from the Storage quickstart app and never wired in (because there is nothing to wire it into). I'll make a note to clean it up as part of my work on Firestore's CancellationToken parameter.

If we ever change the implementation of Firestore to natively support the concept of "cancellation" then we will revisit adding CancellationToken parameters to methods like SetAsync(). I'm going to leave this ticket opened for now but we may close it in the future if we decide that we will never add CancellationToken parameters to methods like SetAsync().

Note to Googlers: This feature is being tracked in b/159752361.

dconeybe avatar Dec 23 '20 02:12 dconeybe

Thanks @dconeybe so much for clearing all that up.

If I were to do a RunTransactionAsync() that contained a single SetAsync() and utilized the fancy new CancellationToken you're adding, would that cancel the SetAsync? Or would it actually do the Set, but then revert it? Or neither?

jonahgoldsaito avatar Dec 23 '20 05:12 jonahgoldsaito

In that case it would actually do the "set", but only because transactions do not influence calls to DocumentReference.SetAsync(). In fact, if you're calling SetAsync() inside a transaction that's probably a bug.

In a transaction, instead of calling doc.SetAsync(...) you call Transaction.Set(doc, ...), which is synchronous and simply adds an action to the set of actions that will be performed atomically later when the transaction is committed. When it comes time to commit, if the provided CancellationToken's IsCancellationRequested property equals true then the commit will never be attempted and the transaction will be aborted; however, if the IsCancellationRequested property equaled false and the commit request was sent then the cancellation request will have no effect and the commit operation will run to completion.

https://firebase.google.com/docs/reference/unity/class/firebase/firestore/transaction

dconeybe avatar Dec 23 '20 05:12 dconeybe

Again, thank you for taking the time! You rock 🤘

jonahgoldsaito avatar Dec 23 '20 14:12 jonahgoldsaito