From ddca32779ac471edcaf9f32a83633dcf3cb6ba1e Mon Sep 17 00:00:00 2001
From: Anthony Alfimov <Anthony.Alfimov@gmail.com>
Date: Fri, 11 Nov 2022 23:46:31 +0400
Subject: [PATCH] Fix data race in AudioFilePlayer

---
 Source/AudioFilePlayer.cpp | 39 ++++++++++++++++++++++----------------
 Source/AudioFilePlayer.h   | 11 ++++++-----
 2 files changed, 29 insertions(+), 21 deletions(-)

diff --git a/Source/AudioFilePlayer.cpp b/Source/AudioFilePlayer.cpp
index 41c2fa7..263e5a1 100644
--- a/Source/AudioFilePlayer.cpp
+++ b/Source/AudioFilePlayer.cpp
@@ -13,6 +13,11 @@
 AudioFilePlayer::AudioFilePlayer()
 {
     transportSource.addChangeListener (this);
+
+    //==========================================================================
+    // Check that atomic bool is lock-free
+    static_assert (std::atomic<bool>::is_always_lock_free,
+                   "std::atomic for type bool must be always lock free");
 }
 
 //==============================================================================
@@ -24,17 +29,18 @@ void AudioFilePlayer::setAudioFormatReader (AudioFormatReader* reader)
     // Pass reader ownership to newSource:
     auto newSource = std::make_unique<AudioFormatReaderSource> (reader, true);
 
-    // Set input source for the transportSource object:
-    transportSource.setSource (newSource.get(), 0, nullptr, reader->sampleRate);
+    {
+        SpinLock::ScopedLockType readerLoopingLock (readerLoopingMutex);
 
-    // Transfer memory ownership of the audio source to readerSource ptr:
-    readerSource.reset (newSource.release());
+        // Set input source for the transportSource object:
+        transportSource.setSource (newSource.get(), 0, nullptr, reader->sampleRate);
 
-    // Update looping status of the new readerSource:
-    readerSource->setLooping (looping);
+        // Transfer memory ownership of the audio source to readerSource ptr:
+        readerSource.reset (newSource.release());
 
-    // Stop transport:
-    changeState (TransportState::Stopped);
+        // Update transport state:
+        changeState (TransportState::Stopped);
+    }
 }
 
 //==========================================================================
@@ -45,10 +51,11 @@ void AudioFilePlayer::prepareToPlay (int samplesPerBlockExpected, double sampleR
 
 void AudioFilePlayer::getNextAudioBlock (const AudioSourceChannelInfo& info)
 {
-    if (readerSource == nullptr)
     {
-        info.clearActiveBufferRegion();
-        return;
+        SpinLock::ScopedTryLockType readerLoopingLock (readerLoopingMutex);
+
+        if (readerLoopingLock.isLocked() && readerSource != nullptr)
+            readerSource->setLooping (looping.load());
     }
 
     transportSource.getNextAudioBlock (info);
@@ -93,12 +100,12 @@ void AudioFilePlayer::stop()
         changeState (TransportState::Stopping);
 }
 
-void AudioFilePlayer::setLooping (bool shouldLoop)
+//==============================================================================
+double AudioFilePlayer::getCurrentPosition() const
 {
-    looping = shouldLoop;
-
-    if (readerSource.get() != nullptr)
-        readerSource->setLooping (looping);
+    // getCurrentPosition() method accesses readerSource, so we need to lock:
+    SpinLock::ScopedLockType readerLoopingLock (readerLoopingMutex);
+    return transportSource.getCurrentPosition();
 }
 
 //==============================================================================
diff --git a/Source/AudioFilePlayer.h b/Source/AudioFilePlayer.h
index f4c474e..78496ac 100644
--- a/Source/AudioFilePlayer.h
+++ b/Source/AudioFilePlayer.h
@@ -12,8 +12,7 @@
 
 #include <JuceHeader.h>
 
-// TODO: Pre-buffer the file that is playing?
-// TODO: Examine thread-safety of this class (use thread sanitiser?)
+// TODO: Pre-buffer the file that is playing
 
 class AudioFilePlayer  : public AudioSource,
                          public ChangeListener
@@ -35,13 +34,13 @@ class AudioFilePlayer  : public AudioSource,
     // Player transport controls
     void playPause();
     void stop();
-    void setLooping (bool shouldLoop);
+    void setLooping (bool shouldLoop) { looping.store (shouldLoop); }
 
     //==========================================================================
     // Player transport state
     bool isPlaying() const { return transportSource.isPlaying(); }
     bool isLooping() const { return transportSource.isLooping(); }
-    double getCurrentPosition() const { return transportSource.getCurrentPosition(); }
+    double getCurrentPosition() const;
 
     //==========================================================================
     // Transport state change callbacks
@@ -66,13 +65,15 @@ class AudioFilePlayer  : public AudioSource,
 
     void changeState (TransportState newState);
 
-    bool looping = false;
+    std::atomic<bool> looping = false;
     bool shouldFadeIn = true;
 
     std::unique_ptr<AudioFormatReaderSource> readerSource;
     AudioTransportSource transportSource;
     TransportState state = TransportState::Stopped;
 
+    SpinLock readerLoopingMutex;
+
     //==========================================================================
     // Change listener callback
     void changeListenerCallback (ChangeBroadcaster* source) override;