PR#1673: Improve exception and SIGINT handling for tests/test_build - fm-orchestrator

fm-orchestrator

#1673 Improve exception and SIGINT handling for tests/test_build

Merged 2 years ago by breilly. Opened 3 years ago by otaylor.

otaylor/fm-orchestrator test-build-exceptions into master

test_build: leave Control-C working

Owen W. Taylor • 2 years ago

0d0d809

test_build: exit rather than hanging on event handler exception

Owen W. Taylor • 2 years ago

4ddd3c2

module_build_service/scheduler/events.py

file modified

+43

		`@@ -15,6 +15,7 @@`
		`from __future__ import absolute_import`
		`from functools import wraps`
		`import sched`
		`+ import threading`
		`import time`

		`from module_build_service.common import log`
		`@@ -66,6 +67,45 @@`
		`scheduler = Scheduler(time.time, delayfunc=lambda x: x)`


		`+ class EventTrap():`
		`+ """`
		`+ A context handler that can be used to provide a place to store exceptions`
		`+ that occur in event handlers during a section of code. This is global rather`
		`+ than per-thread, because we we set up the trap in the main thread, but`
		`+ event event handlers are processed in the thread where moksha runs MBSConsumer.`
		`+`
		`+ This is needed because @celery_app.task simply logs and ignores exceptions.`
		`+ """`
		`+ lock = threading.Lock()`
		`+ current = None`
		`+`
		`+ def __init__(self):`
		`+ self.exception = None`
		`+`
		`+ def __enter__(self):`
		`+ with EventTrap.lock:`
		`+ EventTrap.current = self`
		`+ return self`
		`+`
		`+ def __exit__(self, type, value, tb):`
		`+ with EventTrap.lock:`
		`+ EventTrap.current = None`
		`+`
		`+ @classmethod`
		`+ def set_exception(cls, exception):`
		`+ with cls.lock:`
		`+ if cls.current and not cls.current.exception:`
		`+ cls.current.exception = exception`
		`+`
		`+ @classmethod`
		`+ def get_exception(cls):`
		`+ with cls.lock:`
		`+ if cls.current:`
		`+ return cls.current.exception`
		`+ else:`
		`+ return None`
		`+`
		`+`
		`def mbs_event_handler(func):`
		`"""`
		`A decorator for MBS event handlers. It implements common tasks which should otherwise`
		`@@ -77,6 +117,9 @@`
		`def wrapper(args, *kwargs):`
		`try:`
		`return func(args, *kwargs)`
		`+ except Exception as e:`
		`+ EventTrap.set_exception(e)`
		`+ raise e`
		`finally:`
		`scheduler.run()`
		`# save origin function as functools.wraps from python2 doesn't preserve the signature`

tests/test_build/test_build.py

file modified

+49 -13

		`@@ -10,6 +10,7 @@`
		`from os.path import dirname`
		`import re`
		`import sched`
		`+ import signal`
		`from random import randint`
		`from shutil import copyfile`

		`@@ -83,6 +84,16 @@`
		`return stop_condition`


		`+ def make_trapped_stop_condition(stop_condition):`
		`+ def trapped_stop_condition(message):`
		`+ if events.EventTrap.get_exception():`
		`+ return True`
		`+`
		`+ return stop_condition(message)`
		`+`
		`+ return trapped_stop_condition`
		`+`
		`+`
		`def main(initial_messages, stop_condition):`
		`""" Run the consumer until some condition is met.`

		`@@ -108,18 +119,42 @@`

		`consumers = [module_build_service.scheduler.consumer.MBSConsumer]`

		`- # Note that the hub we kick off here cannot send any message. You`
		`- # should use fedmsg.publish(...) still for that.`
		`- moksha.hub.main(`
		`- # Pass in our config dict`
		`- options=config,`
		`- # Only run the specified consumers if any are so specified.`
		`- consumers=consumers,`
		`- # Do not run default producers.`
		`- producers=[],`
		`- # Tell moksha to quiet its logging.`
		`- framework=False,`
		`- )`
		`+ old_run = moksha.hub.reactor.reactor.run`
		`+ old_sigint_handler = signal.getsignal(signal.SIGINT)`
		`+`
		`+ def trap_sigint(self, *args):`
		`+ try:`
		`+ raise KeyboardInterrupt()`
		`+ except KeyboardInterrupt as e:`
		`+ events.EventTrap.set_exception(e)`
		`+`
		`+ def set_signals_and_run(args, *kwargs):`
		`+ signal.signal(signal.SIGINT, trap_sigint)`
		`+ try:`
		`+ old_run(args, *kwargs)`
		`+ finally:`
		`+ signal.signal(signal.SIGINT, old_sigint_handler)`
		`+`
		`+ with patch('moksha.hub.reactor.reactor.run', set_signals_and_run):`
		`+ # The events.EventTrap context handler allows us to detect exceptions`
		`+ # in event handlers and re-raise them here so that tests fail usefully`
		`+ # rather than just hang.`
		`+ with events.EventTrap() as trap:`
		`+ # Note that the hub we kick off here cannot send any message. You`
		`+ # should use fedmsg.publish(...) still for that.`
		`+ moksha.hub.main(`
		`+ # Pass in our config dict`
		`+ options=config,`
		`+ # Only run the specified consumers if any are so specified.`
		`+ consumers=consumers,`
		`+ # Do not run default producers.`
		`+ producers=[],`
		`+ # Tell moksha to quiet its logging.`
		`+ framework=False,`
		`+ )`
		`+`
		`+ if trap.exception:`
		`+ raise trap.exception`


		`class FakeSCM(object):`
		`@@ -388,9 +423,10 @@`
		`class BaseTestBuild:`

		`def run_scheduler(self, msgs=None, stop_condition=None):`
		`+ stop_condition = stop_condition or make_simple_stop_condition()`
		`main(`
		`msgs or [],`
		`- stop_condition or make_simple_stop_condition()`
		`+ make_trapped_stop_condition(stop_condition)`
		`)`

otaylor commented 3 years ago

This PR has two related patches to fix problems with tests/test_build. The first one makes exceptions occurred during test_build not hang the tests - this made the intermittent failures from the problem fixed in #1670 particularly annoying. The second makes Control-C work during and after the test_build tests.

test_build: exit rather than hanging on event handler exception

Event handlers decorated with @celery_app_task don't raise an exception - they just log the exception, leaving the Moksha hub running. This meant that any failures in test_build would result in the test suite hanging rather than failing usefully.

We solve this by adding a new class EventTrap which acts as a context manager. Any exceptions that occur in event handlers are set on the current EventTrap, and the test_build tests re-raise the exception.

test_build: leave Control-C working

Two problems occurred with the moksha/twisted handling of SIGINT:

While KeyboardInterrupt caused the moksha loop to exit, it just left the test in a confused state, instead of triggering standard pytest behavior and aborting the entire test run.
The handler was left-over for remaining tests that prevent Control-C from working at all.

Fix that by using mock.patch to override moksha's signal handler with our own signal handler that stores the KeyboardInterrupt in the current EventTrap, and restores the default signal handler after the loop ends.

Note that since the KeyboardInterrupt is always handled in the main thread, we don't get a useful backtrace from the child thread.

2 new commits added