Add FinishTask to FairSource

[YanzhaoW]

Add an inheritance interface to FairSource such that certain actions can be taken in the end of the run.

Motivation

FairSource class may have multiple readers to analyze raw lmd data of each event (e.g. R3BUcesbSource in R3BRoot). Sometimes it's also necessary to perform certain actions in those readers at the end of the run, such as save histograms to the FileSink. Therefore, it's not suitable to do that in Close() method as some resources may be already closed.

The Reason why both Finish and FinishTask are needed: As stated in C++ coreguidelines:

Guideline #2: Prefer to make virtual functions private. This lets the derived classes override the function to customize the behavior as needed, without further exposing the virtual functions directly by making them callable by derived classes (as would be possible if the functions were just protected). The point is that virtual functions exist to allow customization; unless they also need to be invoked directly from within derived classes' code, there's no need to ever make them anything but private

Virtual member functions should be private unless in an interface base class. Therefore FinishTask should be private. The public interface of calling this virtual method is Finish.

---

Checklist:

• [x] Followed the Contributing Guidelines

[YanzhaoW]

@ChristianTackeGSI Something wrong in CI?

I got an error saying cmake version is too old in CI machine.

CMake Error at CMakeLists.txt:8 (cmake_minimum_required): CMake 3.18 or higher is required. You are running version 3.17.5

[ChristianTackeGSI]

@ChristianTackeGSI Something wrong in CI?

I got an error saying cmake version is too old in CI machine.

CMake Error at CMakeLists.txt:8 (cmake_minimum_required): CMake 3.18 or higher is required. You are running version 3.17.5

We have some issues with some of our CI container images. @dennisklein wanted to update them. Seems this hasn't happened yet.

Please just ignore them. If some succeed (especially the ubuntu-rolling one), then everything is fine for now.

[YanzhaoW]

Thanks very much for your comment.

The "at the end of the run" argument. If you want to do anything at the end of the run, what about deriving from FairTask a dedicated MyAtEndofRun class that overrides FairTask::Finish()

In my case, it is the readers in the FairSource that are doing the heavy-lifting list mode data analysis instead of FairTask. Actually no FairTask is needed when reading the list mode data. So in the end, all computation results are obtained in those readers along with histograms during the analysis.

As you intend to derive from FairSource anyways, you could alternatively override FairSource::Close() (and call the base class' Close at the end of your Close()). So if you intend to do something about the source just before it's getting closed, that could be your place.

So there is a problem that when those histograms should be written to file and whether the target files are available at that moment. My first attempt is of course writing histogram during FairSource::Close() to FairRootFileSink, which requires sink file to be ready for writing at that moment. But sometimes I got segmentation fault saying sink file is not available. But in the code of FairRunOnline, FairSource::Close() is actually be called before the FairSink::Close(). So my current guess is somehow FairSource::Close() is called twice, one before FairSink::Close and another after (We had lots of double deleting. So double closing could be very possible). The one after causes the segmentation fault.

And in my opinion, this is also a design issue because there is no logic reason why FairSource::Close() must be called before FairSink::Close() and we shouldn't impose this non-trivial restriction neither. Close() function should just be used to shutdown the status of each resources and the shutdown of one resource shouldn't be dependent on other resources. If we still need to use one resource, it should be done before any shutdown starts, guaranteeing the safety of the action.

Please tell me if I need more elaboration.

[ChristianTackeGSI]

Hi @YanzhaoW,

are you using FairLmdSource? If so, could you take a look at #1465?

[YanzhaoW]

Sure, I will check it out.

I don't know what's the best way to fix this currently. Closed function should always be used in a dtor with the correct ownership. Maybe a total re-structure?

[ChristianTackeGSI]

Many thanks for taking a look! :-)

We can't call Close directly from the destructor. Calling a virtual member function from a constructor doesn't do what you would expect. So each destructor should care for the member variables / resources of its class. That's why I invented this private CloseLmd, so that this code can be reused from the destructor and Close.

That said, yes, all of this needs another look. That's why I started with #1465.

[YanzhaoW]

Calling a virtual member function from a constructor doesn't do what you would expect.

You mean "destructor" instead? I don't quite understand here. If so, what kind of behaviour is unexpected?