Make AnalyticsCoordinator and AnalyticsSourceProvider MainActor-isolated - #4956
Make AnalyticsCoordinator and AnalyticsSourceProvider MainActor-isolated#4956kean wants to merge 1 commit into
Conversation
Removes the Thread.isMainThread check from track() and hops non-isolated call sites onto the main actor instead.
kean
added
refactoring
|
Claude finished @kean's task in 7m 14s —— View job Review
I couldn't build or run tests here (no Xcode toolchain on this runner), so everything below is from reading the code. Overall this is a well-executed migration and the bug it fixes is real: The thing I'd push back on is the ordering argument in the description. The FIFO assumption deserves a caveat
This holds in practice for every pair in this diff, but it isn't a language guarantee: This matters more than it normally would because The places that bundle the source assignment and the triggering call into a single hop ( Related but separate: Inline comments
Things I checked that look correct
One note for later, not this PRSeveral of these hops now send non- |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| let previousSource = Task { @MainActor () -> AnalyticsSource? in | ||
| let previousSource = AnalyticsPlaybackHelper.shared.currentSource | ||
| AnalyticsPlaybackHelper.shared.currentSource = .playbackFailed | ||
| return previousSource | ||
| } | ||
| pause(userInitiated: false) | ||
| AnalyticsPlaybackHelper.shared.currentSource = previousSource | ||
| Task { @MainActor in | ||
| AnalyticsPlaybackHelper.shared.currentSource = await previousSource.value | ||
| } |
There was a problem hiding this comment.
This save/override/restore is the one place where the split into two hops isn't semantics-preserving, and I'd argue it's now worse than inert.
Before, all three writes happened synchronously on the caller's thread, so the net effect on currentSource was exactly zero — a true no-op. Now:
- T1 saves
previousSourceand sets.playbackFailed. - T2 is enqueued behind it, and
await previousSource.valuesuspends T2 even though T1 has already finished — so the main actor is released and any job queued behind T2 runs before the restore lands. - Whatever ran in between may have legitimately set
currentSource(e.g. the user taps play in the player: the VC sets.playerinline,play()enqueues its own hop). T2 then resumes and stamps the stale pre-failure source over it.
Because currentAnalyticsSource is consume-once, that stale value is then attributed to the next event that reads it instead of falling through to topAnalyticsSourceProvider().
Given pause(userInitiated: false) already suppresses the only event this was meant to attribute (as you note in the description), I'd just delete the block in this PR rather than carry the Task-value dance forward:
| let previousSource = Task { @MainActor () -> AnalyticsSource? in | |
| let previousSource = AnalyticsPlaybackHelper.shared.currentSource | |
| AnalyticsPlaybackHelper.shared.currentSource = .playbackFailed | |
| return previousSource | |
| } | |
| pause(userInitiated: false) | |
| AnalyticsPlaybackHelper.shared.currentSource = previousSource | |
| Task { @MainActor in | |
| AnalyticsPlaybackHelper.shared.currentSource = await previousSource.value | |
| } | |
| pause(userInitiated: false) |
If you'd rather keep it faithful for now, collapsing it into a single hop makes the no-op explicit (and self-evidently deletable):
Task { @MainActor in
let previousSource = AnalyticsPlaybackHelper.shared.currentSource
AnalyticsPlaybackHelper.shared.currentSource = .playbackFailed
AnalyticsPlaybackHelper.shared.currentSource = previousSource
}
pause(userInitiated: false)
| let manager = DownloadManager(dataManager: DataManager.sharedManager) | ||
| AnalyticsEpisodeHelper.shared.setup() | ||
| Task { @MainActor in | ||
| AnalyticsEpisodeHelper.shared.setup() |
There was a problem hiding this comment.
setup() is an empty method whose only job is to force AnalyticsEpisodeHelper.shared to initialize, and that initialization is what registers the episodeDownloaded / userEpisodeUploadStatusChanged observers.
Deferring it into a Task means observer registration now happens on a later main-actor turn than DownloadManager.shared initialization. AppDelegate.swift:110 touches DownloadManager.shared and immediately calls startAllQueued(), so there's now a window at launch where downloads are running but nothing is listening — any download that completes in that window silently loses its episode_download_finished event (and leaves a stale entry in episodeDownloadSources).
Narrow in practice, but easy to avoid: now that the helper is main-actor isolated, this side effect reads better as an explicit call from the main-actor launch path rather than a hop hidden in a lazy static.
| let manager = DownloadManager(dataManager: DataManager.sharedManager) | |
| AnalyticsEpisodeHelper.shared.setup() | |
| Task { @MainActor in | |
| AnalyticsEpisodeHelper.shared.setup() | |
| return manager | |
| }() |
…and call AnalyticsEpisodeHelper.shared.setup() from AppDelegate alongside the other launch-time setup.
| Task { @MainActor in | ||
| AnalyticsEpisodeHelper.shared.downloaded(episodeUUID: uuid) | ||
| } | ||
| DownloadManager.shared.addToQueue(episodeUuid: uuid) |
There was a problem hiding this comment.
downloaded(episodeUUID:) isn't only a track() — it also does the bookkeeping the completion observer depends on:
func downloaded(episodeUUID: String) {
let source = cacheDownloadSource(for: episodeUUID)
episodeDownloadQueue.insert(episodeUUID) // <-- gate for episode_download_finished
...
}Previously that insert was guaranteed to happen before addToQueue. Now addToQueue runs first and the insert lands on a later main-actor turn, so a download that completes very fast (already-present file, warm cache) can post episodeDownloaded before the UUID is in episodeDownloadQueue — episodeDownloadedNotificationReceived bails on the contains guard and episode_download_finished is dropped.
Moving addToQueue inside the hop restores the ordering, and has the bonus of getting the DownloadManager call onto a consistent actor:
| Task { @MainActor in | |
| AnalyticsEpisodeHelper.shared.downloaded(episodeUUID: uuid) | |
| } | |
| DownloadManager.shared.addToQueue(episodeUuid: uuid) | |
| for uuid in uuids { | |
| Task { @MainActor in | |
| AnalyticsEpisodeHelper.shared.downloaded(episodeUUID: uuid) | |
| DownloadManager.shared.addToQueue(episodeUuid: uuid) | |
| } | |
| } |
If you'd rather not change when the download actually starts, a single hop wrapping the whole loop for the analytics side would work too.
| Task { @MainActor in | ||
| AnalyticsPlaybackHelper.shared.currentSource = .watch | ||
| } | ||
| if PlaybackManager.shared.isPlaying { | ||
| PlaybackManager.shared.pause() | ||
| } else { | ||
| PlaybackManager.shared.play() | ||
| } | ||
| } else if WatchConstants.Messages.SkipBackRequest.type == messageType { | ||
| AnalyticsPlaybackHelper.shared.currentSource = .watch | ||
| DispatchQueue.main.async { | ||
| Task { @MainActor in | ||
| AnalyticsPlaybackHelper.shared.currentSource = .watch | ||
| PlaybackManager.shared.skipBack() | ||
| } |
There was a problem hiding this comment.
Nit / consistency: SkipBackRequest below puts the source assignment and the PlaybackManager call in the same hop (good — that's the pattern PodcastViewController uses too), but PlayPauseRequest here and SkipForwardRequest leave the PlaybackManager call outside it. All three would be more obviously correct with the same shape, and it would also move PlaybackManager.shared.isPlaying off the WatchConnectivity queue:
| Task { @MainActor in | |
| AnalyticsPlaybackHelper.shared.currentSource = .watch | |
| } | |
| if PlaybackManager.shared.isPlaying { | |
| PlaybackManager.shared.pause() | |
| } else { | |
| PlaybackManager.shared.play() | |
| } | |
| } else if WatchConstants.Messages.SkipBackRequest.type == messageType { | |
| AnalyticsPlaybackHelper.shared.currentSource = .watch | |
| DispatchQueue.main.async { | |
| Task { @MainActor in | |
| AnalyticsPlaybackHelper.shared.currentSource = .watch | |
| PlaybackManager.shared.skipBack() | |
| } | |
| } else if WatchConstants.Messages.PlayPauseRequest.type == messageType { | |
| Task { @MainActor in | |
| AnalyticsPlaybackHelper.shared.currentSource = .watch | |
| if PlaybackManager.shared.isPlaying { | |
| PlaybackManager.shared.pause() | |
| } else { | |
| PlaybackManager.shared.play() | |
| } | |
| } | |
| } else if WatchConstants.Messages.SkipBackRequest.type == messageType { | |
| Task { @MainActor in | |
| AnalyticsPlaybackHelper.shared.currentSource = .watch | |
| PlaybackManager.shared.skipBack() | |
| } |
(Only worth doing if you're comfortable with play/pause now starting a turn later — the old code was synchronous here, unlike skip-back which already hopped.)
| if let extraProperties = properties { | ||
| baseProperties = baseProperties.merging(extraProperties, uniquingKeysWith: { current, _ in return current}) | ||
| } | ||
| analyticsPlaybackHelper.track(event, properties: baseProperties) | ||
| let mergedProperties = baseProperties | ||
| Task { @MainActor in | ||
| AnalyticsPlaybackHelper.shared.track(event, properties: mergedProperties) | ||
| } |
There was a problem hiding this comment.
let mergedProperties = baseProperties only exists to get an immutable binding for the closure capture. Making baseProperties a let in the first place is a bit tidier:
| if let extraProperties = properties { | |
| baseProperties = baseProperties.merging(extraProperties, uniquingKeysWith: { current, _ in return current}) | |
| } | |
| analyticsPlaybackHelper.track(event, properties: baseProperties) | |
| let mergedProperties = baseProperties | |
| Task { @MainActor in | |
| AnalyticsPlaybackHelper.shared.track(event, properties: mergedProperties) | |
| } | |
| func trackChapterEvent(_ event: AnalyticsEvent, properties: [String: Any]? = nil) { | |
| let baseProperties = chapterManager.chaptersAnalyticsProperties | |
| .merging(properties ?? [:], uniquingKeysWith: { current, _ in current }) | |
| Task { @MainActor in | |
| AnalyticsPlaybackHelper.shared.track(event, properties: baseProperties) | |
| } | |
| } |
|
|
||
| // MARK: - AnalyticsPlaybackHelper Mock | ||
|
|
||
| @MainActor |
There was a problem hiding this comment.
Minor: AnalyticsPlaybackHelper already inherits @MainActor from AnalyticsCoordinator, so the annotation on the subclass is redundant. (The one on the XCTestCase above is necessary, of course.)
1 participant
Marks
AnalyticsSourceProviderandAnalyticsCoordinator(and thereforeAnalyticsEpisodeHelper/AnalyticsPlaybackHelper) as@MainActor.track()used to checkThread.isMainThreadand re-dispatch itself withDispatchQueue.main.async. That check is gone — the isolation now guarantees it. It also fixes a real bug: onlytrack()hopped, so the surroundingcurrentSourcebookkeeping ran inline on the caller's thread, andcurrentAnalyticsSourcewould reachSceneHelper.rootViewController()andUIApplication.shared.applicationStateoff the main thread.Task { @MainActor in … }hops are temporaryDraft because the hops added at non-isolated call sites are scaffolding, not the destination. They exist only so this can land ahead of the callers, and they should disappear one by one as
PlaybackManager,EpisodeManager,SiriShortcutsManager,DownloadManagerand friends become main-actor isolated themselves — continuing the incremental top-down adoption from #4953. Once a caller is isolated, its hop collapses into a plain synchronous call, which is what most of the analytics call sites in this diff already look like.Reviewing the hops as permanent API would be reading them wrong; the thing to check here is the isolation of the two analytics types and that no call site changed meaning.
All 110 call sites were checked. The ones already on the main actor (view controllers,
MultiSelectHelper,PlaybackActionHelper,PlaybackManager.playBookmark, the tvOS SwiftUI views) call through synchronously with no hop at all. The genuinely non-isolated ones got an explicitTask { @MainActor in … }:PlaybackManager,EpisodeManager,SiriShortcutsManager,ServerSyncManager,DownloadManager,GoogleCastManager,WatchManager,UserEpisodeManager, and fourDispatchQueue.global()blocks inPodcastViewController. Ordering within each call chain is preserved because the hops are enqueued in source order — which matters for the set-currentSource-then-track andignoreNextSeekpatterns.Other notes:
SearchAnalytics.episodeTapped,AppCoordinator.remotePlayPauseToggle,PlayerStatusObserver.updatePlayStateand theSearchableViewModel.playEpisoderequirement were annotated@MainActorinstead of hopping — their callers are all main-actor already.OnboardingFlow'sAnalyticsSourceProviderconformance moved to an extension so the struct itself doesn't inherit@MainActor(it's reached from the non-isolatedIAPHelper).AnalyticsPlaybackHelperarenonisolated— they only read a feature flag, the episode andPlaybackManager.shared, so non-isolated callers and the tests don't need a hop.AnalyticsEpisodeHelper'sThreadSafeDictionaryis now a plain dictionary, and its two notification observer bodies were extracted into named methods.PlaybackManager.playbackDidFailsaves/overrides/restorescurrentSourcearoundpause(userInitiated: false). I kept it faithful rather than movepause()into the hop, sincecleanupCurrentPlayerbelow depends on the pause having run. Worth flagging that this block is inert today: it was written in 5064b71 when the call waspause(), anduserInitiated: falsenow suppresses the very event it was meant to attribute. Happy to remove it in a follow-up.To test
playback_play/playback_pause/playback_skip_back/playback_skip_forwardare logged withsource: player.sourcematches the surface you used rather than falling back tounknown.playback_seekfires, and that skipping back/forward does not additionally fire one.episode_download_queuedandepisode_download_finishedboth reportsource: podcast_screen.source: filters.source: podcast_screen.source: siri.source: watch.search_result_tappedplusplayback_playwithsource: search.Checklist
CHANGELOG.mdif necessary.