Skip to content

[io] Prevent losing data due to SIGTERM (under Unix) #18403

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ferdymercury
Copy link
Collaborator

@ferdymercury ferdymercury commented Apr 14, 2025

This Pull request:

Changes or fixes:

Fixes #13300

  • I tested this locally on Ubu22 and it works fine.
  • I do not know how to properly test it using GTEST, I only found test_polling.cxx doing something like that but it looked too complicated, so I ended up simplifying.
  • I do not know if Windows must catch SIGTERM somewhere else @bellenot ?

Checklist:

  • tested changes locally
  • updated the docs (if necessary)

Copy link

github-actions bot commented Apr 14, 2025

Test Results

    20 files      20 suites   4d 6h 27m 36s ⏱️
 3 016 tests  3 015 ✅ 0 💤 1 ❌
58 726 runs  58 725 ✅ 0 💤 1 ❌

For more details on these failures, see this check.

Results for commit beb694c.

♻️ This comment has been updated with latest results.

@bellenot
Copy link
Member

@ferdymercury The SIGILL and SIGTERM signals aren't generated under Windows. They're included for ANSI compatibility. See the signal reference

@ferdymercury ferdymercury changed the title [io] Prevent losing data due to SIGTERM [io] Prevent losing data due to SIGTERM (under Unix) Apr 22, 2025
@ferdymercury ferdymercury added this to the 6.38.00 milestone Apr 22, 2025
@bellenot bellenot assigned dpiparo and pcanal and unassigned bellenot Apr 25, 2025
@dpiparo dpiparo closed this May 23, 2025
@dpiparo dpiparo reopened this May 23, 2025
@dpiparo
Copy link
Member

dpiparo commented May 23, 2025

Let's have a look to the tests once again

Comment on lines +109 to +110
// gentle save and close if SIGTERM
if (signum == SIGTERM) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understood correctly, this would mean that after code similar to:

root [] TFile f(filename, RECREATE);
root [] do_stuff();
root [] if (all_is_well) f.Write();

We have a different behavior if the user does:

root [] .q

and

send SIGTERM

Where the former does not write the file while the later would now write the file.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

@ferdymercury ferdymercury force-pushed the sigterm branch 2 times, most recently from f6f707e to 4f28265 Compare May 25, 2025 16:31
@ferdymercury ferdymercury reopened this May 25, 2025
@ferdymercury ferdymercury added the clean build Ask CI to do non-incremental build on PR label May 25, 2025
@ferdymercury ferdymercury reopened this May 25, 2025
@ferdymercury ferdymercury removed the clean build Ask CI to do non-incremental build on PR label May 25, 2025
@dpiparo dpiparo closed this Jun 2, 2025
@dpiparo dpiparo reopened this Jun 2, 2025
Fixes root-project#13300

[core] Implement public static functions allowing save and close

[io] only SaveAndClose TFiles and derived

TSystemFile, in general, does not implemente Write and Close methods

[io] Do not write/close files open in READ mode

[core] use TDirectory instead TMethodCall
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test SIGTERM vs ~TFile
4 participants