Remove calling Close method in FairRootManager/FairRun #1468
Conversation
|
a sidenote: |
|
I have tested FairRoot's private branch without FairSampler::Close() and FairSink::Close() |
Deprecations in GeneralFor me a deprecation means, that
In FairRoot: Public APILet's look at (1): Public API is basically everything in When it comes to FairRoot: The confusing Close situationIn a cool world, the old situation would look like this: class FairSourceSink {
public:
void CloseForUsers() { CloseImpl(); }
private:
// Please override in your implementation
virtual void CloseImpl() = 0;
private:
// So that the library can call private member functions
friend FairRun;
friend FairRootManager;
// Will be called internally by the library as a "call back like thing"
void CloseForInternalUse() { CloseImpl(); }
};Let's go step by step.
That's my current idea on how this should look like. The problem is now, that all of those three ( To maybe simplify things, we could temporarily invent: private
void CloseForInternalUse() {
// All of these pragmas, because we would call
// CloseImpl (which is not deprecated internally),
// but we have to call Close, which is deprecated
// from (1).
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
Close();
#pragma GCC diagnostic pop
}That's the whole problem here from my point of view. |
|
Thanks for the replies. Sorry I'm very busy working on other things and don't have time to deal with this PR these days. But probably on this Friday I will read the replies in details and continue to fix the things here if needed. |
|
Hi, I thought it over again. Yes, the procedures described above is probably the right way to go. But there is still something weird that I couldn't pinpoint what exactly it is in the beginning, especially when I see: private
void CloseForInternalUse() {
// All of these pragmas, because we would call
// CloseImpl (which is not deprecated internally),
// but we have to call Close, which is deprecated
// from (1).
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
Close();
#pragma GCC diagnostic pop
}For me, we are kind of contradicting ourself when we enable a warning from a new deprecation but at the same time we also disable it. I guess the people who designed the "deprecation" in C++ also didn't expect the people to disable it at the same time using a compiler-specific macro (There are some FairRoot users using apple-clang). Our main dispute is whether we should remove the calling of the void CloseSink()
{
if (fSink) {
fSink->Close();
}
}Here are my arguments why it should just be removed based on the following principles:
The point 2 and 3 are supported by C++ guidelines from Herb Sutter about why virtual functions should be private. In this articles, it says
It also implies here that the derived classes which overrides the virtual functions are just customization. For example, if we are designing an abstract class for a person wearing a T-shirt and pants. It's the job of the derived classes (User's classes) to decide what kind of T-shirt and pants are (e.g colors and sizes). But the logic connection between those objects are determined by the abstract class (the library implementer). Users shouldn't be concerned about in which order they are put up or whether they are put up or not. Suppose if we want to let the person wear T-shirt first instead of pants first, we could just change that order without doing any deprecation of the old order. Deprecation is used to notify the users. If it's irrelevant to users, it should not be deprecated but rather simply removed. So now let's get back to |
WalkthroughWalkthroughThe recent changes significantly enhance resource management in the Changes
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (21)
Files skipped from review due to trivial changes (5)
Additional comments not posted (19)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
|
I'm ready to restart working on this PR. But before I change any code, I would like to discuss the strategy and see whether we could reach some agreements:
|
|
For me (1) and (2) sound reasonable by now! So I think, the first step is to make sure that all sinks/sources actually clean up their resources (close) in their dtors, unless they haven't done so? Next step is to remove all Close calls in FairRootManager / FairRun. And document this as a feature or even a breaking change? Does that sound right? |
|
Yes. Good, then I will proceed this PR very shortly.
I think so. I may double check it.
Yes, I will also put this change in the CHANGELOG |
|
Awesome! If this is easier for you, consider handling the first step in this PR and when it's done, create a new PR for the second step. |
YanzhaoW
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (1)
fairroot/base/source/FairFileSourceBase.h (1)
53-53: Consider usingstd::unique_ptrfor ownership clarity.The TODO comment suggests using
std::unique_ptrforfRootFile. This would provide clearer ownership semantics and automatic resource management.// TODO : Ownership should be clear. Use std::unique_ptr? fairroot::detail::maybe_owning_ptr<TFile> fRootFile; //!
|
I have to remove the calling of Close functions in FairRootManager and FairRoot derivatives in this PR. Otherwise, the program is guaranteed to perform the double closing, which may cause double deletion for downstream projects. If everything is already, I will squash all commits before the merging. (Don't know why CI in linux machines don't work right now.) |
YanzhaoW
force-pushed
the
edwin_fairroot
branch
3 times, most recently
from
1aebc2e to
f4d76cd
Compare
|
Hi, I have made the changes according to your standard. So if there is nothing else, it's ready to be merged. |
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (2)
fairroot/base/steer/FairRunSim.cxx (1)
326-328: Resource cleanup in destructor not handled.The destructor of
FairSinkis empty and does not contain any resource cleanup logic. This could lead to potential resource leaks since thefSink->Close()call was removed fromFairRunSim::Run.
fairroot/base/sink/FairSink.cxx: The destructor~FairSink()is empty.Please ensure that the destructor of
FairSinkproperly handles resource cleanup to prevent potential leaks.Analysis chain
Verify resource cleanup in destructor.
The removal of the
fSink->Close()call implies that resource cleanup forfSinkis now expected to be handled in the destructor. Ensure that the destructor offSinkproperly handles resource cleanup to prevent potential leaks.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the destructor of `fSink` handles resource cleanup. # Test: Search for the destructor implementation of `fSink`. Expect: Proper resource cleanup logic. rg --type cpp --pattern $'~fSink()'Length of output: 448
Script:
#!/bin/bash # Description: Verify that the destructor of `fSink` handles resource cleanup. # Test: Search for the destructor implementation of `fSink`. Expect: Proper resource cleanup logic. rg '~fSink()' --type cpp -A 10Length of output: 32
Script:
#!/bin/bash # Description: Identify the class type of `fSink`. # Test: Search for the definition of `fSink` to determine its class type. rg 'fSink' --type cpp -A 5Length of output: 10242
Script:
#!/bin/bash # Description: Verify that the destructor of `FairSink` handles resource cleanup. # Test: Search for the definition of the `FairSink` class. rg 'class FairSink' --type cpp -A 10 # Test: Search for the destructor implementation of `FairSink`. rg '~FairSink()' --type cpp -A 10Length of output: 2980
fairroot/base/steer/FairRunAna.cxx (1)
561-561: Potential resource leak due to empty destructor inFairSink.The destructor of the
FairSinkclass is currently empty, which means no resource cleanup is being performed. This could lead to potential resource leaks since thefRootManager->CloseSink()call has been removed fromFairRunAna::TerminateRun. Ensure that resource management is properly handled to avoid leaks.
fairroot/base/sink/FairSink.cxx:FairSink::~FairSink() {}Analysis chain
Verify resource cleanup in destructor.
The removal of the
fRootManager->CloseSink()call implies that resource cleanup for the sink is now expected to be handled in the destructor. Ensure that the destructor of the sink properly handles resource cleanup to prevent potential leaks.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the destructor of the sink handles resource cleanup. # Test: Search for the destructor implementation of the sink. Expect: Proper resource cleanup logic. rg --type cpp --pattern $'~FairSink()'Length of output: 451
Script:
#!/bin/bash # Description: Verify that the destructor of the FairSink class handles resource cleanup. # Test: Search for the destructor implementation of the FairSink class. Expect: Proper resource cleanup logic. rg --type cpp '~FairSink()'Length of output: 141
YanzhaoW
force-pushed
the
edwin_fairroot
branch
from
f4d76cd to
d90a6d9
Compare
Deprecation of Close method in FairSource/Sink.Checklist: