From 21930e161dc7021ec0c6b323224cb0dc20ebe578 Mon Sep 17 00:00:00 2001 From: Christian Tacke <58549698+ChristianTackeGSI@users.noreply.github.com> Date: Mon, 27 May 2024 22:05:41 +0200 Subject: [PATCH] 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: https://github.com/FairRootGroup/FairRoot/issues/1418 (see "Double deleting of file source in FairRootManager and FairRun") --- fairroot/base/steer/FairRootManager.cxx | 11 +++------ fairroot/base/steer/FairRootManager.h | 30 ++++++++++--------------- fairroot/base/steer/FairRun.cxx | 24 +++++++++++--------- 3 files changed, 28 insertions(+), 37 deletions(-) diff --git a/fairroot/base/steer/FairRootManager.cxx b/fairroot/base/steer/FairRootManager.cxx index 71df4b4588..1236ea5030 100644 --- 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 index addffb804b..c440cbdd43 100644 --- 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 // for TObject #include // for TRefArray #include // for TString, operator< -#include // for map, multimap, etc +#include +#include // for map, multimap, etc #include #include #include // 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{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{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 fSource; //! TChain* fSourceChain = nullptr; std::map fSignalChainList; //! FairEventHeader* fEventHeader; - FairSink* fSink; + fairroot::detail::maybe_owning_ptr fSink; //! Bool_t fUseFairLinks; //! Bool_t fFinishRun; //! diff --git a/fairroot/base/steer/FairRun.cxx b/fairroot/base/steer/FairRun.cxx index acceeadebb..2d93d36939 100644 --- 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 // fot gROOT #include // 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,14 +190,14 @@ Bool_t FairRun::GetWriteRunInfoFile() void FairRun::SetSink(std::unique_ptr newsink) { fSink = std::move(newsink); - fRootManager->SetSink(fSink.get()); + fRootManager->fSink = maybe_owning_ptr{fSink.get(), non_owning}; fUserOutputFileName = fSink->GetFileName(); } void FairRun::SetSink(FairSink* tempSink) { fSink.reset(tempSink); - fRootManager->SetSink(fSink.get()); + fRootManager->fSink = maybe_owning_ptr{fSink.get(), non_owning}; fUserOutputFileName = fSink->GetFileName(); } @@ -201,8 +205,7 @@ void FairRun::SetOutputFile(const char* fname) { LOG(warning) << "FairRun::SetOutputFile() deprecated. Use FairRootFileSink."; fSink = std::make_unique(fname); - if (fRootManager) - fRootManager->SetSink(fSink.get()); + fRootManager->fSink = maybe_owning_ptr{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(name); - if (fRootManager) - fRootManager->SetSink(fSink.get()); + fRootManager->fSink = maybe_owning_ptr{fSink.get(), non_owning}; fUserOutputFileName = name; } @@ -256,7 +258,7 @@ void FairRun::AddAlignmentMatrices(const std::map& ali void FairRun::SetSource(FairSource* othersource) { - fRootManager->SetSource(othersource); + fRootManager->fSource = maybe_owning_ptr{othersource, non_owning}; fSource.reset(othersource); }