Assert DownloadTask enqueueWithData not on main thread

[portellaa]

• 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.
🍻

[portellaa]

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]

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]

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, run pod update, fix the issues because of the update on the firebase/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:

func image(for path: Path) -> SignalProducer<UIImage?, ImageFirebaseStoreError> {
        return getPersistenceData(for: path)
            .map(UIImage.init(data:))
            .flatMapError { [weak self] in
                guard let strongSelf = self else { return SignalProducer.empty }

                switch $0 {
                case .noData: return strongSelf.getRemoteData(for: path).map(UIImage.init(data:))
                default: return SignalProducer(error: $0)
                }
        }
    }

private func getPersistenceData(for path: Path) -> SignalProducer<Data, ImageFirebaseStoreError> {
        let key = persistenceKey(for: path)
        return SignalProducer { observer, lifetime in
            self.persistenceStack.object(for: key) { (inner: () throws -> Data) -> Void in
                do {
                    let data = try inner()

                    observer.send(value: data)
                } catch Alicerce.Persistence.Error.noObjectForKey {
                    // cache/persistence miss
                    observer.send(error: .noData)
                } catch {
                    log.error("⚠️ Failed to get persisted value for path \(path) with error \(error). Go fetch...")
                    observer.send(error: .persistence(error))
                }
            }
        }
    }

private func getRemoteData(for path: Path) -> SignalProducer<Data, ImageFirebaseStoreError> {
        return firebaseStorage.child(path).reactive
            .getData(maxSize: Constants.maxSize)
            .on { [weak self] in self?.persist(data: $0, for: path) }
            .mapError(ImageFirebaseStoreError.remote)
    }

This is part of the implementation of the store and above is my wrapper to Reactive

func getData(maxSize: Int64) -> SignalProducer<Data, NSError> {
        return SignalProducer { observer, lifetime in
            let downloadTask = self.base.getData(maxSize: maxSize) { (data, error) in
                if let error = error {
                    return observer.send(error: error as NSError)
                }

                defer { observer.sendCompleted() }

                guard let data = data else { return }

                observer.send(value: data)
            }

            lifetime += self.lifetime.ended.observeCompleted(observer.sendCompleted)

            lifetime.observeEnded(downloadTask.cancel)
        }
    }

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 🍻