fix: Handle Sink/Source ownership more properly

FairRootManager and FairRun both used to `delete` fSink and fSource. In specific situations (FairRootManager getting destructed before FairRun) this leads to a double delete! Deprecate the old FairRootManager::Set{Source,Sink} APIs that take something like an owning pointer. FairRootManager should only have a non-owning reference to sink and source. To allow FairRun to set it, `friend` it from FairRootManager. See: #1418 (see "Double deleting of file source in FairRootManager and FairRun")
FairRootGroup · May 27, 2024 · 21930e1 · 21930e1
1 parent 3385d5e
commit 21930e1
Show file tree

Hide file tree

Showing 3 changed files with 28 additions and 37 deletions.
diff --git a/fairroot/base/steer/FairRootManager.cxx b/fairroot/base/steer/FairRootManager.cxx
@@ -1,5 +1,5 @@
 /********************************************************************************
- * Copyright (C) 2014-2023 GSI Helmholtzzentrum fuer Schwerionenforschung GmbH  *
+ * Copyright (C) 2014-2024 GSI Helmholtzzentrum fuer Schwerionenforschung GmbH  *
  *                                                                              *
  *              This software is distributed under the terms of the             *
  *              GNU Lesser General Public Licence (LGPL) version 3,             *
@@ -84,10 +84,8 @@ FairRootManager::FairRootManager()
     , fBrPerMap()
     , fFillLastData(kFALSE)
     , fEntryNr(0)
-    , fSource(0)
     , fSignalChainList()
     , fEventHeader(new FairEventHeader())
-    , fSink(nullptr)
     , fUseFairLinks(kFALSE)
     , fFinishRun(kFALSE)
 {
@@ -105,10 +103,6 @@ FairRootManager::~FairRootManager()
 
     delete fEventHeader;
     delete fSourceChain;
-    if (fSink)
-        delete fSink;
-    if (fSource)
-        delete fSource;
 
     // Global cleanup
 
@@ -388,6 +382,7 @@ void FairRootManager::WriteFolder()
         fSink->WriteObject(fTimeBasedBranchNameList, "TimeBasedBranchList", TObject::kSingleKey);
     }
 }
+
 Bool_t FairRootManager::SpecifyRunId()
 {
     if (!fSource) {
@@ -892,7 +887,7 @@ std::string FairRootManager::GetNameFromFile(const char* namekind)
 {
     std::string default_name = "";
 
-    if (strcmp(namekind, "treename=") && strcmp(namekind, "foldername=")) {
+    if ((strcmp(namekind, "treename=") != 0) && (strcmp(namekind, "foldername=") != 0)) {
         LOG(fatal) << "FairRootManager, requested " << namekind << ", while only treename= and foldername= available.";
         return default_name;
     }

diff --git a/fairroot/base/steer/FairRootManager.h b/fairroot/base/steer/FairRootManager.h
@@ -1,5 +1,5 @@
 /********************************************************************************
- * Copyright (C) 2014-2023 GSI Helmholtzzentrum fuer Schwerionenforschung GmbH  *
+ * Copyright (C) 2014-2024 GSI Helmholtzzentrum fuer Schwerionenforschung GmbH  *
  *                                                                              *
  *              This software is distributed under the terms of the             *
  *              GNU Lesser General Public Licence (LGPL) version 3,             *
@@ -8,7 +8,7 @@
 #ifndef FAIR_ROOT_MANAGER_H
 #define FAIR_ROOT_MANAGER_H
 
-#include "FairLogger.h"
+#include "FairMemory.h"
 #include "FairSink.h"
 #include "FairSource.h"
 
@@ -17,7 +17,8 @@
 #include <TObject.h>     // for TObject
 #include <TRefArray.h>   // for TRefArray
 #include <TString.h>     // for TString, operator<
-#include <map>           // for map, multimap, etc
+#include <fairlogger/Logger.h>
+#include <map>   // for map, multimap, etc
 #include <memory>
 #include <string>
 #include <type_traits>   // is_pointer, remove_pointer, is_const, remove...
@@ -49,6 +50,8 @@ class TTree;
 
 class FairRootManager : public TObject
 {
+    friend class FairRun;
+
   public:
     /**dtor*/
     ~FairRootManager() override;
@@ -218,21 +221,12 @@ class FairRootManager : public TObject
     void SetUseFairLinks(Bool_t val) { fUseFairLinks = val; };
     Bool_t GetUseFairLinks() const { return fUseFairLinks; };
 
-    /**
-     * @param Status : if  true all inputs are mixed, i.e: each read event will take one entry from each input and put
-     * them in one big event and send it to the next step
-     */
-    /* void SetMixAllInputs(Bool_t Status) { */
-    /*    fMixAllInputs=kTRUE; */
-    /* } */
-
-    /** These methods have been moved to the FairFileSource */
-    void SetSource(FairSource* tempSource) { fSource = tempSource; }
-    FairSource* GetSource() { return fSource; }
+    [[deprecated]] void SetSource(FairSource* source) { fSource = std::unique_ptr<FairSource>{source}; }
+    FairSource* GetSource() { return fSource.get(); }
     Bool_t InitSource();
 
