Access BlockFile::mFileName without copying, with proper multithreading cautions

src/BlockFile.cpp

@@ -120,9 +120,9 @@ BlockFile::~BlockFile()
 /// but most BlockFiles have at least their summary data here.
 /// (some, i.e. SilentBlockFiles, do not correspond to a file on
 ///  disk and have empty file names)
-wxFileName BlockFile::GetFileName() const
+auto BlockFile::GetFileName() const -> GetFileNameResult
 {
-   return mFileName;
+   return { mFileName };
 }
 
 ///sets the file name the summary info will be saved in.  threadsafe.

src/BlockFile.h

@@ -24,6 +24,8 @@
 
 #include "wxFileNameWrapper.h"
 
+#include "ondemand/ODTaskThread.h"
+
 
 class SummaryInfo {
  public:
@@ -72,7 +74,24 @@ class PROFILE_DLL_API BlockFile /* not final, abstract */ {
    /// Gets the filename of the disk file associated with this BlockFile
    /// (can be empty -- some BlockFiles, like SilentBlockFile, correspond to
    ///  no file on disk)
-   virtual wxFileName GetFileName() const;
+   /// Avoids copying wxFileName by returning a reference, but for some subclasses
+   /// of BlockFile, you must exclude other threads from changing the name so long
+   /// as you have only a reference.  Thus, this wrapper object that guarantees release
+   /// of any lock when it goes out of scope.  Call mLocker.reset() to unlock it sooner.
+   struct GetFileNameResult {
+      const wxFileName &name;
+      ODLocker mLocker;
+
+      GetFileNameResult(const wxFileName &name_, ODLocker &&locker = ODLocker{})
+      : name{ name_ }, mLocker{ std::move(locker) } {}
+
+      GetFileNameResult(const GetFileNameResult&) PROHIBITED;
+      GetFileNameResult &operator= (const GetFileNameResult&) PROHIBITED;
+
+      GetFileNameResult(GetFileNameResult &&that)
+      : name{ that.name }, mLocker{ std::move(that.mLocker) } {}
+   };
+   virtual GetFileNameResult GetFileName() const;
    virtual void SetFileName(wxFileNameWrapper &&name);
 
    virtual sampleCount GetLength() const { return mLen; }

src/DirManager.cpp

@@ -941,7 +941,8 @@ bool DirManager::ContainsBlockFile(const BlockFile *b) const
 {
    if (!b)
       return false;
-   BlockHash::const_iterator it = mBlockFileHash.find(b->GetFileName().GetName());
+   auto result = b->GetFileName();
+   BlockHash::const_iterator it = mBlockFileHash.find(result.name.GetName());
    return it != mBlockFileHash.end() && it->second == b;
 }
 
@@ -957,7 +958,8 @@ bool DirManager::ContainsBlockFile(const wxString &filepath) const
 // the BlockFile.
 BlockFile *DirManager::CopyBlockFile(BlockFile *b)
 {
-   const auto &fn = b->GetFileName();
+   auto result = b->GetFileName();
+   const auto &fn = result.name;
 
    if (!b->IsLocked()) {
       b->Ref();
@@ -996,6 +998,9 @@ BlockFile *DirManager::CopyBlockFile(BlockFile *b)
             return NULL;
       }
 
+      // Done with fn
+      result.mLocker.reset();
+
       b2 = b->Copy(std::move(newFile));
 
       if (b2 == NULL)
@@ -1090,7 +1095,7 @@ bool DirManager::HandleXMLTag(const wxChar *tag, const wxChar **attrs)
    // return a reference to the existing object instead.
    //
 
-   wxString name = target->GetFileName().GetName();
+   wxString name = target->GetFileName().name.GetName();
    BlockFile *retrieved = mBlockFileHash[name];
    if (retrieved) {
       // Lock it in order to DELETE it safely, i.e. without having
@@ -1114,23 +1119,24 @@ bool DirManager::HandleXMLTag(const wxChar *tag, const wxChar **attrs)
 
 bool DirManager::MoveOrCopyToNewProjectDirectory(BlockFile *f, bool copy)
 {
-   const auto oldFileName = f->GetFileName();
+   auto result = f->GetFileName();
+   const auto &oldFileNameRef = result.name;
 
    // Check that this BlockFile corresponds to a file on disk
    //ANSWER-ME: Is this checking only for SilentBlockFiles, in which case
    //    (!oldFileName.IsOk()) is a more correct check?
-   if (oldFileName.GetName().IsEmpty()) {
+   if (oldFileNameRef.GetName().IsEmpty()) {
       return true;
    }
 
    wxFileNameWrapper newFileName;
-   if (!this->AssignFile(newFileName, oldFileName.GetFullName(), false))
+   if (!this->AssignFile(newFileName, oldFileNameRef.GetFullName(), false))
       return false;
 
-   if (newFileName != oldFileName) {
+   if (newFileName != oldFileNameRef) {
       //check to see that summary exists before we copy.
       bool summaryExisted = f->IsSummaryAvailable();
-      auto oldPath = oldFileName.GetFullPath();
+      auto oldPath = oldFileNameRef.GetFullPath();
       auto newPath = newFileName.GetFullPath();
       if (summaryExisted) {
          auto success = copy
@@ -1139,11 +1145,21 @@ bool DirManager::MoveOrCopyToNewProjectDirectory(BlockFile *f, bool copy)
          if (!success)
             return false;
       }
-      f->SetFileName(std::move(newFileName));
 
-      //there is a small chance that the summary has begun to be computed on a different thread with the
-      //original filename.  we need to catch this case by waiting for it to finish and then copy.
       if (!summaryExisted && (f->IsSummaryAvailable() || f->IsSummaryBeingComputed())) {
+
+         // We will need to remember the old file name, so copy it
+         wxFileName oldFileName{ oldFileNameRef };
+
+         // Now we can free any lock (and should, if as the comment below says, we need
+         // the other threads to progress)
+         result.mLocker.reset();
+
+         f->SetFileName(std::move(newFileName));
+
+         //there is a small chance that the summary has begun to be computed on a different thread with the
+         //original filename.  we need to catch this case by waiting for it to finish and then copy.
+
          //block to make sure OD files don't get written while we are changing file names.
          //(It is important that OD files set this lock while computing their summary files.)
          while(f->IsSummaryBeingComputed() && !f->IsSummaryAvailable())
@@ -1160,6 +1176,11 @@ bool DirManager::MoveOrCopyToNewProjectDirectory(BlockFile *f, bool copy)
                return false;
          }
       }
+      else {
+         // Can free this now, and must, because of nonrecursive mutexes
+         result.mLocker.reset();
+         f->SetFileName(std::move(newFileName));
+      }
    }
 
    return true;
@@ -1190,7 +1211,7 @@ int DirManager::GetRefCount(BlockFile * f)
 
 void DirManager::Deref(BlockFile * f)
 {
-   wxString theFileName = f->GetFileName().GetName();
+   const wxString theFileName = f->GetFileName().name.GetName();
 
    //printf("Deref(%d): %s\n",
    //       f->mRefCount-1,
@@ -1338,7 +1359,8 @@ bool DirManager::EnsureSafeFilename(const wxFileName &fName)
             {
                ab->ChangeAliasedFileName(wxFileNameWrapper{ renamedFileName });
                ab->UnlockRead();
-               wxPrintf(_("Changed block %s to new alias name\n"), b->GetFileName().GetFullName().c_str());
+               wxPrintf(_("Changed block %s to new alias name\n"),
+                        b->GetFileName().name.GetFullName().c_str());
 
             }
             else if (!b->IsDataAvailable() && db->GetEncodedAudioFilename() == fName) {

src/Sequence.cpp

@@ -987,7 +987,7 @@ void Sequence::HandleXMLEndTag(const wxChar *tag)
    for (unsigned b = 0, nn = mBlock.size(); b < nn;  b++) {
       SeqBlock &block = mBlock[b];
       if (block.start != numSamples) {
-         wxString sFileAndExtension = block.f->GetFileName().GetFullName();
+         wxString sFileAndExtension = block.f->GetFileName().name.GetFullName();
          if (sFileAndExtension.IsEmpty())
             sFileAndExtension = wxT("(replaced with silence)");
          else
@@ -1829,7 +1829,7 @@ void Sequence::DebugPrintf(wxString *dest) const
           seqBlock.f ? mDirManager->GetRefCount(seqBlock.f) : 0);
 
       if (seqBlock.f)
-         *dest += seqBlock.f->GetFileName().GetFullName();
+         *dest += seqBlock.f->GetFileName().name.GetFullName();
       else
          *dest += wxT("<missing block file>");
 

src/blockfile/ODDecodeBlockFile.cpp

@@ -369,13 +369,9 @@ void ODDecodeBlockFile::SetFileName(wxFileNameWrapper &&name)
 }
 
 ///sets the file name the summary info will be saved in.  threadsafe.
-wxFileName ODDecodeBlockFile::GetFileName() const
+auto ODDecodeBlockFile::GetFileName() const -> GetFileNameResult
 {
-   wxFileName name;
-   mFileNameMutex.Lock();
-   name = mFileName;
-   mFileNameMutex.Unlock();
-   return name;
+   return { mFileName, ODLocker{ &mFileNameMutex } };
 }
 
 /// A thread-safe version of CalcSummary.  BlockFile::CalcSummary

src/blockfile/ODDecodeBlockFile.h

@@ -133,7 +133,7 @@ class ODDecodeBlockFile final : public SimpleBlockFile
 
    ///sets the file name the summary info will be saved in.  threadsafe.
    void SetFileName(wxFileNameWrapper &&name) override;
-   wxFileName GetFileName() const override;
+   GetFileNameResult GetFileName() const override;
 
    /// Prevents a read on other threads of the encoded audio file.
    void LockRead() const override;

src/blockfile/ODPCMAliasBlockFile.cpp

@@ -381,13 +381,9 @@ void ODPCMAliasBlockFile::SetFileName(wxFileNameWrapper &&name)
 }
 
 ///sets the file name the summary info will be saved in.  threadsafe.
-wxFileName ODPCMAliasBlockFile::GetFileName() const
+auto ODPCMAliasBlockFile::GetFileName() const -> GetFileNameResult
 {
-   wxFileName name;
-   mFileNameMutex.Lock();
-   name = mFileName;
-   mFileNameMutex.Unlock();
-   return name;
+   return { mFileName, ODLocker{ &mFileNameMutex } };
 }
 
 /// Write the summary to disk, using the derived ReadData() to get the data

src/blockfile/ODPCMAliasBlockFile.h

@@ -125,7 +125,7 @@ class ODPCMAliasBlockFile final : public PCMAliasBlockFile
 
    ///sets the file name the summary info will be saved in.  threadsafe.
    void SetFileName(wxFileNameWrapper &&name) override;
-   wxFileName GetFileName() const override;
+   GetFileNameResult GetFileName() const override;
 
    //when the file closes, it locks the blockfiles, but only conditionally.
    // It calls this so we can check if it has been saved before.