Firestore futures don't complete on desktop

[mark-grimes]

Please fill in the following fields:

Pre-built SDK from the website or open-source from this repo: website
Firebase C++ SDK version: 6.15.0
Firebase plugins in use (Auth, Database, etc.): Firestore, Auth
Additional SDKs you are using (Facebook, AdMob, etc.): N/A
Platform you are using the C++ SDK on (Mac, Windows, or Linux): Mac
Platform you are targeting (iOS, Android, and/or desktop): iOS and desktop (iOS is fine)

Please describe the issue here:

(Please list the full steps to reproduce the issue. Include device logs, Unity logs, and stack traces if available.)

Apply firebase/quickstart-cpp#47 so that the firestore quickstart will compile. Compile and run. The logs have a lot of ERROR: <something> returned an invalid result which looking at the code is the error printed when the timeout elapses.
Full output from the quickstart:

Initialized Firebase App.
Initializing Firebase Auth...
Initialized Firebase Auth.
Signing in...
Signed in as an anonymous user, uid: T3ui2N9WzMXFLBTdvjNflZDHFzR2, email: .

Initialize Firebase Firestore.
Attempt to initialize Firebase Firestore.
Successfully initialized Firebase Firestore.
Successfully set Firestore settings.
Testing non-wrapping types.
Tested non-wrapping types.
Testing collections.
Tested collections.
Testing documents.
Testing Set().
ERROR: document.Set returned an invalid result.
ERROR: failed to write document.
Testing Update().
ERROR: document.Update returned an invalid result.
ERROR: failed to write document.
Testing Get().
ERROR: document.Get returned an invalid result.
Testing Delete().
ERROR: document.Delete returned an invalid result.
ERROR: failed to delete document.
Tested document operations.
Successfully added and removed document snapshot listener.
Testing batch write.
ERROR: batch.Commit returned an invalid result.
ERROR: failed to write batch.
Tested batch write.
Testing transaction.
ERROR: firestore.RunTransaction returned an invalid result.
ERROR: failed to run transaction.
Tested transaction.
Testing query.
ERROR: query.Get returned an invalid result.
ERROR: failed to fetch query result.
Tested query.
Shutdown the Firestore library.
WARNING: Future with handle 1 still exists though its backing API 0x48C0C6C0 is being deleted. Please call Future::Release() before deleting the backing API.
WARNING: Future with handle 1 still exists though its backing API 0x48E077E0 is being deleted. Please call Future::Release() before deleting the backing API.
WARNING: Future with handle 1 still exists though its backing API 0x48E2B640 is being deleted. Please call Future::Release() before deleting the backing API.
WARNING: Future with handle 1 still exists though its backing API 0x48E2F1B0 is being deleted. Please call Future::Release() before deleting the backing API.
WARNING: Future with handle 2 still exists though its backing API 0x48E2F1B0 is being deleted. Please call Future::Release() before deleting the backing API.
WARNING: Future with handle 3 still exists though its backing API 0x48E2F1B0 is being deleted. Please call Future::Release() before deleting the backing API.
WARNING: Future with handle 4 still exists though its backing API 0x48E2F1B0 is being deleted. Please call Future::Release() before deleting the backing API.
Shutdown Auth.
WARNING: Future with handle 1 still exists though its backing API 0x48D049B8 is being deleted. Please call Future::Release() before deleting the backing API.
WARNING: Future with handle 3 still exists though its backing API 0x48D049B8 is being deleted. Please call Future::Release() before deleting the backing API.
WARNING: Future with handle 1 still exists though its backing API 0x48E06BB0 is being deleted. Please call Future::Release() before deleting the backing API.
WARNING: Future with handle 4 still exists though its backing API 0x48E06BB0 is being deleted. Please call Future::Release() before deleting the backing API.
Shutdown Firebase App.
Tests PASS.
WARNING: Future with handle 1 still exists though its backing API 0x48E2F730 is being deleted. Please call Future::Release() before deleting the backing API.

In my own project code I have:

pFirestore->Collection("myCollection").Get().OnCompletion( <some lambda> );

as part of fairly extensive use of Firestore. It all runs fine on iOS but the lambda is never called on desktop.

Please answer the following, if applicable:

Have you been able to reproduce this issue with just the Firebase C++ quickstarts ?

Yes, after updating the quickstart so that it compiles with 6.15.0 (see firebase/quickstart-cpp#47)

What's the issue repro rate? (eg 100%, 1/5 etc)

100% on desktop.

[morganchen12]

Just to sanity check, what are your database's security rules?

edit: Actually based on the code running fine on iOS it's unlikely security rules are the issue here.

[wilhuff]

The issue here is that the default dispatch queue for resolving futures is the the main queue on Apple platforms. The quickstart is running directly out of the main thread though so the main queue is never getting processed.

On iOS running with UIApplicationMain or macOS running with NSApplicationMain or similar you won't see this problem.

A workaround for code in this scenario is to set a non main queue as the default callback queue using Settings::set_dispatch_queue.

We have a change pending that will change the C++ SDK to always call back on non-main queues/threads to avoid this problem in the future.

[mark-grimes]

Okay. Thanks for the explanation. Our code is packaged as a pure C++ cross platform library which is then wrapped for iOS (and eventually Android) so the #if defined(__OBJC__) rules out your suggestion. Would an all-platforms set_dispatch_queue( std::thread ) be feasible? Otherwise having it handled transparently as in your mentioned feature works.

In practice any desktop code we write (e.g. internal admin tools using the said library) will not be susceptible to this - it was a quick test on desktop in a simple main() to make sure it worked that was failing. So your description is helpful, thanks.

[wilhuff]

In tests, the way we've handled setting the dispatch queue is to have one .mm file in the project with a single SetDefaultDispatchQueue function in it, that's callable from vanilla C++. On other platforms there's another definition that's a no-op.

Re std::thread there are a couple problems:

• std::thread defines how to start a thread but doesn't provide a way to push work to a particular thread. For that we'd need more of an Executor-style interface.
• On Apple platforms we're not actually using std::thread anyway since we're using Grand Central Dispatch. There's no straightforward/efficient way to bridge between them.

Eventually we'll have to come up with some cross-platform way to handle this, but I think changing the default to some other queue makes this not broken by default and handles all but the most demanding use cases.

[chkuang-g]

This seems to be related to the fix in 6.15.1.
https://firebase.google.com/support/release-notes/cpp-relnotes#version_6151_-_june_29_2020

Does it still happen after you upgrade to 6.15.1?

[mark-grimes]

It still happens. My understanding about the fix in 6.15.1 is it's just the warnings about handle lifetimes when you shut down.

[chkuang-g]

@mark-grimes

The warning does not seems to have adverse effect. We still need to figure out how to get rid of it from our side.
For the Future::OnComplete() issue on desktop, this should be fixed in the next release.