-    void SetSink(FairSink* tempSink) { fSink = tempSink; }
-    FairSink* GetSink() { return fSink; }
+    [[deprecated]] void SetSink(FairSink* sink) { fSink = std::unique_ptr<FairSink>{sink}; }
+    FairSink* GetSink() { return fSink.get(); }
     Bool_t InitSink();
 
     void SetListOfFolders(TObjArray* ta) { fListFolder = ta; }
@@ -383,14 +377,14 @@ class FairRootManager : public TObject
 
     TObjArray* fListFolder{nullptr};   //!
 
-    FairSource* fSource;
+    fairroot::detail::maybe_owning_ptr<FairSource> fSource;   //!
 
     TChain* fSourceChain = nullptr;
     std::map<UInt_t, TChain*> fSignalChainList;   //!
 
     FairEventHeader* fEventHeader;
 
-    FairSink* fSink;
+    fairroot::detail::maybe_owning_ptr<FairSink> fSink;   //!
 
     Bool_t fUseFairLinks;   //!
     Bool_t fFinishRun;      //!

diff --git a/fairroot/base/steer/FairRun.cxx b/fairroot/base/steer/FairRun.cxx
@@ -1,5 +1,5 @@
 /********************************************************************************
- * Copyright (C) 2014-2023 GSI Helmholtzzentrum fuer Schwerionenforschung GmbH  *
+ * Copyright (C) 2014-2024 GSI Helmholtzzentrum fuer Schwerionenforschung GmbH  *
  *                                                                              *
  *              This software is distributed under the terms of the             *
  *              GNU Lesser General Public Licence (LGPL) version 3,             *
@@ -30,6 +30,9 @@
 #include <TROOT.h>     // fot gROOT
 #include <cassert>     // for... well, assert
 
+using fairroot::detail::maybe_owning_ptr;
+using fairroot::detail::non_owning;
+
 TMCThreadLocal FairRun* FairRun::fRunInstance = nullptr;
 
 FairRun* FairRun::Instance() { return fRunInstance; }
@@ -82,9 +85,10 @@ FairRun::~FairRun()
 {
     LOG(debug) << "Enter Destructor of FairRun";
 
-    // So that FairRootManager does not try to delete these, because we will do that:
-    fRootManager->SetSource(nullptr);
-    fRootManager->SetSink(nullptr);
+    // So that FairRootManager does not have a dangling reference
+    // to these. Our unique_ptr will destruct them.
+    fRootManager->fSource.reset();
+    fRootManager->fSink.reset();
 
     if (fTask) {
         // FairRunAna added it, but let's remove it here, because we own it
@@ -186,23 +190,22 @@ Bool_t FairRun::GetWriteRunInfoFile()
 void FairRun::SetSink(std::unique_ptr<FairSink> newsink)
 {
     fSink = std::move(newsink);
-    fRootManager->SetSink(fSink.get());
+    fRootManager->fSink = maybe_owning_ptr<FairSink>{fSink.get(), non_owning};
     fUserOutputFileName = fSink->GetFileName();
 }
 
 void FairRun::SetSink(FairSink* tempSink)
 {
     fSink.reset(tempSink);
-    fRootManager->SetSink(fSink.get());
+    fRootManager->fSink = maybe_owning_ptr<FairSink>{fSink.get(), non_owning};
     fUserOutputFileName = fSink->GetFileName();
 }
 
 void FairRun::SetOutputFile(const char* fname)
 {
     LOG(warning) << "FairRun::SetOutputFile() deprecated. Use FairRootFileSink.";
     fSink = std::make_unique<FairRootFileSink>(fname);
-    if (fRootManager)
-        fRootManager->SetSink(fSink.get());
+    fRootManager->fSink = maybe_owning_ptr<FairSink>{fSink.get(), non_owning};
     fUserOutputFileName = fname;
 }
 
@@ -216,8 +219,7 @@ void FairRun::SetOutputFileName(const TString& name)
 {
     LOG(warning) << "FairRun::SetOutputFileName() deprecated. Use FairRootFileSink.";
     fSink = std::make_unique<FairRootFileSink>(name);
-    if (fRootManager)
-        fRootManager->SetSink(fSink.get());
+    fRootManager->fSink = maybe_owning_ptr<FairSink>{fSink.get(), non_owning};
     fUserOutputFileName = name;
 }
 
@@ -256,7 +258,7 @@ void FairRun::AddAlignmentMatrices(const std::map<std::string, TGeoHMatrix>& ali
 
 void FairRun::SetSource(FairSource* othersource)
 {
-    fRootManager->SetSource(othersource);
+    fRootManager->fSource = maybe_owning_ptr<FairSource>{othersource, non_owning};
     fSource.reset(othersource);
 }