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:

• 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.

[ChristianTackeGSI]

I am not against this. (And I like virtual functions being private!). Having hooks for things is great.

That said, I have two issues with this:

1. 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()?

   All the places that call GetSource()->Finish(); are preceded by fTask->FinishTask(); (which calls FairTask::Finish()). So it should be the right place for your "end of the run" idea?

   And if you want to do something "run specific", it should not be hooked to the Source. A FairSource::Finish should do something specific to the source.

2. 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.

   (Maybe we're missing a GetSource->Close(); in FairRunAna? If so, we should probably add it?)

Can you please elaborate on your use case and why it would be better to have a Finish(Task) hook on the FairSource?

[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]

Just one thing ahead: I am relatively new to FairRoot and still learning. So if I miss something, just let me know!

I think, we should have a deeper look into how Close is called for the Source.

We have a suspected double Close on the one side. And even a missed close on the other side.

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.

Could you have a look, if this really happens? Maybe add a LOG(DEBUG) to your Close? (would be really interesting to learn how this happens!)

I found one suspicious Close():

FairRoot/fairroot/online/source/FairLmdSource.cxx

Line 169 in 7104250

Close();

[ChristianTackeGSI]

And this one is the one where I wonder, whether Close is called at all.

This is in FairRunAna::TerminateRun. And the only place I could find that calls into TerminateRun is in FairAnaSelector, which is only used in FairRunAnaProof. So it looks like this is never called. I again wonder, where GetSource()->Close() gets really called?

[YanzhaoW]

The TerminateRun is never called (see this issue #1462)

GetSource()->Close() is only called in FairRunOnline::Finish(), which is called in FairRun::Run(). It won't be called neither if Run() is skipped.

[ChristianTackeGSI]

Let's take a deeper look at #1462 first, please!

[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?