firebase / firebase-ios-sdk

Firebase SDK for Apple App Development
https://firebase.google.com
Apache License 2.0
5.59k stars 1.46k forks source link

Setting custom callback queue in Firestore doesn't affect Firestore-dependent Auth callbacks #7911

Open lorenzofiamingo opened 3 years ago

lorenzofiamingo commented 3 years ago

Environment

Problem

I need to make a call to Firestore and wait for the query result. Since Firestore make its async calls on the main thread and I need my code to be on the main thread, I have to change the default dispatchQueue used by Firestore callbacks, in order to execute them while I'm waiting. When I run the project this is the output:

2021-04-16 19:06:12.080367+0200 FirestoreIncrementalStore[1794:547410] 7.10.0 - [Firebase/Firestore][I-FST000001] Could not reach Cloud Firestore backend. Backend didn't respond within 10 seconds.
 This typically indicates that your device does not have a healthy Internet connection at the moment. The client will operate in offline mode until it is able to successfully connect to the backend.
Async Optional(<FIRQuerySnapshot: 0x12eb044e0>)
Sync Optional(<FIRQuerySnapshot: 0x12eb044e0>)

So the code works, since I received the correct snapshot, but I had to wait 10 secs for the timeout of the internet connection (untrue, since the query ran well). My only explanation is that exist a piece of async code inside Firestore APIs that is not performed using the dispatchQueue of firestore.settings (maybe the callback that stops the timer).

Steps to reproduce:

Run the following code in a sample project

let firestore: Firestore = .firestore()
let dispatchGroup = DispatchGroup()
var syncSnapshot: QuerySnapshot? = nil
dispatchGroup.enter()
let settings = FirestoreSettings()
settings.dispatchQueue = DispatchQueue(label: "com.test.my-thread")
firestore.settings = settings
firestore.collection(path).getDocuments { snapshot, error in
    guard let snapshot = snapshot else { return }
    syncSnapshot = snapshot
    print("Async", syncSnapshot)
    dispatchGroup.leave()
}
dispatchGroup.wait()
print("Sync", syncSnapshot)

PS:

firestore.settings.dispatchQueue = DispatchQueue(label: "com.test.my-thread") doesn't work in order to set the queue.

rizafran commented 3 years ago

Hi @lorenzofiamingo, we'll try to reproduce this, and let you know with the updates or findings. It'll also be better if you can share a sample project that I can run locally. Thanks.

lorenzofiamingo commented 3 years ago

Hi @rizafran, you should be able to reproduce it in any kind of project, just pasting the code above at any point, for example after FirebaseApp.configure() in AppDelegate.

dconeybe commented 3 years ago

Hi @rizafran. The code sample you provided will cause the main thread to block. This is a hazardous thing to do as it will cause your app to become unresponsive while waiting for Firestore to call back, which could take seconds, or even longer if the collection is large.

Firestore's API is specifically designed so that its work, like downloading documents from the server, does not block the main thread. Some of its initialization code (i.e. code that is executed the first time that it connects to the backend) requires that the main thread not be blocked. So by blocking the main thread it is causing Firestore's initialization to time out. Another customer had the same issue and it was resolved by moving the call to wait() off of the main thread: https://github.com/firebase/firebase-ios-sdk/issues/7615.

I'm going to close this issue because we specifically do not support the use case of blocking the main thread waiting for Firestore to download documents from the server. Please try to restructure your code so that the main thread does not get blocked.

lorenzofiamingo commented 3 years ago

Hi @dconeybe. The problem remains: setting the dispatchQueue in the FirestoreSettings has no effect. If I want to block the main thread for a reason, I should be able to move the asynchronous tasks of Firestore in an another queue.

Causes can be two:

  1. When getDocuments is called not all the asynchronous tasks are sent to the dispatchQueue of FirestoreSettings.
  2. Firestore.firestore() has some asynchronous tasks sent in the default FirestoreSetting.dispatchQueue.

If it's case 2 this should exist: Firestore.firestore(app: FirebaseApp, with settings: FirestoreSettings = .default) in order to set the DispatchQueue before init.

