Remove calling Close method in FairRootManager/FairRun

[YanzhaoW]

• ~~Deprecation of Close method in FairSource/Sink.~~
• Move the Close executions in the derived classes to their dtor (if needed).
• Remove callings of the Close methods in FairRootManager and FairRun.

---

Checklist:

• [x] Followed the Contributing Guidelines

[YanzhaoW]

a sidenote: I have rewritten the FairFileSource class in R3BRoot in this PR (actually it's R3BFileSource, which is a copy-paste from FairFileSource). Maybe we could migrate the new class here?

[karabowi]

I have tested FairRoot's private branch without FairSampler::Close() and FairSink::Close() with PandaRoot. I did not observe any crash.

[ChristianTackeGSI]

Deprecations in General

For me a deprecation means, that

1. All old code stays as it is (as long as it can be considered public API). So we should not change too much code. So that users' code continues to work as without this PR!
2. Any public API that should not be used in the future by users, should be marked as deprecated (either via [[deprecated]], or via comments, if the former is not possible).

In FairRoot: Public API

Let's look at (1):

Public API is basically everything in fairroot/. So we should not change any code or behaviour there, basically. Of course we can do some simple stuff: Instead of calling FairRootManager::CloseSink (which calls fSink->Close();) we can call fSink->Close(); directly. This does not change any actual behaviour.

When it comes to examples/ we should go the full route! We should show to users what we expect their code to look like in the future.

FairRoot: The confusing Close situation

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

1. We want to deprecate CloseForUsers, so that users do not call it any more:

   public:
       [[deprecated]] void CloseForUsers() { CloseImpl(); }
   

   Note: CloseForInternalUse is not deprecated. We want to continue calling it from the library, so that the CloseImpl is still called.

   Corollary: We would remove any calls to CloseForUsers from examples/!

2. We want to deprecate overriding CloseImpl:

   private:
       // \deprecated Please stop overriding it!
       virtual void CloseImpl() {}
   

   Note 1: There is no point in marking it override. We want to deprecate overriding it. No need to deprecate calling it. It can only be called by us. All of our business.

   Note 2: We still continue calling CloseForInternalUse (to keep bevahiour exactly as it is)

   Note 3: We would still keep the overriding in fairroot/, because we did not yet remove CloseForUsers(). So code that depends on CloseForUsers() calling something should be happy.

   Corollary: We would remove any overrding of CloseImpl from examples/.

3. Remove CloseForUsers, keep everything else the same

4. Remove CloseImpl, and remove all of CloseForInternalUse.

That's my current idea on how this should look like.

The problem is now, that all of those three (CloseImpl, CloseForInternalUse, and CloseForUsers) are currently in one function Close to make things complex.

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.

[YanzhaoW]

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.

[YanzhaoW]

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 FairSink::Close() or keep it in the public API of FairRootManager:

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

Here are my arguments why it should just be removed based on the following principles:

1. Deprecation should only be applied to public API.
2. Public API shouldn't be called internally.
3. Virtual functions should be private and shouldn't be called in the derived classes.

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

Prefer to use Template Method to make the interface stable and nonvirtual, while delegating customizable work to nonpublic virtual functions that are responsible for implementing the customizable behavior. After all, virtual functions are designed to let derived classes customize behavior; it's better to not let publicly derived classes also customize the inherited interface, which is supposed to be consistent.

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 CLoseSink(). Here the closing of the fSink is an internal logic for FairRootManager. Users of FairRootManager only interact with the public API CloseSink instead of fSink directly. For them, what's inside this public API shouldn't be their concerns and shouldn't break their code if changed. So if we need to discard the Close() method of FairSink, I think it's totally fine to be removed in FairRootManager::CloseSink() now.

[coderabbitai[bot]]

Walkthrough

Walkthrough

The recent changes significantly enhance resource management in the FairRoot module by deprecating explicit Close methods and relying on destructors for automatic cleanup. This shift improves encapsulation and reduces the risk of resource leaks. Additionally, operator overloads and public API elements have been refined for better safety and performance, contributing to a cleaner and more efficient codebase.

Changes

Files and Changes	Summary
fairroot/base/sink/FairSink.h, fairroot/base/source/FairSource.h	Deprecated Close methods in both classes, shifting resource management to destructors with empty implementations.
fairroot/base/source/FairMixedSource.cxx, fairroot/base/source/FairMixedSource.h	Removed Close method and updated destructor for resource management; altered AddSignalFile for clarity.
fairroot/base/source/FairFileSource.cxx, fairroot/base/source/FairFileSource.h	Removed destructor and Close method; enhanced implementations for AddFriend and AddFile.
fairroot/online/source/FairOnlineSource.h	Removed overridden Close method, indicating a shift in resource management responsibilities.
fairroot/online/steer/FairRunOnline.cxx	Eliminated resource closure calls in Finish, suggesting changes in cleanup operations at the end of tasks.
fairroot/base/steer/FairRootManager.h	Adjusted conditional check in CloseSink method for clarity by comparing fSink to nullptr.

---

Recent review details

Configuration used: CodeRabbit UI Review profile: CHILL

Commits

Files that changed from the base of the PR and between f4d76cd8423d47f52079748826ed8d7012a42101 and d90a6d929b2daa33190b8251bb14626122b931b4.

Files selected for processing (21)

• CHANGELOG.md (1 hunks)
• examples/MQ/pixelDetector/src/FairOnlineSink.h (1 hunks)
• examples/MQ/pixelDetector/src/PixelDigiBinSource.cxx (2 hunks)
• examples/MQ/pixelDetector/src/PixelDigiBinSource.h (2 hunks)
• examples/MQ/pixelDetector/src/PixelDigiSource.cxx (1 hunks)
• examples/MQ/pixelDetector/src/PixelDigiSource.h (2 hunks)
• fairroot/base/sim/FairMCApplication.cxx (1 hunks)
• fairroot/base/sink/FairRootFileSink.cxx (2 hunks)
• fairroot/base/sink/FairRootFileSink.h (2 hunks)
• fairroot/base/sink/FairSink.h (1 hunks)
• fairroot/base/source/FairFileSource.cxx (2 hunks)
• fairroot/base/source/FairFileSource.h (2 hunks)
• fairroot/base/source/FairFileSourceBase.h (1 hunks)
• fairroot/base/source/FairMixedSource.cxx (3 hunks)
• fairroot/base/source/FairMixedSource.h (1 hunks)
• fairroot/base/source/FairSource.h (1 hunks)
• fairroot/base/steer/FairRootManager.h (2 hunks)
• fairroot/base/steer/FairRunAna.cxx (1 hunks)
• fairroot/base/steer/FairRunSim.cxx (1 hunks)
• fairroot/online/source/FairOnlineSource.h (1 hunks)
• fairroot/online/steer/FairRunOnline.cxx (1 hunks)

Files skipped from review due to trivial changes (5)

• examples/MQ/pixelDetector/src/FairOnlineSink.h
• examples/MQ/pixelDetector/src/PixelDigiBinSource.cxx
• examples/MQ/pixelDetector/src/PixelDigiSource.cxx
• fairroot/base/sink/FairRootFileSink.cxx
• fairroot/base/steer/FairRootManager.h

Additional comments not posted (19)

fairroot/online/source/FairOnlineSource.h (1)

Line range hint 14-14: Removal of Close() method.

The Close() method has been removed from the FairOnlineSource class. This change suggests a design shift towards handling resource cleanup in destructors, which can improve encapsulation and reduce the risk of resource leaks. Ensure that derived classes manage their resources appropriately.

fairroot/base/source/FairFileSourceBase.h (1)

58-63: Addition of Close() method for resource management.

The new Close() method in FairFileSourceBase ensures that the file is closed if fRootFile is not null. This enhances resource management by providing a clear and explicit mechanism for closing files, reducing the risk of resource leaks.

examples/MQ/pixelDetector/src/PixelDigiSource.h (1)

80-81: Implementation of Close() method for resource management.

The Close() method now directly closes fInputFile within the PixelDigiSource class. This implementation ensures proper resource cleanup, aligning with the objective of improving resource management.

examples/MQ/pixelDetector/src/PixelDigiBinSource.h (1)

81-81: Encapsulation of the Close method.

The Close method has been moved to a private scope and implemented inline. This change enhances encapsulation by preventing external access to Close, ensuring that file handling is managed internally. Ensure that the destructor or other mechanisms properly handle resource cleanup.

fairroot/base/source/FairSource.h (1)

38-38: Default implementation for Close.

The Close method now has a default empty implementation. This change simplifies subclassing by not requiring an explicit override. However, ensure that derived classes implement any necessary cleanup logic if they rely on specific resource management.

fairroot/base/sink/FairSink.h (1)

43-43: Default implementation for Close.

The Close method now has a default empty implementation. This change simplifies subclassing by not requiring an explicit override. Ensure that derived classes implement necessary cleanup logic if they have specific resource management needs.

fairroot/base/sink/FairRootFileSink.h (1)

68-68: Consider marking the Close() method as deprecated.

Reintroducing the Close() method is necessary for backward compatibility. However, it should be marked as deprecated to inform users of its future removal.

- void Close() override;
+ [[deprecated("Use destructor for cleanup")]] void Close() override;

fairroot/base/source/FairFileSource.h (2)

43-43: LGTM: Destructor defaulted for simplicity.

The destructor is now defaulted, which simplifies the class definition and adheres to modern C++ practices.

---

67-69: Deprecation of CloseInFile() method.

The CloseInFile() method is correctly marked as deprecated, guiding users to use the Close() function instead.

Ensure that all usages of CloseInFile() are updated to use the Close() method.

Verification successful

---

All usages of CloseInFile() have been updated.

The search did not find any occurrences of CloseInFile() apart from its declaration, indicating that all usages have been updated to use the Close() method.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all usages of `CloseInFile()` are updated to `Close()`.

# Test: Search for the usage of `CloseInFile()`. Expect: No occurrences.
rg --type cpp $'CloseInFile()'

Length of output: 131

fairroot/base/source/FairMixedSource.h (2)

46-46: LGTM: Destructor defaulted for simplicity.

The destructor is now defaulted, which simplifies the class definition and adheres to modern C++ practices.

---

46-46: Verify the impact of removing the Close() method.

The removal of the Close() method suggests a change in resource management. Ensure that this change does not affect the system's stability or functionality.

Verification successful

---

Removal of Close() Method in FairMixedSource Appears Safe

The Close() method was not found in the FairMixedSource class or any derived classes, indicating that its removal does not impact the current functionality or resource management strategy. No direct references or overrides were identified in the codebase.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the impact of removing the `Close()` method.

# Test: Search for the usage of `Close()` in `FairMixedSource`. Expect: No occurrences.
rg --type cpp $'FairMixedSource::Close()'

Length of output: 42

---

Script:

#!/bin/bash
# Search for the declaration of `Close()` in FairMixedSource.h
rg --type cpp 'Close()' -A 5

# Search for any classes derived from FairMixedSource
rg --type cpp 'class .*: public FairMixedSource' -A 5

# Search for any documentation or comments mentioning `Close()`
rg --type cpp 'Close' -A 5

Length of output: 107553

fairroot/online/steer/FairRunOnline.cxx (1)

312-314: Verify resource management after removing Close calls.

The removal of GetSource()->Close(); and fRootManager->CloseSink(); in the Finish method indicates a change in how resources are managed. Ensure that resources are properly released elsewhere to prevent leaks.

fairroot/base/steer/FairRunSim.cxx (1)

326-328: Verify resource management for fSink.

The removal of the conditional block checking fSink before calling Close() suggests a change in resource management. Ensure that fSink is properly managed elsewhere to prevent resource leaks.

Verification successful

---

Resource Management for fSink is Properly Handled

The use of std::unique_ptr for fSink in FairRun and FairRootManager ensures that resources are automatically managed and deallocated. The absence of a direct Close() call in FairRunSim.cxx does not lead to resource leaks, as the smart pointer effectively manages the lifecycle of fSink.

• fSink is declared as a std::unique_ptr<FairSink> in FairRun.h.
• Resource management is confirmed through the use of smart pointers, which handle deallocation automatically when the object goes out of scope or is reset.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that `fSink` resources are managed properly after removing the Close call.

# Test: Search for destructors in classes managing `fSink` to ensure they handle resource cleanup.
ast-grep --lang cpp --pattern $'class $_ : public $_ {
  $$$
  ~$_() {
    $$$
  }
  $$$
}'

