Skip to content

Fix signal timer resetting #946

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.

Sign up for GitHub

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

Merged
emeryberger merged 1 commit into from
Sep 1, 2025
Merged

Fix signal timer resetting #946

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .github/workflows/test-smoketests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,12 @@ jobs:
- name: decorator smoke test
run: python test/smoketest_profile_decorator.py

# NOTE: this is a regression test for signals not being
# restarted properly. It checks whether the program
# halts or runs forever, not for correctness of timing.
- name: signal smoketest
run: python test/smoketest.py test/signal_test.py
Copy link
Member

@emeryberger emeryberger Aug 31, 2025

Choose a reason for hiding this comment

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

This is a bad failure case, plus we have a regression test suite in tests/. Make it a pytest and put it in that directory so it gets run in that workflow, but with a timeout for failure.

Copy link
Collaborator Author

@sternj sternj Aug 31, 2025

Choose a reason for hiding this comment

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

I think the failure case is the right one-- the behaviors this patch was in response to were the signal not being delivered for whatever reason and this test checks for progress.

I'll put it in the tests workflow, I actually think it makes sense as a task with a timeout rather than as a pytest though, unless you wanted to start making pytests that spawn new processes (which it doesn't seem like we do at the moment, the pytest suite seems to be mostly unit tests). I do think it's structurally more similar to the smoketests, since we're just checking "are the signals delivered and does it create a profile with any data in it", but that's your call

Copy link
Member

@emeryberger emeryberger Aug 31, 2025

Choose a reason for hiding this comment

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

I mean that the CI will get hung up - no idea what the timeout is for that thing - so the latter suggestion works.

Copy link
Collaborator Author

@sternj sternj Aug 31, 2025

Choose a reason for hiding this comment

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

The timeout of the CI is 15 minutes and I put in a timeout of 30 seconds for the task I created, where would the hangup happen?

Copy link
Collaborator Author

@sternj sternj Aug 31, 2025

Choose a reason for hiding this comment

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

Also the smoketests worked fine, I could make sure the timeout works as expected but I trust the timeout-minutes directive to do the right thing

timeout-minutes: 0.5
# FIXME: these tests are broken under the current Github runner
#
# - name: line invalidation test
Expand Down
1 change: 0 additions & 1 deletion scalene/scalene_profiler.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -731,7 +731,6 @@ def cpu_signal_handler(
to_wait = min(remaining_time, next_interval)
else:
to_wait = next_interval
Scalene.client_timer.reset()
Copy link
Member

@emeryberger emeryberger Aug 31, 2025

Choose a reason for hiding this comment

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

This works but I still get the warning about the signal, so we need to filter that.

WARNING: Scalene uses Alarm clock: 14 to profile.
If your code raises Alarm clock: 14 from non-Python code, use SIGUSR1.
Code that raises signals from within Python code will be rerouted.

Copy link
Collaborator Author

@sternj sternj Aug 31, 2025

Choose a reason for hiding this comment

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

That is desired behavior-- we put in that warning because we're changing the behavior in a way transparent when you're doing Python-only stuff, but if someone was using a C extension module that raised SIGALRM their code would break in weird ways.

Copy link
Member

@emeryberger emeryberger Aug 31, 2025

Choose a reason for hiding this comment

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

Not sure I follow. This warning comes from when the signal is set in Python-land (in scalene/replacement_signal_fns.py). It wouldn't be seen by C code that didn't invoke Python.

Copy link
Collaborator Author

@sternj sternj Aug 31, 2025

Choose a reason for hiding this comment

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

Right-- we print the warning because, while we can transparently redirect everything in Python-land, we want to warn the user in case they're doing something in C-land

Scalene.__signal_manager.restart_timer(to_wait)
else:
Scalene.__signal_manager.restart_timer(next_interval)
Expand Down
16 changes: 16 additions & 0 deletions test/signal_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import signal

iterations = 10

def my_handler(sig, frame):
global iterations
print(f"seconds remaining: {iterations}")
if iterations > 0:
iterations -= 1
signal.setitimer(signal.ITIMER_REAL, 1.0, 0)

signal.signal(signal.SIGALRM, my_handler)
signal.setitimer(signal.ITIMER_REAL, 1.0, 0)

while iterations:
signal.pause()
Loading