If it's case 1 and you don't want to support that (what's the purpose of FirestoreSettings.dispatchQueue, then?), I have to say that I cannot change the code, since I'm trying to create a NSIncrementalStore for Firestore and it works on the DispatchQueue that CoreData choose for it.

Therefore, you'll never probably be able to adopt async/await without solving this use case, since an await would be the perfect replacement for my sample code.

dconeybe commented 3 years ago

Setting the dispatch queue in FirestoreSettings specifies the dispatch queue for dispatching user callbacks, such as calling the block of code specified to getDocuments(). There are other internal implementation details where work is scheduled on the main thread that are not visible outside of the Firestore internals.

I must admit that I'm not personally familiar with NSIncrementalStore so I don't fully understand why the main thread needs to be blocked in order to use it. But I encourage you to find an alternate way of using it since blocking the main thread is never a good idea. If you could provide a minimal app (e.g. via a GitHub repository) that demonstrates your use case, perhaps we could find an alternative.

As a workaround, would it be possible to make a Firestore call that hits the backend server without blocking the main thread before the problematic Firestore call? That way, the one-time initialization that schedules work on the main thread would have already completed.

Note that usage of async/await would not be blocked by this implementation detail of Firestore because await does not block the main thread but rather suspends the current path of execution, allowing other code to continue on the main thread. In contrast, calling dispatchGroup.wait() on the main thread blocks the main thread completely, preventing it from doing any other work.

lorenzofiamingo commented 3 years ago

https://github.com/lorenzofiamingo/FirestoreIncrementalStore

In the db I created two items, they arrive after a 10 seconds timeout. (as shown by the console, not the UI)

dconeybe commented 3 years ago

Thanks @lorenzofiamingo. I've been able to reproduce with your app. I'm talking to some team members about this issue and hope to reply in the next few days.

lorenzofiamingo commented 3 years ago

@dconeybe Thanks to you.

dconeybe commented 3 years ago

@lorenzofiamingo Could you provide some additional information about your use case? Any context you could provide could help investigation on our end.

lorenzofiamingo commented 3 years ago

@lorenzofiamingo Could you provide some additional information about your use case? Any context you could provide could help investigation on our end.

I’m trying to get Firestore to work with CoreData. To accomplish that in the best way possible I want to create a reusable and general purpose swift package. I’m exploring two ways: 1) Create a new NSPersistentStore (NSIncrementalStore) called NSFirebaseStore 2) Create a NSPersistentContainer called NSPersitentFirebaseContainer, as Apple did with CloudKit (NSPersistentCloudKitContainer)

For the 1) way I was following this.

dconeybe commented 3 years ago

tl;dr The "internal implementation details where work is scheduled on the main thread" turns out to be in Firebase Auth, where the main dispatch queue is hardcoded for callbacks. I tested out changing these Auth callbacks to instead use a background dispatch queue and it worked perfectly in my isolated test, allowing the code presented in the first comment of this issue to succeed. I'll dig into this a little more.

I found the sources of the internal usages of the main dispatch queue. They all reside in FirebaseAuth:

https://cs.opensource.google/search?q=dispatch_get_main_queue%20f:FirebaseAuth&ss=firebase-sdk%2Ffirebase-ios-sdk

I tested out a "fix" of replacing all calls to dispatch_get_main_queue() with calls to dispatch_get_global_queue(QOS_CLASS_USER_INITIATED, 0) and that fixed the "Could not reach Cloud Firestore backend. Backend didn't respond within 10 seconds." error and also allowed the callback specified to getDocuments() to actually be invoked.

I haven't yet investigated what the side effects would be of this change, and if it invalidates assumptions made elsewhere in the code about the threading model. If we were to go down this route then it would more likely look like providing a parameter/setting to the Auth library whereby a dispatch queue can be configured (rather than hardcoding the main dispatch queue). My expertise lies in Firestore (and not Auth) so I've added the "api: auth" label to this issue to see if someone with expertise in Auth can comment.

dconeybe commented 3 years ago

One last note before @morganchen12 takes over this issue... talking briefly with the Auth folks yielded this interesting document: https://github.com/firebase/firebase-ios-sdk/blob/master/FirebaseAuth/Docs/threading.md. tl;dr They explicitly decided to use the main dispatch queue for callbacks to make things easy from a concurrency perspective. This is not to say it couldn't be changed (or made to be configurable) but it will require some degree of care to change it correctly and in a backwards-compatible manner.

morganchen12 commented 3 years ago

Hey all, apologies for the delay on this issue. @lorenzofiamingo can you try out the changes in this PR? https://github.com/firebase/firebase-ios-sdk/pull/8288

This will need to go through API review, but in the meantime it'd be good to get your feedback. cc @rosalyntan