-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
bpo-31479: Always reset the signal alarm in tests #3588
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
Changes from all commits
11408ec
c30f5d2
1daaa9c
93b025c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -399,17 +399,19 @@ def handler(*args): | |
|
||
orig_alrm_handler = signal.signal(signal.SIGALRM, handler) | ||
self.addCleanup(signal.signal, signal.SIGALRM, orig_alrm_handler) | ||
self.addCleanup(signal.alarm, 0) | ||
|
||
signal.alarm(1) | ||
try: | ||
signal.alarm(1) | ||
|
||
s.register(rd, selectors.EVENT_READ) | ||
t = time() | ||
# select() is interrupted by a signal which raises an exception | ||
with self.assertRaises(InterruptSelect): | ||
s.select(30) | ||
# select() was interrupted before the timeout of 30 seconds | ||
self.assertLess(time() - t, 5.0) | ||
s.register(rd, selectors.EVENT_READ) | ||
t = time() | ||
# select() is interrupted by a signal which raises an exception | ||
with self.assertRaises(InterruptSelect): | ||
s.select(30) | ||
# select() was interrupted before the timeout of 30 seconds | ||
self.assertLess(time() - t, 5.0) | ||
finally: | ||
signal.alarm(0) | ||
|
||
@unittest.skipUnless(hasattr(signal, "alarm"), | ||
"signal.alarm() required for this test") | ||
|
@@ -421,17 +423,19 @@ def test_select_interrupt_noraise(self): | |
|
||
orig_alrm_handler = signal.signal(signal.SIGALRM, lambda *args: None) | ||
self.addCleanup(signal.signal, signal.SIGALRM, orig_alrm_handler) | ||
self.addCleanup(signal.alarm, 0) | ||
|
||
signal.alarm(1) | ||
try: | ||
signal.alarm(1) | ||
|
||
s.register(rd, selectors.EVENT_READ) | ||
t = time() | ||
# select() is interrupted by a signal, but the signal handler doesn't | ||
# raise an exception, so select() should by retries with a recomputed | ||
# timeout | ||
self.assertFalse(s.select(1.5)) | ||
self.assertGreaterEqual(time() - t, 1.0) | ||
s.register(rd, selectors.EVENT_READ) | ||
t = time() | ||
# select() is interrupted by a signal, but the signal handler doesn't | ||
# 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
finally: | ||
signal.alarm(0) | ||
|
||
|
||
class ScalableSelectorMixIn: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3854,7 +3854,6 @@ def setUp(self): | |
orig_alrm_handler = signal.signal(signal.SIGALRM, | ||
lambda signum, frame: 1 / 0) | ||
self.addCleanup(signal.signal, signal.SIGALRM, orig_alrm_handler) | ||
self.addCleanup(self.setAlarm, 0) | ||
|
||
# Timeout for socket operations | ||
timeout = 4.0 | ||
|
@@ -3891,9 +3890,12 @@ def setUp(self): | |
def checkInterruptedRecv(self, func, *args, **kwargs): | ||
# Check that func(*args, **kwargs) raises | ||
# errno of EINTR when interrupted by a signal. | ||
self.setAlarm(self.alarm_time) | ||
with self.assertRaises(ZeroDivisionError) as cm: | ||
func(*args, **kwargs) | ||
try: | ||
self.setAlarm(self.alarm_time) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't this be outside of the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the clarification. |
||
with self.assertRaises(ZeroDivisionError) as cm: | ||
func(*args, **kwargs) | ||
finally: | ||
self.setAlarm(0) | ||
|
||
def testInterruptedRecvTimeout(self): | ||
self.checkInterruptedRecv(self.serv.recv, 1024) | ||
|
@@ -3948,10 +3950,13 @@ def checkInterruptedSend(self, func, *args, **kwargs): | |
# Check that func(*args, **kwargs), run in a loop, raises | ||
# OSError with an errno of EINTR when interrupted by a | ||
# signal. | ||
with self.assertRaises(ZeroDivisionError) as cm: | ||
while True: | ||
self.setAlarm(self.alarm_time) | ||
func(*args, **kwargs) | ||
try: | ||
with self.assertRaises(ZeroDivisionError) as cm: | ||
while True: | ||
self.setAlarm(self.alarm_time) | ||
func(*args, **kwargs) | ||
finally: | ||
self.setAlarm(0) | ||
|
||
# Issue #12958: The following tests have problems on OS X prior to 10.7 | ||
@support.requires_mac_ver(10, 7) | ||
|
@@ -4675,8 +4680,8 @@ def alarm_handler(signal, frame): | |
raise Alarm | ||
old_alarm = signal.signal(signal.SIGALRM, alarm_handler) | ||
try: | ||
signal.alarm(2) # POSIX allows alarm to be up to 1 second early | ||
try: | ||
signal.alarm(2) # POSIX allows alarm to be up to 1 second early | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why this is moved? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same reason than my previous comment. |
||
foo = self.serv.accept() | ||
except socket.timeout: | ||
self.fail("caught timeout instead of Alarm") | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this is changed?
There was a problem hiding this comment.
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.