Skip to content
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

Merged
merged 2 commits into from
Jul 4, 2022

Conversation

paulgessinger
Copy link
Member

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.

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.
@paulgessinger paulgessinger added this to the next milestone Jun 28, 2022
@codecov
Copy link

codecov bot commented Jun 28, 2022

Codecov Report

Merging #1296 (318e72b) into main (5ad6aa7) will not change coverage.
The diff coverage is n/a.

@@           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           

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@AJPfleger
Copy link
Contributor

Does this problem affect only these two classes? I am asking, because we use the same approach for some/all other writer-classes.

@paulgessinger
Copy link
Member Author

We certainly could. I saw crashes for these two classes specifically, so those are the ones I touched for now.

Copy link
Contributor

@AJPfleger AJPfleger left a 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

@paulgessinger
Copy link
Member Author

We could extend the scope of #881 for this purpose, @AJPfleger?

@AJPfleger
Copy link
Contributor

Yes, #881 seems well suited.

@AJPfleger AJPfleger added Impact - Minor Nuissance bug and/or affects only a single module automerge labels Jul 4, 2022
@kodiakhq kodiakhq bot merged commit 83880e0 into acts-project:main Jul 4, 2022
@andiwand andiwand modified the milestones: next, v19.4.0 Jul 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Impact - Minor Nuissance bug and/or affects only a single module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants