781flyingdutchman / background_downloader

Flutter plugin for file downloads and uploads
Other
162 stars 76 forks source link

Implement multiple background channel support on iOS #382

Closed kuhnroyal closed 4 weeks ago

kuhnroyal commented 1 month ago

Is your feature request related to a problem? Please describe. On iOS multiple plugin registrations cause deadlocks. The postOnBackgroundChannel call never returns which leads to the holding queue being locked.

Describe the solution you'd like Android supports correct handling of multiple plugin registrations and tracking of the primary (firstBackgroundChannel). This needs to be implemented in iOS.

Additional context Discovered while analyzing https://github.com/781flyingdutchman/background_downloader/issues/362

781flyingdutchman commented 1 month ago

Interesting. I was under the impression that multiple plugin registrations are not possible on iOS, as the iOS model does not allow multiple instances of an application to run in parallel (whereas Android does). The multiple plugin issue in Android only arose because of that (e.g. launching the app as a new activity with flags set to create a new instance).

Can you provide an example project (not just code) that triggers multiple plugin registrations, and a log that shows where and when those re-registrations happen? This was extremely complex in the Android case, also because other plugins can be less cooperative and mess things up, so I would want to start from a concrete, reproducable example.

Also, have you tried changing the order of your dependencies in pubspec.yaml, e.g. placing the background_downloader at the top versus the bottom? The order of dependencies is the order of plugin registration, and because some plugins mess things up this can sometimes resolve the problem (it helped on Android).

kuhnroyal commented 1 month ago

There can be multiple Flutter engines attached to the same iOS app, each registering all the plugins.

I already confirmed this fixes the deadlocks by adjusting the BDPlugin.register method with a nil check for the background channel:

    public static func register(with registrar: FlutterPluginRegistrar) {
        os_log("Registering %@", log: log, type: .info, instance)
        let channel = FlutterMethodChannel(name: "com.bbflight.background_downloader", binaryMessenger: registrar.messenger())
        if (backgroundChannel == nil) {
            backgroundChannel = FlutterMethodChannel(name: "com.bbflight.background_downloader.background", binaryMessenger: registrar.messenger())
        }
        registrar.addMethodCallDelegate(instance, channel: channel)
        registrar.addApplicationDelegate(instance)
        let defaults = UserDefaults.standard
        requireWiFi = RequireWiFi(rawValue: defaults.integer(forKey: BDPlugin.keyRequireWiFi))!
    }

I will try to come up with a sample.

781flyingdutchman commented 1 month ago

Ok, that may solve it when there is only one engine that actually uses the downloader (and not just registers it), but it will not solve it if multiple engines do, or if another engine (that doesn't use it) registers it first. That may be enough of a fix for now - let me know what you find.

If not, we have to do the same as on Android where every engine gets its own background channel and each task is linked to the background channel associated with the engine that posted it, so that the appropriate background channel is used to post information back (eg. status updates). It's quite a bit of overhead (and not entire fail proof either, in the case of Android) so I'd like to avoid having to do all that for iOS.

If your simple solution fixes it, can you submit a PR (after running all the tests)? Thanks!

kuhnroyal commented 1 month ago

I added #385 as initial workaround but this should get to feature parity with Android at some point IMO.

github-actions[bot] commented 1 month ago

This issue is stale because it has been open for 14 days with no activity.

781flyingdutchman commented 1 month ago

Will be included in base form in next release. Not full parity with Android, but should do for now.

781flyingdutchman commented 4 weeks ago

Implemented in V8.6.0

kuhnroyal commented 4 weeks ago

Thanks!