Exception safety in: locking of BlockFile for read

src/BlockFile.h

@@ -163,11 +163,43 @@ class PROFILE_DLL_API BlockFile /* not final, abstract */ {
    // not balanced by unlocking calls.
    virtual void CloseLock(){Lock();}
 
+ protected:
    /// Prevents a read on other threads.  The basic blockfile runs on only one thread, so does nothing.
    virtual void LockRead() const {}
    /// Allows reading on other threads.
    virtual void UnlockRead() const {}
 
+   struct ReadLocker { void operator () ( const BlockFile *p ) const {
+      if (p) p->LockRead(); } };
+   struct ReadUnlocker { void operator () ( const BlockFile *p ) const {
+      if (p) p->UnlockRead(); } };
+   using ReadLockBase =
+      movable_ptr_with_deleter< const BlockFile, ReadUnlocker >;
+
+ public:
+   class ReadLock : public ReadLockBase
+   {
+      friend BlockFile;
+      ReadLock ( const BlockFile *p, const BlockFile::ReadUnlocker &u )
+         : ReadLockBase { p, u } {}
+   public:
+#ifdef __AUDACITY_OLD_STD__
+      ReadLock (const ReadLock &that) : ReadLockBase( that ) {}
+      ReadLock &operator= (const ReadLock &that)
+      {
+         *((ReadLockBase*)this) = that;
+      }
+#endif
+      ReadLock(ReadLock&&that) : ReadLockBase{ std::move(that) } {}
+      using Suspension = std::unique_ptr< const BlockFile, ReadLocker >;
+      Suspension Suspend() const
+      { if (get()) get()->UnlockRead();
+        return Suspension{ get(), ReadLocker{} }; }
+   };
+
+   // RAII wrapper about the read locking functions
+   ReadLock LockForRead() const { LockRead(); return { this, ReadUnlocker{} }; }
+
  private:
 
  protected:

src/DirManager.cpp

@@ -1405,6 +1405,7 @@ bool DirManager::EnsureSafeFilename(const wxFileName &fName)
    bool needToRename = false;
    wxBusyCursor busy;
    BlockHash::iterator iter = mBlockFileHash.begin();
+   std::vector< BlockFile::ReadLock > readLocks;
    while (iter != mBlockFileHash.end())
    {
       BlockFilePtr b = iter->second.lock();
@@ -1420,14 +1421,14 @@ bool DirManager::EnsureSafeFilename(const wxFileName &fName)
             needToRename = true;
 
             //ODBlocks access the aliased file on another thread, so we need to pause them before this continues.
-            ab->LockRead();
+            readLocks.push_back( ab->LockForRead() );
          }
          //now for encoded OD blocks  (e.g. flac)
          else if (!b->IsDataAvailable() && db->GetEncodedAudioFilename() == fName) {
             needToRename = true;
 
             //ODBlocks access the aliased file on another thread, so we need to pause them before this continues.
-            db->LockRead();
+            readLocks.push_back( db->LockForRead() );
          }
       }
       ++iter;
@@ -1443,27 +1444,12 @@ bool DirManager::EnsureSafeFilename(const wxFileName &fName)
          // second earlier.)  But we'll handle this scenario
          // just in case!!!
 
-         // Put things back where they were
-         BlockHash::iterator iter = mBlockFileHash.begin();
-         while (iter != mBlockFileHash.end())
-         {
-            BlockFilePtr b = iter->second.lock();
-            if (b) {
-               auto ab = static_cast< AliasBlockFile * > ( &*b );
-               auto db = static_cast< ODDecodeBlockFile * > ( &*b );
-
-               if (b->IsAlias() && (ab->GetAliasedFileName() == fName))
-                  ab->UnlockRead();
-               if (!b->IsDataAvailable() && (db->GetEncodedAudioFilename() == fName))
-                  db->UnlockRead();
-            }
-            ++iter;
-         }
-
          // Print error message and cancel the export
          wxLogSysError(_("Unable to rename '%s' to '%s'."),
                        fullPath.c_str(),
                        renamedFullPath.c_str());
