bpo-31479: Always reset the signal alarm in tests #3588
Conversation
Use "try: ... finally: signal.signal(0)" pattern to make sure that tests don't "leak" a pending fatal signal alarm.
Lib/test/test_selectors.py
Outdated
| # select() was interrupted before the timeout of 30 seconds | ||
| self.assertLess(time() - t, 5.0) | ||
| try: | ||
| s.register(rd, selectors.EVENT_READ) |
There was a problem hiding this comment.
What if the process is switched before this line, paused on more than 1 sec, and a SIGALRM signal is raised before registering a selector?
There was a problem hiding this comment.
I moved the signal.alarm(1) into the try/except.
"
"What if (...) a SIGALRM signal is raised before registering a selector?" : the test would fail, no? In my PR, I only care of making sure that a scheduled alarm is always cancelled if something goes wrong.
| # raise an exception, so select() should by retries with a recomputed | ||
| # timeout | ||
| self.assertFalse(s.select(1.5)) | ||
| self.assertGreaterEqual(time() - t, 1.0) |
There was a problem hiding this comment.
Shouldn't t be calculated before signal.alarm(1)?
There was a problem hiding this comment.
I don't think that this question is related to my change. If you would like to enhance test_selectors.py, please propose a different PR. I'm not aware of any issue with the existing code.
(I don't know if the line should be moved or not.)
But I also moved signal.alarm(1) into the try block.
| with self.assertRaises(ZeroDivisionError) as cm: | ||
| func(*args, **kwargs) | ||
| try: | ||
| self.setAlarm(self.alarm_time) |
There was a problem hiding this comment.
Shouldn't this be outside of the try block?
There was a problem hiding this comment.
If you press CTRL+c to interrupt the test just after setAlarm() but before entering the try block, the scheduled alarm is not cancelled. The purpose of my change is to make sure that it's always cancelled.
There was a problem hiding this comment.
Thanks for the clarification.
| try: | ||
| signal.alarm(2) # POSIX allows alarm to be up to 1 second early |
There was a problem hiding this comment.
Same reason than my previous comment.
| @@ -51,14 +51,11 @@ class PtyTest(unittest.TestCase): | |||
| def setUp(self): | |||
| # isatty() and close() can hang on some platforms. Set an alarm | |||
| # before running the test to make sure we don't hang forever. | |||
| self.old_alarm = signal.signal(signal.SIGALRM, self.handle_sig) | |||
| old_alarm = signal.signal(signal.SIGALRM, self.handle_sig) | |||
| self.addCleanup(signal.signal, signal.SIGALRM, old_alarm) | |||
There was a problem hiding this comment.
In my experience, tearDown() is an called if a test raises an exception. But I'm not sure about that.
Well, I also like the addCleanup() API to move the cleanup code closer to the setup code.
Fix also typo: replace signal.signal(0) with signal.alarm(0)
Lib/test/signalinterproctester.py
Outdated
| self.wait_signal(None, 'SIGALRM', KeyboardInterrupt) | ||
| self.assertEqual(self.got_signals, {'SIGHUP': 1, 'SIGUSR1': 1, | ||
| 'SIGALRM': 0}) | ||
| try: |
There was a problem hiding this comment.
Move signal.alarm(1) into a 'try' block?
| with self.assertRaises(ZeroDivisionError) as cm: | ||
| func(*args, **kwargs) | ||
| try: | ||
| self.setAlarm(self.alarm_time) |
There was a problem hiding this comment.
Thanks for the clarification.
|
Would a special context manager make the code clearer? |
I'm not sure that it's easy to factorize the code since many tests are different. I let you try the exercice if you want :-) |
|
Crap, as usual I forgot to modify the commit message :-( |
* bpo-31479: Always reset the signal alarm in tests Use "try: ... finally: signal.signal(0)" pattern to make sure that tests don't "leak" a pending fatal signal alarm. * Move two more alarm() calls into the try block Fix also typo: replace signal.signal(0) with signal.alarm(0) * Move another signal.alarm() into the try block (cherry picked from commit 9abee72)
* bpo-31479: Always reset the signal alarm in tests Use "try: ... finally: signal.signal(0)" pattern to make sure that tests don't "leak" a pending fatal signal alarm. * Move two more alarm() calls into the try block Fix also typo: replace signal.signal(0) with signal.alarm(0) * Move another signal.alarm() into the try block (cherry picked from commit 9abee72)
Use "try: ... finally: signal.signal(0)" pattern to make sure that
tests don't "leak" a pending fatal signal alarm.
https://bugs.python.org/issue31479