Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AVPlayerSubscription is not thread safe #816

Closed
hakonk opened this issue Aug 25, 2023 · 1 comment
Closed

AVPlayerSubscription is not thread safe #816

hakonk opened this issue Aug 25, 2023 · 1 comment
Labels
type:defect Bugs or weaknesses. The issue has to contain steps to reproduce.
Milestone

Comments

@hakonk
Copy link

hakonk commented Aug 25, 2023

Describe the bug
I have not encountered a bug related to this. However, it is evident from the way KVO and subscription to NotificationCenter is employed that AVPlayerSubscription is not thread safe. This is because notifications within AVFoundation as well as KVO updates may be called on different threads. Consider this example AVPlayerSubscription.swift:49-66,

rateObserver = player.observe(\.rate, options: [.old, .new]) { [weak self] player, change in
            guard let oldRate = change.oldValue else { return }
            guard let newRate = change.newValue else { return }

            if oldRate != 0 && newRate == 0 { // paused
                self?.lastPauseTime = player.currentTime()
                self?.track(MediaPauseEvent())
            } else if oldRate == 0 && newRate != 0 { // started playing
                // when the current time diverges significantly, i.e. more than 1 second, from what it was when last paused, track a seek event
                if let lastPauseTime = self?.lastPauseTime {
                    if abs(player.currentTime().seconds - lastPauseTime.seconds) > 1 {
                        self?.track(MediaSeekEndEvent())
                    }
                }
                self?.lastPauseTime = nil
                self?.track(MediaPlayEvent())
            }
        }

Here self is accessed within the closure passed into the KVO. This closure may be called on any thread AVFoundation wants to. Oftentimes it will be the main thread, however, this is highly dependent on the state of the device on which the code is executing, and thus hard to reproduce. The same goes for this code AVPlayerSubscription.swift:94-105:

@objc private func handleNotification(_ notification: Notification) {
        switch notification.name {
        case .AVPlayerItemPlaybackStalled:
            track(MediaBufferStartEvent())
        case .AVPlayerItemDidPlayToEndTime:
            track(MediaEndEvent())
        case .AVPlayerItemFailedToPlayToEndTime:
            track(MediaErrorEvent(errorDescription: player.error?.localizedDescription))
        default:
            return
        }
    }

Notifications may be sent on different threads. The code in the body of the method above is also thread agnostic and lacking synchronisation.

@hakonk hakonk added the type:defect Bugs or weaknesses. The issue has to contain steps to reproduce. label Aug 25, 2023
@matus-tomlein matus-tomlein added this to the 6.0.0 milestone Feb 1, 2024
@matus-tomlein
Copy link
Contributor

Fixed in version 6.0.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:defect Bugs or weaknesses. The issue has to contain steps to reproduce.
Projects
None yet
Development

No branches or pull requests

2 participants