strummachine / sm-audiokit

0 stars 0 forks source link

More problems with completionHandler #10

Open banjerluke opened 2 years ago

banjerluke commented 2 years ago

See #5 for context. I'm having a different issue now, although it's also a mutex deadlock situation. But this time it's one hung call to AudioPlayer.stop(), and a bunch of calls hung on isInManualRenderingMode:

image

(Thread 2 is expanded but they're all identical.)

I've only had this happen when running in the testbed Cordova app, with the BPM cranked way up, and letting it run for a minute. Still, unacceptable for production...

banjerluke commented 2 years ago

I just tried pausing execution at random points, and there were no queued calls like in the screenshot. But after a few times, it did hang again shortly after resuming execution. This time when I paused again I saw that it was again hung on AudioPlayer.stop(), but there was only one of the "Queue : AVAudioPlayerNodeImpl.CompletionHandlerQueue" threads.

banjerluke commented 2 years ago

Pausing execution and resuming, with BPM cranked up, seems to be a pretty reliable way of getting in a mutex deadlock situation.

My hunch is that the problem arises when an AudioPlayer is getting reused while it's still playing, or more likely in the process of stopping. .stop() gets called while the completionHandler is getting run, and the two are stuck in a deadlock.

The solution I came up with is to modify AudioManager.playSample. What I want to do is check right after the call to channel.getPlayer to see if the returned player: SamplePlayer's .available property is false (without checking the player directly, to avoid another deadlock situation).

If the player is not available, that means we've reached the polyphony limit and we now have to reuse a player (getPlayer returns the oldest player for minimal disruption). My idea is to wait like 25ms before continuing in this scenario. In Javascript land, I would make this an async function that returns a Promise, and finish the rest of the function (lines 113-123) after a 25ms timeout. But I'm not in Javascript land, so I'm not sure what the best way to proceed is...

banjerluke commented 2 years ago

On second thought, I'm not sure why my proposed solution would work in the first place. I'm getting tired, I think. 😂

Let's hold off on this one, maybe...

banjerluke commented 2 years ago

Unable to reproduce. Will revisit if I encounter another deadlock.

banjerluke commented 2 years ago

Damn, it's back. Looks like AudioPlayer.stop() is getting called on one thread just as AudioPlayer.internalCompletionHandler() is being called on another thread

banjerluke commented 2 years ago

Here's what I see; all collapsed threads (there are 41 threads in total) are identical to Thread 3.

image

banjerluke commented 2 years ago

Here's a way to bypass the whole issue.

It looks like the deadlock is happening because of the calls to engine in the completion handler (in AudioPlayer.swift:

    func internalCompletionHandler() {
        guard !isSeeking,
              isPlaying,
              engine?.isInManualRenderingMode == false else { return }

        scheduleTime = nil
        isPlaying = false
        completionHandler?()

        if !isBuffered, isLooping, engine?.isRunning == true {
            if !isEditTimeEnabled {
                editStartTime = 0
                editEndTime = 0
            }
            play()
            return
        }
    }

In our case, engine?.isInManualRenderingMode and isLooping will always be false, so we could shorten this function to:

    func internalCompletionHandler() {
        guard !isSeeking, isPlaying else { return }

        scheduleTime = nil
        isPlaying = false
        completionHandler?()
    }

And assuming completionHandler is thread-safe (I'll make sure it is), there should be no issues.

Not sure if there's a way to do this in the plugin or if we have to do it in the fork... but what do you think? It ain't pretty but it should get the job done...