EXECUTOR_PROPAGATE_EXCEPTIONS does not propagate exceptions

[okke-formsma]

Futures will swallow all exceptions thrown by callbacks added to the future using the _done_callbacks:

# concurrent.futures._base
class Future(object):
    """Represents the result of an asynchronous computation."""
    # ... 
    def _invoke_callbacks(self):
        for callback in self._done_callbacks:
            try:
                callback(self)
            except Exception:
                LOGGER.exception('exception calling callback for %r', self)

The tests pass because the propagate_exceptions_callback is called explicitly. However, this bypasses the configuration setting completely.

def test_propagate_exception_callback(app, caplog):
    caplog.set_level(logging.ERROR)
    app.config['EXECUTOR_PROPAGATE_EXCEPTIONS'] = True # the test succeeds even if this is false
    executor = Executor(app)
    with pytest.raises(NameError):
        with app.test_request_context('/'):
            future = executor.submit(fail)
            assert propagate_exceptions_callback in future._done_callbacks
            concurrent.futures.wait([future])
            propagate_exceptions_callback(future) # without this line the test fails

To ensure an exception will be raised when the thread had an exception, just access the future's result:

future.result()

I think it is impossible to have other threads interrupt the main thread when they encounter an exception.

[RuneStone0]

+1

[Zhenshan-Jin]

+1

[dchevell]

This is a good catch and a bit of a fail on my end:

• The callback effectively logs the exception, so it seems to appear
• If the callable has already finished, adding the propagate exceptions callback causes it to be returned immediately, which does raise the exception rather than swallowing it
• The test here is faulty

I'll look at this one and see what else I can do. Had an idea about throwing BaseException, pytest doesn't seem to like it but I'll apply some real-world tests first and backsolve for pytest if it works. If not, this will need some more complex machinery to track and store futures so exceptions can be thrown in the calling thread.

[dchevell]

Alright, going back to this after a long time I think this is actually a misunderstanding of the intended functionality here. That's in large part because my original description of this in the README isn't clear enough, and because my original tests for it were faulty.

The original intention here is in fact to simply get the exception logged, because otherwise the failure would be invisible. An exception occurring directly within a route would normally lead to a 500 error, but that can't be possible when we're talking about background tasks because by definition they may still be running after the request has already completed. So enabling this option ensures that even in that scenario, exceptions are caught and logged, rather than swallowed silently.

It wouldn't make a lot of sense to try and halt the entire app, either; exceptions directly within routes don't cause that so it would seem strange for a background task to do so

I'll leave this open a little longer just in case anyone wants to share more about their expected behaviour