From 4ddd3c2637aee52a39a70dae88f163024fee7699 Mon Sep 17 00:00:00 2001 From: "Owen W. Taylor" Date: Fri, 22 Jan 2021 15:11:04 -0500 Subject: [PATCH 1/2] 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. --- module_build_service/scheduler/events.py | 43 +++++++++++++++++++++++ tests/test_build/test_build.py | 44 +++++++++++++++++------- 2 files changed, 74 insertions(+), 13 deletions(-) diff --git a/module_build_service/scheduler/events.py b/module_build_service/scheduler/events.py index b02dc7bf..bb56c04a 100644 --- a/module_build_service/scheduler/events.py +++ b/module_build_service/scheduler/events.py @@ -15,6 +15,7 @@ however Brew sends to topic brew.build.complete, etc. 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 @@ class Scheduler(sched.scheduler): 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 mbs_event_handler(func): 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 diff --git a/tests/test_build/test_build.py b/tests/test_build/test_build.py index 1f8c5152..a7e3a3f2 100644 --- a/tests/test_build/test_build.py +++ b/tests/test_build/test_build.py @@ -83,6 +83,16 @@ def make_simple_stop_condition(): 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 +118,25 @@ def main(initial_messages, stop_condition): 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, - ) + # 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 +405,10 @@ def cleanup_moksha(): 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) ) From 0d0d8091ac06e91d2409bcd4d7ecfc90ac13e45a Mon Sep 17 00:00:00 2001 From: "Owen W. Taylor" Date: Fri, 22 Jan 2021 15:12:10 -0500 Subject: [PATCH 2/2] 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. --- tests/test_build/test_build.py | 54 ++++++++++++++++++++++------------ 1 file changed, 36 insertions(+), 18 deletions(-) diff --git a/tests/test_build/test_build.py b/tests/test_build/test_build.py index a7e3a3f2..df5fb56a 100644 --- a/tests/test_build/test_build.py +++ b/tests/test_build/test_build.py @@ -10,6 +10,7 @@ from os import path, mkdir from os.path import dirname import re import sched +import signal from random import randint from shutil import copyfile @@ -118,25 +119,42 @@ def main(initial_messages, stop_condition): consumers = [module_build_service.scheduler.consumer.MBSConsumer] - # 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, - ) + old_run = moksha.hub.reactor.reactor.run + old_sigint_handler = signal.getsignal(signal.SIGINT) - if trap.exception: - raise trap.exception + 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):