+
+         // Destruction of readLocks puts things back where they were
          return false;
       }
       else
@@ -1480,14 +1466,12 @@ bool DirManager::EnsureSafeFilename(const wxFileName &fName)
                if (b->IsAlias() && ab->GetAliasedFileName() == fName)
                {
                   ab->ChangeAliasedFileName(wxFileNameWrapper{ renamedFileName });
-                  ab->UnlockRead();
                   wxPrintf(_("Changed block %s to new alias name\n"),
                            b->GetFileName().name.GetFullName().c_str());
 
                }
                else if (!b->IsDataAvailable() && db->GetEncodedAudioFilename() == fName) {
                   db->ChangeAudioFile(wxFileNameWrapper{ renamedFileName });
-                  db->UnlockRead();
                }
             }
             ++iter;

src/blockfile/ODDecodeBlockFile.cpp

@@ -164,7 +164,7 @@ BlockFilePtr ODDecodeBlockFile::Copy(wxFileNameWrapper &&newFileName)
    BlockFilePtr newBlockFile;
 
    //mAliasedFile can change so we lock readdatamutex, which is responsible for it.
-   LockRead();
+   auto locker = LockForRead();
    if(IsSummaryAvailable())
    {
       //create a simpleblockfile, because once it has the summary it is a simpleblockfile for all intents an purposes
@@ -181,8 +181,6 @@ BlockFilePtr ODDecodeBlockFile::Copy(wxFileNameWrapper &&newFileName)
       //It can do this by checking for IsDataAvailable()==false.
    }
 
-   UnlockRead();
-
    return newBlockFile;
 }
 
@@ -194,7 +192,7 @@ BlockFilePtr ODDecodeBlockFile::Copy(wxFileNameWrapper &&newFileName)
 void ODDecodeBlockFile::SaveXML(XMLWriter &xmlFile)
 // may throw
 {
-   LockRead();
+   auto locker = LockForRead();
    if(IsSummaryAvailable())
    {
       SimpleBlockFile::SaveXML(xmlFile);
@@ -202,12 +200,12 @@ void ODDecodeBlockFile::SaveXML(XMLWriter &xmlFile)
    else
    {
       xmlFile.StartTag(wxT("oddecodeblockfile"));
+      {
          //unlock to prevent deadlock and resume lock after.
-      UnlockRead();
-      mFileNameMutex.Lock();
+         auto suspension = locker.Suspend();
+         ODLocker locker2{ &mFileNameMutex };
          xmlFile.WriteAttr(wxT("summaryfile"), mFileName.GetFullName());
-      mFileNameMutex.Unlock();
-      LockRead();
+      }
       xmlFile.WriteAttr(wxT("audiofile"), mAudioFileName.GetFullPath());
       xmlFile.WriteAttr(wxT("aliasstart"),
                         mAliasStart.as_long_long());
@@ -217,7 +215,6 @@ void ODDecodeBlockFile::SaveXML(XMLWriter &xmlFile)
 
       xmlFile.EndTag(wxT("oddecodeblockfile"));
    }
-   UnlockRead();
 }
 
 /// Constructs a ODDecodeBlockFile from the xml output of WriteXML.
@@ -435,17 +432,16 @@ size_t ODDecodeBlockFile::ReadData(samplePtr data, sampleFormat format,
                                 size_t start, size_t len) const
 {
    size_t ret;
-   LockRead();
+   auto locker = LockForRead();
    if(IsSummaryAvailable())
       ret = SimpleBlockFile::ReadData(data,format,start,len);
    else
    {
-      //we should do an ODRequest to start processing the data here, and wait till it finishes. and just do a SimpleBlockFIle
+      //we should do an ODRequest to start processing the data here, and wait till it finishes. and just do a SimpleBlockFile
       //ReadData.
       ClearSamples(data, format, 0, len);
       ret = len;
    }
-   UnlockRead();
    return ret;
 }
 

src/blockfile/ODPCMAliasBlockFile.cpp

@@ -196,7 +196,7 @@ BlockFilePtr ODPCMAliasBlockFile::Copy(wxFileNameWrapper &&newFileName)
    BlockFilePtr newBlockFile;
 
    //mAliasedFile can change so we lock readdatamutex, which is responsible for it.
-   LockRead();
+   auto locker = LockForRead();
    //If the file has been written AND it has been saved, we create a PCM alias blockfile because for
    //all intents and purposes, it is the same.
    //However, if it hasn't been saved yet, we shouldn't create one because the default behavior of the
@@ -218,8 +218,6 @@ BlockFilePtr ODPCMAliasBlockFile::Copy(wxFileNameWrapper &&newFileName)
       //The client code will need to schedule this blockfile for OD summarizing if it is going to a NEW track.
    }
 
