Skip to content

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

Merged
merged 4 commits into from
Sep 19, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
11 changes: 7 additions & 4 deletions Lib/test/signalinterproctester.py
Original file line number Diff line number Diff line change
Expand Up @@ -74,10 +74,13 @@ def test_interprocess_signal(self):
# Nothing should happen: SIGUSR2 is ignored
child.wait()

signal.alarm(1)
self.wait_signal(None, 'SIGALRM', KeyboardInterrupt)
self.assertEqual(self.got_signals, {'SIGHUP': 1, 'SIGUSR1': 1,
'SIGALRM': 0})
try:
signal.alarm(1)
self.wait_signal(None, 'SIGALRM', KeyboardInterrupt)
self.assertEqual(self.got_signals, {'SIGHUP': 1, 'SIGUSR1': 1,
'SIGALRM': 0})
finally:
signal.alarm(0)


if __name__ == "__main__":
Expand Down
3 changes: 3 additions & 0 deletions Lib/test/test_io.py
Original file line number Diff line number Diff line change
Expand Up @@ -3940,6 +3940,7 @@ def on_alarm(*args):
if isinstance(exc, RuntimeError):
self.assertTrue(str(exc).startswith("reentrant call"), str(exc))
finally:
signal.alarm(0)
wio.close()
os.close(r)

Expand Down Expand Up @@ -3968,6 +3969,7 @@ def alarm_handler(sig, frame):
# - third raw read() returns b"bar"
self.assertEqual(decode(rio.read(6)), "foobar")
finally:
signal.alarm(0)
rio.close()
os.close(w)
os.close(r)
Expand Down Expand Up @@ -4035,6 +4037,7 @@ def alarm2(sig, frame):
self.assertIsNone(error)
self.assertEqual(N, sum(len(x) for x in read_results))
finally:
signal.alarm(0)
write_finished = True
os.close(w)
os.close(r)
Expand Down
9 changes: 3 additions & 6 deletions Lib/test/test_pty.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

Why this is changed?

Copy link
Member Author

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.

self.addCleanup(signal.alarm, 0)
signal.alarm(10)

def tearDown(self):
# remove alarm, restore old alarm handler
signal.alarm(0)
signal.signal(signal.SIGALRM, self.old_alarm)

def handle_sig(self, sig, frame):
self.fail("isatty hung")

Expand Down
40 changes: 22 additions & 18 deletions Lib/test/test_selectors.py
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand All @@ -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)
Copy link
Member

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)?

Copy link
Member Author

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.

finally:
signal.alarm(0)


class ScalableSelectorMixIn:
Expand Down
23 changes: 14 additions & 9 deletions Lib/test/test_socket.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member

Choose a reason for hiding this comment

The 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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

Why this is moved?

Copy link
Member Author

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.

foo = self.serv.accept()
except socket.timeout:
self.fail("caught timeout instead of Alarm")
Expand Down
10 changes: 7 additions & 3 deletions Lib/test/test_threadsignals.py
Original file line number Diff line number Diff line change
Expand Up @@ -56,9 +56,11 @@ def test_signals(self):
# wait for it return.
if signal_blackboard[signal.SIGUSR1]['tripped'] == 0 \
or signal_blackboard[signal.SIGUSR2]['tripped'] == 0:
signal.alarm(1)
signal.pause()
signal.alarm(0)
try:
signal.alarm(1)
signal.pause()
finally:
signal.alarm(0)

self.assertEqual( signal_blackboard[signal.SIGUSR1]['tripped'], 1)
self.assertEqual( signal_blackboard[signal.SIGUSR1]['tripped_by'],
Expand Down Expand Up @@ -98,6 +100,7 @@ def test_lock_acquire_interruption(self):
# after timeout return of lock.acquire() (which can fool assertRaises).
self.assertLess(dt, 3.0)
finally:
signal.alarm(0)
signal.signal(signal.SIGALRM, oldalrm)

@unittest.skipIf(USING_PTHREAD_COND,
Expand Down Expand Up @@ -131,6 +134,7 @@ def other_thread():
# See rationale above in test_lock_acquire_interruption
self.assertLess(dt, 3.0)
finally:
signal.alarm(0)
signal.signal(signal.SIGALRM, oldalrm)

def acquire_retries_on_intr(self, lock):
Expand Down