Fix signal timer resetting #946
Conversation
| # 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I mean that the CI will get hung up - no idea what the timeout is for that thing - so the latter suggestion works.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
| to_wait = min(remaining_time, next_interval) | ||
| else: | ||
| to_wait = next_interval | ||
| Scalene.client_timer.reset() |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
|
I'm going to merge; we can address the testing question later. |
Fixed user timer being disabled after it "rings" instead of being reset