chore: Improve closing on FairLmdSource #1465

ChristianTackeGSI · 2023-11-20T11:06:25Z

Close is a virtual method. It's getting called internally in a scenario where only FairLmdSource specific resources should be closed.

Also the desctructor did not close.

This was triggered by a comment here: #1464 (review)

Checklist:

Followed the Contributing Guidelines

`Close` is a virtual method. It's getting called internally in a scenario where only FairLmdSource specific resources should be closed. Also the destructor did not close.

YanzhaoW · 2023-11-20T14:22:44Z

Hi, thanks for the PR.

There are few problems:

The overridden Closed function from Fairlmd should be called from its base class (dtor of bass class) instead of itself.
The overridden Closed function should be a private member function. I don't understand why you have private Closelmd and a public Close instead of just making Close private.

ChristianTackeGSI · 2023-11-20T18:46:35Z

The overridden Closed function from Fairlmd should be called from its base class (dtor of bass class) instead of itself.

That does not work.

Inside the base class destructor the object is of type base-class. So any calls to any virtual functions in the dtor go to the virtual methods of that base class (or its parents).

That's basically, why calling virtual functions in the dtor / ctor (there it's the same story) is a bad idea, because it does not do what you would think it should do.

There's even a clang-tidy warning for this: clang-analyzer-optin.cplusplus.VirtualCall. And yes, we have enabled this one in our CI. And yes, we should fix them…

The overridden Closed function should be a private member function. I don't understand why you have private Closelmd and a public Close instead of just making Close private.

Close is a public API of FairSource. So if we want to make it private, we should declare that a breaking change.

