bpo-31479: Always reset the signal alarm in tests by vstinner · Pull Request #3588 · python/cpython (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation16 Commits4 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Use "try: ... finally: signal.signal(0)" pattern to make sure that tests don't "leak" a pending fatal signal alarm.
| # select() was interrupted before the timeout of 30 seconds |
|---|
| self.assertLess(time() - t, 5.0) |
| try: |
| s.register(rd, selectors.EVENT_READ) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't t be calculated before signal.alarm(1)?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be outside of the try block?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clarification.
| try: |
|---|
| signal.alarm(2) # POSIX allows alarm to be up to 1 second early |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this is moved?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this is changed?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
| self.wait_signal(None, 'SIGALRM', KeyboardInterrupt) |
|---|
| self.assertEqual(self.got_signals, {'SIGHUP': 1, 'SIGUSR1': 1, |
| 'SIGALRM': 0}) |
| try: |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move signal.alarm(1) into a 'try' block?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| with self.assertRaises(ZeroDivisionError) as cm: |
|---|
| func(*args, **kwargs) |
| try: |
| self.setAlarm(self.alarm_time) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clarification.
Would a special context manager make the code clearer?
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 :-(
vstinner added a commit that referenced this pull request
- 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)
vstinner added a commit that referenced this pull request
- 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)