-
Notifications
You must be signed in to change notification settings - Fork 173
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Output ROOT file closing on destruct #1296
fix: Output ROOT file closing on destruct #1296
Conversation
While I believe the writing should happen in `endRun`, I've seen segfaults when ROOT's exit handler tries to close them and sees corrupted memory. This PR should fix that for these two classes.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1296 +/- ##
=======================================
Coverage 47.43% 47.43%
=======================================
Files 375 375
Lines 19785 19785
Branches 9290 9290
=======================================
Hits 9385 9385
Misses 4020 4020
Partials 6380 6380 ☔ View full report in Codecov by Sentry. |
Does this problem affect only these two classes? I am asking, because we use the same approach for some/all other writer-classes. |
We certainly could. I saw crashes for these two classes specifically, so those are the ones I touched for now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look good to me, but I think we should do one of the following:
- Check and adapt the other writer-classes
- Open an Issue to not forget about ^ in the future
We could extend the scope of #881 for this purpose, @AJPfleger? |
Yes, #881 seems well suited. |
Caution Review failedThe pull request is closed. WalkthroughIn the realm of file management within the Acts Examples project, two Root-based writer classes have undergone subtle refinements. The Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
While I believe the writing should happen in
endRun
, I've seensegfaults when ROOT's exit handler tries to close them and sees
corrupted memory. This PR should fix that for these two classes.
Summary by CodeRabbit
Bug Fixes
endRun
method by removing redundant file closure calls.Refactor
RootTrajectoryStatesWriter
andRootTrajectorySummaryWriter
to enhance error handling and resource management.