And IMHO it is okay for a FairSource to have Open and Close functions. The Close one can be virtual (yes, it would be nicer, if the virtual functions would be private/protected and the public interface would be another member functions, but we don't have that yet), so that FairRun can close the source when it's done, but doesn't have to destruct it yet.

So the idea is:

Each class can have a private (non-virtual!) helper function to cleanup resources / close the resources.
Call that helper from the dtor.
Override Close, call your helper
And call the parent class' Close (if the parent class has one).
(So yes, we probably should give the base class an empty "default" Close.)

YanzhaoW · 2023-11-20T21:55:50Z

Inside the base class destructor the object is of type base-class. So any calls to any virtual functions in the dtor go to the virtual methods of that base class (or its parents).

You are right! I always thought calling a virtual function in dtor is same as in any other member functions. Glad I learned this new thing.

And IMHO it is okay for a FairSource to have Open and Close functions. The Close one can be virtual (yes, it would be nicer, if the virtual functions would be private/protected and the public interface would be another member functions, but we don't have that yet), so that FairRun can close the source when it's done, but doesn't have to destruct it yet.

Even though we can't call the Close in FairSource dtor, we could still call the Fair::Close in the dtor of its owner, as is also a better place to call it. As is a golden rule for the ownership, whoever owns it is responsible for how it's disposed (delete, close, log ext.).

So in my opinion, what's the correct way is to have a public non-virtual close member function in the base class (in our case, FairSource::Close()) and also another virtual private member function, for example, Close_imp(), which is called in Close. Then in the derived class, we only override this private member function Close_imp() (without overloading public method Close()). If we decides FairSource is owned by FairRun (I'm still not sure whether is FairRun or FairRooManager), in the dtor of FairRun, we call the public method FairSource::Close, which actually executes the Close_imp() from the true type of the "source" object.

A very simple illustration.

YanzhaoW

Following my comment above.

The benefit of this way is that the close action is automatically handled by the base class. The developer who implements a new derived class just needs to override the Close_imp() without concerning where it should be called. And we have only one line of Close for all derived classes.

YanzhaoW · 2023-11-20T21:57:16Z

fairroot/online/source/FairLmdSource.cxx

@@ -54,6 +54,7 @@ FairLmdSource::FairLmdSource(const FairLmdSource& source)

 FairLmdSource::~FairLmdSource()
 {
+    CloseLmd();


This step could be executed by its owner.

Well, no.

I don't think, the "owner" should be resposible for calling Close. It could be considered "being a good citizen", but it should not be strictly needed.

FairLmdSource mysource; mysource.Open(…); if (true) throw std::runtime_exception("Something else went wrong"); mysource.Close();

This should not leak resources! (See "RAII printciple").

So yes, This is some overhead for the developer of a FairSource/Sink. But that's how C++ is like.

YanzhaoW · 2023-11-20T21:58:47Z

fairroot/online/source/FairLmdSource.cxx

 void FairLmdSource::Close()
 {
+    CloseLmd();
+}


This could be implemented in the base class FairSource:

void FairSource::Close() {Close_imp();}

Can we postpone this for another PR?

YanzhaoW · 2023-11-20T21:59:31Z

fairroot/online/source/FairLmdSource.h

@@ -65,6 +65,9 @@ class FairLmdSource : public FairMbsSource

    FairLmdSource& operator=(const FairLmdSource&);

+  private:
+    void CloseLmd();


This could be an overridden private virtual function.

Let's please postpone this for another PR as well.

kresan

Much cleaner closing now. Looks good.

ChristianTackeGSI · 2023-11-27T10:21:36Z

Should we backport this to 18.8 (or even 18.6)?

ChristianTackeGSI requested a review from kresan November 20, 2023 12:21

ChristianTackeGSI mentioned this pull request Nov 20, 2023

Add FinishTask to FairSource #1464

Open

1 task

ChristianTackeGSI force-pushed the pr/lmd branch from ebf8600 to 9f7501b Compare November 20, 2023 13:16

chore: Improve closing on FairLmdSource

46b14b6

`Close` is a virtual method. It's getting called internally in a scenario where only FairLmdSource specific resources should be closed. Also the destructor did not close.

ChristianTackeGSI force-pushed the pr/lmd branch from 9f7501b to 46b14b6 Compare November 20, 2023 13:18

YanzhaoW reviewed Nov 20, 2023

View reviewed changes

kresan approved these changes Nov 24, 2023

View reviewed changes

ChristianTackeGSI merged commit 46b14b6 into FairRootGroup:dev Nov 27, 2023

ChristianTackeGSI deleted the pr/lmd branch November 27, 2023 10:21

ChristianTackeGSI mentioned this pull request Nov 27, 2023

feat: Close Sink/Source in FairRun dtor #1467

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Improve closing on FairLmdSource #1465

chore: Improve closing on FairLmdSource #1465

ChristianTackeGSI commented Nov 20, 2023

YanzhaoW commented Nov 20, 2023

ChristianTackeGSI commented Nov 20, 2023

YanzhaoW commented Nov 20, 2023 •

edited

Loading

YanzhaoW left a comment •

edited

Loading

YanzhaoW Nov 20, 2023 •

edited

Loading

ChristianTackeGSI Nov 21, 2023

YanzhaoW Nov 20, 2023 •

edited

Loading

ChristianTackeGSI Nov 21, 2023

YanzhaoW Nov 20, 2023 •

edited

Loading

ChristianTackeGSI Nov 21, 2023

kresan left a comment

ChristianTackeGSI commented Nov 27, 2023 •

edited

Loading

chore: Improve closing on FairLmdSource #1465

chore: Improve closing on FairLmdSource #1465

Conversation

ChristianTackeGSI commented Nov 20, 2023

YanzhaoW commented Nov 20, 2023

ChristianTackeGSI commented Nov 20, 2023

YanzhaoW commented Nov 20, 2023 • edited Loading

YanzhaoW left a comment • edited Loading

Choose a reason for hiding this comment

YanzhaoW Nov 20, 2023 • edited Loading

Choose a reason for hiding this comment

ChristianTackeGSI Nov 21, 2023

Choose a reason for hiding this comment

YanzhaoW Nov 20, 2023 • edited Loading

Choose a reason for hiding this comment

ChristianTackeGSI Nov 21, 2023

Choose a reason for hiding this comment

YanzhaoW Nov 20, 2023 • edited Loading

Choose a reason for hiding this comment

ChristianTackeGSI Nov 21, 2023

Choose a reason for hiding this comment

kresan left a comment

Choose a reason for hiding this comment

ChristianTackeGSI commented Nov 27, 2023 • edited Loading

YanzhaoW commented Nov 20, 2023 •

edited

Loading

YanzhaoW left a comment •

edited

Loading

YanzhaoW Nov 20, 2023 •

edited

Loading

YanzhaoW Nov 20, 2023 •

edited

Loading

YanzhaoW Nov 20, 2023 •

edited

Loading

ChristianTackeGSI commented Nov 27, 2023 •

edited

Loading