Examine potential retain cycle from unstructured Task blocks

[yo1995]

https://github.com/Esri/arcgis-maps-sdk-swift-samples/blob/b068fdae82d66b88107a508093130d0a85e92200/Shared/Samples/Sketch%20on%20map/SketchOnMapView.swift#L216-L230

According to this discussion, there might be a retain cycle in these 2 Task blocks. Check in debugger and fix if needed.

---

More to be fixed

https://github.com/Esri/arcgis-maps-sdk-swift-samples/blob/62fea7a27765ad2866817db16e7b5ec9e4f47574/Shared/Samples/Download%20preplanned%20map%20area/DownloadPreplannedMapAreaView.Model.swift#L104-L107

https://github.com/Esri/arcgis-maps-sdk-swift-samples/blob/62fea7a27765ad2866817db16e7b5ec9e4f47574/Shared/Samples/Find%20route%20in%20transport%20network/FindRouteInTransportNetworkView.Model.swift#L94

[mhdostal]

@yo1995 I have verified that similar code in the Display device location with NMEA data sources sample also creates a retain cycle. My solution to resolving the problem is below.

The sample has this code in the model class:

Task {
    await observeLocations()
}
...
func observeLocations() async {
    for await location in nmeaLocationDataSource.locations {
        // Do stuff here...
    }
}

Adding the following deinit method to the model verifies that the model is never released (the print line never gets called):

deinit {
    print("NMEA sample model deinit")
}

Following an article recommended by @philium, I started with this:

private var tasks = [Task<Void, Never>]()

deinit {
    print("NMEA sample model deinit")
    tasks.forEach { task in
        task.cancel()
    }
    tasks.removeAll()
}
...
tasks.append(
    Task.detached { [unowned self] in
        await observeLocations()
    }
)

But that still didn't solve the problem. It appears that the call to await observeLocations() is creating the retain cycle. I replaced the await observeLocations() call with the contents of that method:

Task.detached { [unowned self] in
    defer { print("nmeaLocationDataSource.locations task ending") }
    for await location in nmeaLocationDataSource.locations {
        // Do stuff here...
    }
}

and that worked successfully. The model and Task are being dealloc'ed correctly. [I don't know specifically why calling the original method (await observeLocations()) caused the retain cycle; anybody have any clue?]

However, the whole class was designated a @MainActor and I needed to remove that to get the Task.detached code bock to compile. Since the code in the for await location in nmeaLocationDataSource.locations block writes to a @Published property on the model, I was getting the [SwiftUI] Publishing changes from background threads is not allowed error after removing @MainActor (as expected).

To fix that, I simply wrapped the offending code in a DispatchQueue.main.sync block and now all is well:

for await location in nmeaLocationDataSource.locations {
    DispatchQueue.main.sync {
        // Do stuff here...
    }
}

[philium]

In async contexts, MainActor.run(resultType:body:) should be used instead of GCD APIs. But for something like this, it seems like the problem stems from the model kicking off the observation. That's what the task modifier is designed to do! I would suggest refactoring the code in the model to move kicking off the observation out of the initializer and into a method that the view could call from a task modifier.

[mhdostal]

@philium The code in question is in a method on the model, but it's currently being run as the result of a button tap. I will move it into a .task and give it a whirl. Thanks.

[mhdostal]

The model method called when the user selects an external device (or mocked data) kicks off four tasks, one which will finish and three that will not finish (they're watching @Streamed properties). (I think) because of that, using the .task modifier didn't make a difference, I still needed to create Task.detached tasks and hold them in an array for cancellation. In other cases where there's only one task, using the .task modifier probably would work better.

[philium]

A view can have more than one task modifier.

[yo1995]

For a group of for-await-in infinite loops, I settled with sth like below

MapView(map: model.map)
    .task {
        do { try await startGeotriggerMonitors(monitors) } catch { // error handling }
    }

/// Starts the geotrigger monitors and handles posted notifications.
/// - Parameter geotriggerMonitors: The geotrigger monitors to start.
private func startGeotriggerMonitors(_ geotriggerMonitors: [GeotriggerMonitor]) async throws {
    await withThrowingTaskGroup(of: Void.self) { group in
        for monitor in geotriggerMonitors {
            group.addTask {
                try await monitor.start()
                for await newNotification in monitor.notifications where newNotification is FenceGeotriggerNotificationInfo {
                    await model.handleGeotriggerNotification(newNotification as! FenceGeotriggerNotificationInfo)
                }
            }
        }
    }
}

I think there are a few benefits:

1. task modifier's lifetime = view's lifetime, no need to manually cancel a unstructured task.
2. avoid the hassle of managing the lifecycle of unstructured task in the data model.
3. no need of multiple task modifiers - all the tasks are added to 1 task group. Once the task group is canceled, it will propagate to child tasks.