Cleanups not always called when handling KeyboardInterrupt

[bbatliner]

From my testing, it seems that if a KeyboardInterrupt occurs during cleanup of a scenario: https://github.com/behave/behave/blob/61781ede2e701aec6dc0314478ab52a58b2e09b0/behave/runner.py#L226-L228

Then there are two negative consequences:

1. Not all cleanup functions for the scenario will get called (except Exception does not catch KeyboardInterrupt and aborts the loop)
2. The testrun layer of the context does not have its @cleanups run.

Problem 1 can be solved by changing Exception to BaseException (simple) so that the loop doesn't terminate, though the cleanup_func can still be interrupted:

diff --git a/behave/runner.py b/behave/runner.py
index 1f32ec1..5e5c4f5 100644
--- a/behave/runner.py
+++ b/behave/runner.py
@@ -225,7 +225,7 @@ class Context(object):
         for cleanup_func in reversed(cleanup_funcs):
             try:
                 cleanup_func()
-            except Exception as e: # pylint: disable=broad-except
+            except BaseException as e: # pylint: disable=broad-except
                 # pylint: disable=protected-access
                 context._root["cleanup_errors"] += 1
                 cleanup_errors.append(sys.exc_info())

OR by writing more complex logic to disable/defer SIGINT while cleanups are running so that they are uninterruptible.

I will need to monkey patch Behave to solve Problem 1 for now.

Problem 2 can be solved in user space with the following after_all check:

def after_all(context):
    while len(context._stack) > 1:
        context._pop()

OR by explicitly _poping the feature layer from the context stack when a KeyboardInterrupt is received (simple):

diff --git a/behave/runner.py b/behave/runner.py
index 1f32ec1..4461d78 100644
--- a/behave/runner.py
+++ b/behave/runner.py
@@ -698,6 +698,9 @@ class ModelRunner(object):
                     self.aborted = True
                     failed_count += 1
                     run_feature = False
+                    # -- Pop the feature layer if interrupted before feature.run() did so
+                    if self.context._stack[0].get("@layer", "") == "feature":
+                        self.context._stack.pop()
 
             # -- ALWAYS: Report run/not-run feature to reporters.
             # REQUIRED-FOR: Summary to keep track of untested features.

OR by rewriting context._push to return a context manager that guarantees that the stack entry will be popped in a finally block (more complex/requires more code change).

I am curious what you think is the best way forward here.

[jenisys]

NOTES (related to some of the comments/descriptions above):

• Suppressing KeyboardInterrupt exception(s) in general is a bad idea in my opinion
• I may add code that suppresses the KeyboardInterrupt a few times while performing cleanups but not always (see below)
• Same for using BaseException as replacement for Exception
• You can always use contextlib.supress() in your cleanup functions, like:

from contextlib import suppress

def my_cleanup_function():
    with suppress(KeyboardInterrupt):
        # -- Whatever is needed to perform the cleanup 
        #    (without getting interrupted by KeyboardInterrupt exception).
        ...

• But then you must also live with the pain points there
• You can better judge for your code what is more important for your use case

REASONS:

• When a user hits <CONTROL-C> key combination (or similar) to interrupt a running program, he/she wants to interrupt/terminate the program
• If you prevent/suppress this functionality, you get frustrated users, because she/he cannot do what she/he wants to do and will finally perform a forced-kill of the program
• Suppressing the KeyboardInterrupt only a few times (2x or 3x) is completely different issues (especially if it triggers output that this functionality is currently suppressed).

SEE ALSO:

• Python docs: contextlib.suppress()