Assert DownloadTask enqueueWithData not on main thread #1302
Description
- Xcode version: 9.3.1
- Firebase SDK version: 5.0.1
- Firebase Component: Storage
- Component version: 3.0.0
Steps to reproduce:
Before Firebase SDK 5.0 and Storage 3.0 we use to download data from Storage in a background thread, or at least we assume so, because we configure the callbackQueue
property with queue create by us with .background
QoS.
This is my code to configure the Firebase Storage component:
private func setupFirebaseStorage() -> Storage {
let firebaseStorage = Storage.storage()
let queue = DispatchQueue(label: "queue.firebase.storage".reverseDomain,
qos: .background)
firebaseStorage.callbackQueue = queue
return firebaseStorage
}
After the upgrade to Storage v3.0, the application started to stop in the assert in the FIRStorageDownloadTask.m
in the method - (void)enqueueWithData:(nullable NSData *)resumeData
which didn't happened in the previous version of the SDK.
Although, i believe that now is the expected behaviour, but is this correct? Is this something that you want, use the main thread for this? From my point of view, this is a developer responsibility, when the SDK delivers the data on the completion closure.
Relevant Code:
private func setupFirebaseStorage() -> Storage {
let firebaseStorage = Storage.storage()
let queue = DispatchQueue(label: "queue.firebase.storage".reverseDomain,
qos: .background)
firebaseStorage.callbackQueue = queue
return firebaseStorage
}
Call getData(maxSize: Int64)
on a StorageReference to get data for a specific path.
If you need more information, please just ask.
🍻
Activity
portellaa commentedon Jun 1, 2018
Hi 👋
Do you guys have any update on this? I would love to update the Firebase SDK, but i would like to have more details on this and if this will be the expected behaviour.
Cheers
schmidt-sebastian commentedon Jun 6, 2018
Hi @portellaa! I tried to find some changes to our threading behavior since 2.x, and couldn't find anything that stood out (we mostly just changed the way we fetch storage metadata). Can you let us know what operation is hitting this assert?
But, that being said, the assert you are hitting guards the main entry point into the Firebase Storage SDK and ensures that operations run serially.
You should only ever hit this assert if you are calling back into Firebase Storage from a completion callback, which is something that we only support on the main queue.
callbackQueue
only changes the way we call the code you pass to one of the completion handlers. Even without setting this, your downloads will run in the background.portellaa commentedon Jun 11, 2018
Hello and thanks for get back to me.
I understand but why are you guys using the main queue to synchronize serial work? There are a lot of alternatives to make this right, an internal queue for example and you protect yourself against this, wouldn't that be better? You don't need to rely on assumptions or protect your code with this kind of asserts and make this work.
Anyway, i didn't change the code of my app and this was working. I removed the
firebase ~> 4
, runpod update
, fix the issues because of the update on thefirebase/auth
, run the app and it stared failing.Given that, something had to change on the firebase side.
I will share the code relevant for this and this part of my arch.
I have a Store which handles image retrieval, which is configured with a cache entity from here https://github.com/mindera/alicerce/ , a framework where i am a contributor and with a
FirestorageReference
.When some
ViewModel
requires an image, if requests to it's store which in this case will end up on this one, through the method:This is part of the implementation of the store and above is my wrapper to Reactive
Since my persistence is asynchronous, because it can fetch images from the filesystem, the function
getRemoteData
will also be called from that queue.The start is queued from my persistence queue, which isn't correct that i agree, but forcing me to call the function from the main queue, is something that i don't agree, i'm sorry.
I can easily fix this with
.start(on: QueueScheduler.main)
but that is a 🔨 and forcing this kind of work to be done on the main queue is unnecessary, but that it's my opinion.Also, you are relying on that the main queue will always be serial, but what if Apple changes that and makes it concurrent, what does that means on your code? You are still running on the main queue.
Well i will delay the update to this version since i can't agree with this change until i move out from firebase which is an ongoing process and hopefully will occur before i have to update to this version., if not i have a quick 🔨.
So no need to change this because of us, but if want we could submit a PR with what we believe that is the best approach to this.
Feel free to request more info or close this issue if you want 😉
Cheers 🍻