-   UnlockRead();
-
    return newBlockFile;
 }
 
@@ -232,7 +230,7 @@ void ODPCMAliasBlockFile::SaveXML(XMLWriter &xmlFile)
 // may throw
 {
    //we lock this so that mAliasedFileName doesn't change.
-   LockRead();
+   auto locker = LockForRead();
    if(IsSummaryAvailable())
    {
       PCMAliasBlockFile::SaveXML(xmlFile);
@@ -243,11 +241,11 @@ void ODPCMAliasBlockFile::SaveXML(XMLWriter &xmlFile)
       xmlFile.StartTag(wxT("odpcmaliasblockfile"));
 
       //unlock to prevent deadlock and resume lock after.
-      UnlockRead();
-      mFileNameMutex.Lock();
+      {
+         auto suspension = locker.Suspend();
+         ODLocker locker2 { &mFileNameMutex };
          xmlFile.WriteAttr(wxT("summaryfile"), mFileName.GetFullName());
-      mFileNameMutex.Unlock();
-      LockRead();
+      }
 
       xmlFile.WriteAttr(wxT("aliasfile"), mAliasedFileName.GetFullPath());
       xmlFile.WriteAttr(wxT("aliasstart"),
@@ -257,8 +255,6 @@ void ODPCMAliasBlockFile::SaveXML(XMLWriter &xmlFile)
 
       xmlFile.EndTag(wxT("odpcmaliasblockfile"));
    }
-
-   UnlockRead();
 }
 
 /// Constructs a ODPCMAliasBlockFile from the xml output of WriteXML.
@@ -488,20 +484,16 @@ size_t ODPCMAliasBlockFile::ReadData(samplePtr data, sampleFormat format,
                                 size_t start, size_t len) const
 {
 
-   LockRead();
+   auto locker = LockForRead();
 
    if(!mAliasedFileName.IsOk()){ // intentionally silenced
       memset(data,0,SAMPLE_SIZE(format)*len);
-      UnlockRead();
       return len;
    }
 
-   auto result = CommonReadData(
+   return CommonReadData(
       mAliasedFileName, mSilentAliasLog, this, mAliasStart, mAliasChannel,
       data, format, start, len);
-
-   UnlockRead();
-   return result;
 }
 
 /// Read the summary of this alias block from disk.  Since the audio data

src/ondemand/ODDecodeTask.cpp

@@ -57,7 +57,8 @@ void ODDecodeTask::DoSomeInternal()
          //OD TODO: somehow pass the bf a reference to the decoder that manages its file.
          //we need to ensure that the filename won't change or be moved.  We do this by calling LockRead(),
          //which the dirmanager::EnsureSafeFilename also does.
-         bf->LockRead();
+         {
+            auto locker = bf->LockForRead();
             //Get the decoder.  If the file was moved, we need to create another one and init it.
             decoder = GetOrCreateMatchingFileDecoder( &*bf );
             if(!decoder->IsInitialized())
@@ -65,7 +66,7 @@ void ODDecodeTask::DoSomeInternal()
             bf->SetODFileDecoder(decoder);
             // Does not throw:
             ret = bf->DoWriteBlockFile();
-         bf->UnlockRead();
+         }
 
          if(ret >= 0) {
             success = true;