Length of output: 437

---

Script:

#!/bin/bash
# Search for the declaration of `fSink` in FairRunSim class to understand its scope and management.
ast-grep --lang cpp --pattern 'class FairRunSim {
  $$$
  FairField* fSink;
  $$$
}'

# Search for all usages of `fSink` within FairRunSim class to verify resource management.
rg 'fSink' -A 5 --context-separator='---' -- 'fairroot/base/steer/FairRunSim.cxx'

Length of output: 164

---

Script:

#!/bin/bash
# Search for all member variables in the FairRunSim class to identify potential management of fSink.
ast-grep --lang cpp --pattern 'class FairRunSim {
  $$$
  $_ $_;
  $$$
}'

# Search for all member functions in the FairRunSim class to identify potential management of fSink.
ast-grep --lang cpp --pattern 'class FairRunSim {
  $$$
  $_ $_() {
    $$$
  }
  $$$
}'

Length of output: 511

---

Script:

#!/bin/bash
# Search for any occurrences of `fSink` across the entire repository to gather more information about its management.
rg 'fSink' -A 5 --context-separator='---'

Length of output: 10276

CHANGELOG.md (1)

85-87: Changelog entry accurately reflects changes.

The changelog correctly documents the removal of Close() calls and the shift to managing resources in destructors. This entry provides clear information for users about the changes.

