Deadlock in Pause()

[vers-one]

Here's the minimal reproduction:

public partial class MainWindow : Window
{
    private readonly Player player;

    public MainWindow()
    {
        InitializeComponent();
        Engine.Start();
        Config config = new();
        config.Player.AutoPlay = false;
        player = new(config);
        player.OpenCompleted += Player_OpenCompleted;
        player.PlaybackStopped += Player_PlaybackStopped;
        player.OpenAsync("test.mp3");
    }

    private void Player_OpenCompleted(object? sender, OpenCompletedArgs e)
    {
        UpdatePlaybackButtons();
    }

    private void Player_PlaybackStopped(object? sender, PlaybackStoppedArgs e)
    {
        UpdatePlaybackButtons();
    }

    private void playButton_Click(object sender, RoutedEventArgs e)
    {
        player.Play();
        UpdatePlaybackButtons();
    }

    private void pauseButton_Click(object sender, RoutedEventArgs e)
    {
        player.Pause();
        UpdatePlaybackButtons();
    }

    private void UpdatePlaybackButtons()
    {
        Action action = () =>
        {
            playButton.IsEnabled = player.Status == Status.Paused || player.Status == Status.Ended;
            pauseButton.IsEnabled = player.Status == Status.Playing;
        };
        if (Application.Current.Dispatcher.Thread == Thread.CurrentThread)
        {
            action();
        }
        else
        {
            Application.Current.Dispatcher.Invoke(action);
        }
    }
}

Once you click the pause button, the player gets stuck in an infinite loop here:

// FlyleafLib/MediaPlayer/Player.Playback.cs
public void Pause()
{
    lock (lockActions)
    {
        if (!CanPlay || Status == Status.Ended)
            return;

        status = Status.Paused;
        UI(() => Status = Status);

        while (taskPlayRuns) Thread.Sleep(5); // <-- HERE
    }
}

A workaround is to replace Dispatcher.Invoke with Dispatcher.BeginInvoke, but this feels like an unnecessary requirement put onto the consumers of the library. One of the better approaches could be to execute the PlaybackStopped event handlers from within the Pause() method rather than from the internal playback thread.

As a side note, the amount of Thread.Sleeps in the library's code is a little bit concerning.

[coldays]

The PlaybackStopped event has a warning in it's summary: /// Warning: Uses Invoke and it comes from playback thread so you can't pause/stop etc. You need to use another thread if you have to. Maybe it should say that it uses the main thread / GUI thread and maybe pause should warn that is uses the GUI thread as well. Other than that, it looks like this deadlock is created by the example itself.

If you don't like to use BeginInvoke you can also call Pause from another Task. A better "WPF" way to handle this is to create bindings to the player state directly instead of manually setting IsEnabled on the button.

[umlx5h]

As a side note, the amount of Thread.Sleeps in the library's code is a little bit concerning.

Probably BlockingCollection<T>, Channels instead of ConcurrentQueue<T> can be used instead or use ConcurrentQueue<T> with Monitor.Wait, Pulse.

Sorry this has nothing to do with the issue, I am also wondering why Thread.Sleep is used so much.

[SuRGeoNix]

I do agree with you guys, that's the main reason of focusing in the next major version. New threading, locking mechanisms with custom blocking collections using pulse/wait.

However, this is not related with this issue, this has to do with the library being also a view model and if you don't know your events in which thread are running you probably going to mess up. In your case Pause will call also your registered event PlaybackStopped so you could just unsubscribe it when you manually pause and then subscribe again.