urbanairship / ios-library

Urban Airship iOS SDK
http://urbanairship.com
Apache License 2.0
478 stars 265 forks source link

Apply attribute mutations hangs calling thread when run in background app state #316

Closed SwiftNativeDeveloper closed 2 years ago

SwiftNativeDeveloper commented 2 years ago

Preliminary Info

What Airship dependencies are you using?

14.7.0

What are the versions of any relevant development tools you are using?

Standard Xcode setup (12.3.1) running against iOS 15.4.x.

Report

What unexpected behavior are you seeing?

Apply Mutations to channel attributes appears to hang when app is in the background. I've observed TestFlight and App Store crashes pertaining to a watchdog termination. In the stack frames (which unfortunately, I cannot share) it shows Airship on the main thread, and another thread also doing Airship related items. I suspect a deadlock on the main thread and thus the watchdog termination.

I have a BGProcessingTask that fires to periodically update attribute data based on data that is captured/generated in the background. When my BGProcessingTask fires, I update the relevant attributes and call update.

Artificially, simulating BGProcessingTask events using Xcode tooling, when the app is in the background,

What is the expected behavior?

Airship APIs should not block calling threads when the application is in the background or if they are synchronous and have thread locking nature, it should be clearly documented.

What are the steps to reproduce the unexpected behavior?

  1. Schedule a BGProcessingTask when attribute data changes, specify that the task requires network
  2. In the BGProcessingTask handler, change change the channel attributes for the given iOS device3.
  3. Observe that Airship.channel().apply(mutations) does not return to the calling thread4.
  4. This would result in a task assertion from the BackgroundTasks framework but I put a safeguard in the expiration handler to complete the tasks if anything gets stuck.

Do you have logging for the issue?

No, I cannot share my crash logs or app logs. It does very much appear that there is a thread deadlock or some assumption/prerequisite that the channel operations need to happen when the app is foreground.


    func updateUAirship(backgroundMetadata: [Metadata]? = nil) {
        // Create the attribute mutations
        let mutations = UAAttributeMutations()

        // Update metadata related attributes
        if let backgroundMetadata = backgroundMetadata {
            applyMetadataAttributes(mutations, metadata: backgroundMetadata)
        }

        // NOTE: When debugging BGProcessingTask, the following line does not return when the app is in the background, but does 
        // when the app is in the foreground. Are attribute mutations safe to apply when the app state is in the background?
        // It appears not, but there is no such documentation stating anything about thread safety for this API that I have found.

        // Apply the attribute changes to the channel
        UAirship.channel().apply(mutations)
    }
rlepinski commented 2 years ago

I am not seeing anything obvious that would cause it to block. What queue is this being called from? If you dispatch it to a bg queue does it also block?

rlepinski commented 2 years ago

@SwiftNativeDeveloper I was not able to reproduce.

    let taskID = "com.urbanairship.test_task"

    func application(_ application: UIApplication, didFinishLaunchingWithOptions launchOptions: [UIApplication.LaunchOptionsKey : Any]? = nil) -> Bool {
        registerBGTask()
        return true
    }

    func registerBGTask() {
        BGTaskScheduler.shared.register(forTaskWithIdentifier: taskID, using: nil) { task in
            task.expirationHandler = {
              task.setTaskCompleted(success: false)
            }
            let mutations = UAAttributeMutations()
            mutations.setString("hello", forAttribute: "name")
            UAirship.channel().apply(mutations)
            task.setTaskCompleted(success: true)
        }
    }

    func applicationDidEnterBackground(_ application: UIApplication) {
        do {
            let request = BGProcessingTaskRequest(identifier: taskID)
            request.requiresNetworkConnectivity = true
            try BGTaskScheduler.shared.submit(request)
            print("Submitted task request")
        } catch {
            print("Failed to submit BGTask \(error.localizedDescription)")
        }
    }

I registered the task name in my plist and used this fun command to start after scheduling the task: e -l objc -- (void)[[BGTaskScheduler sharedScheduler] _simulateLaunchForTaskWithIdentifier:@"com.urbanairship.test_task"]

SwiftNativeDeveloper commented 2 years ago

@rlepinski Thanks for giving it a shot. When I registered the the processing task with the system I provided an explicit queue that my "PushDelelgate" type has as a worker thread. I will experiment with modifying the QOS of the queue to see if that changes the behavior for me as that is a difference from my code and yours.

Is there an email I can open up a conversation outside of GitHub that I could perhaps then share parts of a crash log with your team?

rlepinski commented 2 years ago

@SwiftNativeDeveloper ryan.lepinski@airship.com

rlepinski commented 2 years ago

@SwiftNativeDeveloper Any updates? I think I might know whats causing it but I am not sure how to configure a queue to trigger it. Could you share your queue code?

SwiftNativeDeveloper commented 2 years ago

@rlepinski I sent you a private email with details.

For public / GitHub, my background task was originally using the .main GCD queue and we got a number of crashes that illustrated a watchdog termination on the main thread and the last stack frames were the apply mutations.

My updated code attempts to move everything off the main thread into a serial dispatch queue I've created locally to my service and then apply the mutations within that queue.

class PushNotificationService: // implements airship delegates, user notification delegate, etc.
// NOTE property

private let queue: DispatchQueue = DispatchQueue(label: "dns-app-name.PushNotificationService-WorkerQueue")

// NOTE: code that registers it during app delegate finish launching as required by background tasks

        guard backgroundTaskScheduler.register(forTaskWithIdentifier: backgroundTaskIdentifier, using: service.queue, launchHandler: { backgroundTask in

            guard let processingTask = backgroundTask as? BGProcessingTask else {
                assertionFailure("Expected a background processing task type but got something else.")
                backgroundTask.setTaskCompleted(success: false)
                return
            }

            service.handleBackgroundTask(task: processingTask)
        }) else {
            assertionFailure("Failed to register background task with identifier \(backgroundTaskIdentifier). Double check it is in the Info.plist")
            return
        }

// NOTE: snip of the task handler
func handleBackgroundTask(task: BGProcessingTask) {
    self.synchronizeMetadata { result in 

        defer {
            task.setCompleted(true)
        }

        // Omitted expiration handler code if sync chokes, but it doesn't

        switch result { 
        case .success(let metadata):

           // !!!! @rlepinski  here I've tried leaving it on the syncMetadata queue which is a URLSession callback worker queue, 
           // !!!! moving it back to the service queue, etc. 
           // !!!! I am nervous to dispatch it back onto the main queue because we received watchdogs doing so and there is 
           // !!!! nothing documented about the thread safety / or lack of, for the Channel and mutations
           // !!!! They're all serial queues and SHOULD have runtime because we are still active in the background processing task.

            self.updateUAirship(backgroundMetadata: metadata)

        case .failure(let error):
            // error handling removed for clarity
        }
}

// NOTE: Code from above
  func updateUAirship(backgroundMetadata: [Metadata]? = nil) {
      // Create the attribute mutations
      let mutations = UAAttributeMutations()

      // Update metadata related attributes
      if let backgroundMetadata = backgroundMetadata {
          applyMetadataAttributes(mutations, metadata: backgroundMetadata)
      }

      // NOTE: When debugging BGProcessingTask, the following line does not return when the app is in the background, but does 
      // when the app is in the foreground. Are attribute mutations safe to apply when the app state is in the background?
      // It appears not, but there is no such documentation stating anything about thread safety for this API that I have found.

      // Apply the attribute changes to the channel
      UAirship.channel().apply(mutations)
  }
rlepinski commented 2 years ago

Fixed in 16.6.0