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.

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

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

Open
jonahgoldsaito opened this issue Dec 23, 2020 · 4 comments

Comments

@jonahgoldsaito
Copy link

jonahgoldsaito commented Dec 23, 2020

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 😅

@dconeybe
Copy link

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

protected CancellationTokenSource cancellationTokenSource = new CancellationTokenSource();

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.

@jonahgoldsaito
Copy link
Author

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?

@dconeybe
Copy link

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

@jonahgoldsaito
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants