feat: Close Sink/Source in FairRun dtor

[ChristianTackeGSI]

If we destruct the sink/source during destruction of a FairRun (via unique_ptr), we could probably call the Close method of the sink and source as well.

The dtor of a sink/source should handle cleaning up anyway. But being nice and closing it could be considered "being good citizens".

See: #1462

---

Checklist:

• Followed the Contributing Guidelines

[YanzhaoW]

Hi, thanks for the PR.

Generally it's a good practise to crash the program if something goes terribly wrong.

[YanzhaoW]

In this case, we should just let program crash if fSource or fSink somehow is nullptr.

Suggested change

	if (fSource) {
	fSource->Close();
	}
	if (fSink) {
	fSink->Close();
	}
	fSource->Close();
	fSink->Close();

[ChristianTackeGSI]

Well, FairRunSim does not have a Source. So it doesn't need to close it.

[YanzhaoW]

I see. Yeah, that makes sense.

[YanzhaoW]

We also need to remove the closings from other places:

FairRoot/fairroot/online/steer/FairRunOnline.cxx

Lines 307 to 315 in 7104250

	void FairRunOnline::Finish()
	{
	fTask->FinishTask();
	fRootManager->LastFill();
	fRootManager->Write();
	GetSource()->Close();
	fRootManager->CloseSink();
	}

FairRoot/fairroot/base/steer/FairRootManager.h

Lines 66 to 71 in 7104250

	void CloseSink()
	{
	if (fSink) {
	fSink->Close();
	}
	}

[ChristianTackeGSI]

We also need to remove the closings from other places:

Before we remove those other places, I would like to have an idea, why this should be like this.

Calling Close multiple times on a FairSource/Sink should not hurt! (like it does not hurt for a std::fstream). So the Close in the dtor could be seen as a "if it wasn't closed until here, let's do it finally". And in the normal operation case it might be okay to close it earlier? I really don't know about that. So I would like to first keep it like it is. At least for this PR.

[YanzhaoW]

Calling Close multiple times on a FairSource/Sink should not hurt! (like it does not hurt for a std::fstream). So the Close in the dtor could be seen as a "if it wasn't closed until here, let's do it finally". And in the normal operation case it might be okay to close it earlier? I really don't know about that. So I would like to first keep it like it is. At least for this PR.

Yes, I agree. It's ideal that multiple closings of one resource shouldn't crash the program. But the problem here is that we are designing a library containing a base class (not a plain class like std::fstream), which other classes can be derived from and we basically have no clue what kind of implementations they would have. But in designing a library, we shouldn't close a resource multiple times by default. Some implementation may not allow that and this limitation is by no means trivial for the users of the library. This could also cause confusion as many users don't understand why their Close methods must be called twice.

For example, in the PR #1464, closing multiple times could crush the program because either sink has been closed or the histograms are already written.

[ChristianTackeGSI]

Well, we could enforce things like this

public:
  /**
   * \brief Close the resource, can be called multiple times.
   * Will call \ref Close
   */
  void CallClose()
  {
    if (IsOpen()) {
      Close();
    }
    fOpen = false;
  }

private:
  /**
   * derived classes might know better, when they're "open"
   */
  virtual IsOpen() const { return fOpen; }

  bool fOpen{false};
protected:
  /**
   * You must call this at the end of your open function, so that Close is called at all, but not twice
   */
  void SetAsOpen() { fOpen = true; }

To be honest: This looks like a lot of overhead.
And really? Is it better to ask derived classes to use SetAsOpen();? Otherwise we can't even know the thing was opened at all.

[YanzhaoW]

To be honest: This looks like a lot of overhead.
And really? Is it better to ask derived classes to use SetAsOpen();? Otherwise we can't even know the thing was opened at all.

Yeah, that's a bit over-complication. But I still don't quite understand why we need to call the Close function twice from the library side. If the simplicity is preferred, I would say it's enough to call it once for all and the second call should be removed.

Nevertheless, I reconsidered the Close strategy on the source and sink. If we follow the rule that "whoever owns it should closes it", then it's probably better not to have this Close function at all. For example, for the base class FairSource, it actually owns nothing. A valid resource is actually owned by its derived class, in such case FairFileSource. The actual TFile object is actually owned by FairFileSource instead of FairSource. Therefore, it's the responsibility for FairFileSource to close TFile handler in its dtor, not FairSource. Therefore, a Close function is actually not needed in such case and users should implement the closing of their resources by themselves in their corresponding derived classes.

[ChristianTackeGSI]

Yeah, I also considered the "Do we need Close at all" thing.

Or rather: What is the idea behind this Close thing at all?

Looking at std::fstream, I guess the idea is to be able to keep the object still (for example, because it is a valued member variable) but still release the resources earlier. So if FairSource/Sink are meant to be of general usefullness (outside the FairRun machinery), then maybe it would be good to be able to release the resources when one is done using it?

But I still don't quite understand why we need to call the Close function twice from the library side.

I think, we don't have to. But we can't (easily) guarantee, that we call it exactly once in every possible scenario.

• If we only call it in the FairRun dtor, you're plain right: We could just let the FairSource/Sink dtor handle stuff and be done.
• If we call it at the end of FairRun*::Run(), we might consider it a "notification, that the Run is over". But we can't guarantee, that Close it called always. Maybe people instanatiate FairRun* but forget to call Run()?

All in all, I don't really know, what would be best.

Some small bits I know: As long, as we offer a public Close API on FairSource/Sink, we should make sure that all our sinks/sources implement it nicely (and for example allow calling it multiple times). Because people could use the sources/sinks somewhere else and rely on Close shutting down resources (without destoying the entire sink/source). That's why I wanted #1465 to happen like it did.

[YanzhaoW]

I think std::fstream is not a good example to follow in our case as it's a non-abstract class instead of an abstract class that we are dealing with. Even so, there is a note of std::fstream::Close in the cppreference

This function is called by the destructor of basic_fstream when the stream object goes out of scope and is not usually invoked directly.

So my recommendation is to deprecate the Close method from FairSource and FairSink and let users close their own resources in the dtor of the classes derived from FairSource/FairSink. If they still want to have a public API for the close, they could create the close function themselves. In this case, the library doesn't need to worry about how many times the close function is called. Such concern can also be decided by users as well.

[ChristianTackeGSI]

So my recommendation is to deprecate the Close method from FairSource and FairSink and let users close their own resources in the dtor of the classes derived from FairSource/FairSink. If they still want to have a public API for the close, they could create the close function themselves. In this case, the library doesn't need to worry about how many times the close function is called. Such concern can also be decided by users as well.

Hmmm!

I could ask you, whether you would be willing to create a PR for this. But I don't know, what the idea of the main FairRoot developers is for all of this.

@karabowi @fuhlig1 What do you think? Or should we put this on the next Monday Meeting?

[YanzhaoW]

Hi

I could ask you, whether you would be willing to create a PR for this. But I don't know, what the idea of the main FairRoot developers is for all of this.

Sure, I could do a PR if needed. :D

[karabowi]

After some tests and discussion with Christian, in my opinion the Close() functions of Sampler and Sink may be deprecated.

[ChristianTackeGSI]

@YanzhaoW, if you want to go ahead, you might create a PR to deprecate Fair{Source,Sink}::Close.

I guess, we should keep the current caller's as they are (you need to put some pragma push/pop around them).