fairroot/base/steer/FairRunAna.cxx (1)

561-561: Verify the impact of removing CloseSink().

The removal of fRootManager->CloseSink() from TerminateRun might affect how resources are managed at the end of a run. Ensure that resources are correctly cleaned up in destructors or other mechanisms.

fairroot/base/source/FairFileSource.cxx (2)

304-307: Enhanced readability for AddFriend.

The conversion of AddFriend to a multi-line function body improves readability and maintainability. Consider adding logging or error handling if needed in future modifications.

---

309-312: Enhanced readability for AddFile.

The conversion of AddFile to a multi-line function body enhances clarity. This change facilitates future modifications, such as adding logging or error handling.

fairroot/base/source/FairMixedSource.cxx (1)

422-425: Improved clarity in AddSignalFile.

The conversion to a multi-line function body in AddSignalFile enhances readability and allows for easier future modifications.

fairroot/base/sim/FairMCApplication.cxx (1)

Line range hint 453-455: Verify resource management in FinishRunOnWorker.

The removal of explicit sink closure lines suggests a reliance on destructors for resource management. Ensure that all resources are properly cleaned up to prevent leaks.

---

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?

Share

• X
• Mastodon
• Reddit
• LinkedIn

Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>.
  • Generate unit testing code for this file.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit testing code for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai generate interesting stats about this repository and render them as a table.
  • @coderabbitai show all the console.log statements in this repository.
  • @coderabbitai read src/utils.ts and generate unit testing code.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

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)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configuration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• Please see the configuration documentation for more information.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[YanzhaoW]

Hi, @ChristianTackeGSI

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:

1. No need to deprecate the Close() method of FairSink/FairSource We could follow the way how STL and ROOT deal with file sources: By default, closing a resource should be done automatically with dtor. But we could still provide a close api for some special needs from user side.
2. Do not call any close in the internal implementation of FairRootManager. Calling time of the Close function should only be determined by the derived class implementors and their users. It should be allowed that derived classes can manage the closing orders of different sources themselves. If they don't want to manage that, it's required that at least they close their own resources in their dtors (ownership).

[ChristianTackeGSI]

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?

[YanzhaoW]

Yes. Good, then I will proceed this PR very shortly.

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?

I think so. I may double check it.

Next step is to remove all Close calls in FairRootManager / FairRun. And document this as a feature or even a breaking change?

Yes, I will also put this change in the CHANGELOG

[ChristianTackeGSI]

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]

@ChristianTackeGSI

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]

Hi,

I have made the changes according to your standard.

So if there is nothing else, it's ready to be merged.