From 1642f0ec016404c2d8e8c306bb87499bae0caa78 Mon Sep 17 00:00:00 2001 From: Sam Martin Date: Mon, 6 May 2019 23:22:55 +0100 Subject: [PATCH 1/3] bpo-33110: Catches errors raised when running add_done_callback on already completed futures. Wraps the callback call within the `add_done_callback` function within concurrent.futures, in order to behave in an identical manner to callbacks added to a running future are triggered once it has completed. --- Lib/concurrent/futures/_base.py | 5 ++++- Lib/test/test_concurrent_futures.py | 17 +++++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/Lib/concurrent/futures/_base.py b/Lib/concurrent/futures/_base.py index ea16eef841c518..ef61bb2bf8ad61 100644 --- a/Lib/concurrent/futures/_base.py +++ b/Lib/concurrent/futures/_base.py @@ -404,7 +404,10 @@ def add_done_callback(self, fn): if self._state not in [CANCELLED, CANCELLED_AND_NOTIFIED, FINISHED]: self._done_callbacks.append(fn) return - fn(self) + try: + fn(self) + except Exception: + LOGGER.exception('exception calling callback for %r', self) def result(self, timeout=None): """Return the result of the call that the future represents. diff --git a/Lib/test/test_concurrent_futures.py b/Lib/test/test_concurrent_futures.py index 903afbd2a4f68a..d2686f8b953532 100644 --- a/Lib/test/test_concurrent_futures.py +++ b/Lib/test/test_concurrent_futures.py @@ -1080,6 +1080,23 @@ def fn(callback_future): f.add_done_callback(fn) self.assertTrue(was_cancelled) + def test_done_callback_raises_already_succeeded(self): + with test.support.captured_stderr() as stderr: + def raising_fn(callback_future): + raise Exception('doh!') + + f = Future() + + # Set the result first to simulate a future that runs instantly, + # effectively allowing the callback to be run immediately. + f.set_result(5) + f.add_done_callback(raising_fn) + + # Verify the callback exception was caught + self.assertIn('exception calling callback for', stderr.getvalue()) + self.assertIn('doh!', stderr.getvalue()) + + def test_repr(self): self.assertRegex(repr(PENDING_FUTURE), '') From 7585a094d28a27be51f69dcf419b411a880855c3 Mon Sep 17 00:00:00 2001 From: "blurb-it[bot]" Date: Mon, 6 May 2019 22:34:48 +0000 Subject: [PATCH 2/3] =?UTF-8?q?=F0=9F=93=9C=F0=9F=A4=96=20Added=20by=20blu?= =?UTF-8?q?rb=5Fit.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../NEWS.d/next/Library/2019-05-06-22-34-47.bpo-33110.rSJSCh.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 Misc/NEWS.d/next/Library/2019-05-06-22-34-47.bpo-33110.rSJSCh.rst diff --git a/Misc/NEWS.d/next/Library/2019-05-06-22-34-47.bpo-33110.rSJSCh.rst b/Misc/NEWS.d/next/Library/2019-05-06-22-34-47.bpo-33110.rSJSCh.rst new file mode 100644 index 00000000000000..3e94163fee7e21 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2019-05-06-22-34-47.bpo-33110.rSJSCh.rst @@ -0,0 +1 @@ +Adds error handling to concurrent.futures add_done_callback, so that callbacks triggered on futures added to already completed futures are handled in the same manner as callbacks attached to running futures are, when the future subsequently finishes. \ No newline at end of file From d4993c9dd3e21ea95b7acc0879c93d98e2b39426 Mon Sep 17 00:00:00 2001 From: Sam Martin Date: Tue, 7 May 2019 16:11:20 +0100 Subject: [PATCH 3/3] Apply code review comments. --- Lib/test/test_concurrent_futures.py | 1 - .../next/Library/2019-05-06-22-34-47.bpo-33110.rSJSCh.rst | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/Lib/test/test_concurrent_futures.py b/Lib/test/test_concurrent_futures.py index d2686f8b953532..be03089cd01915 100644 --- a/Lib/test/test_concurrent_futures.py +++ b/Lib/test/test_concurrent_futures.py @@ -1092,7 +1092,6 @@ def raising_fn(callback_future): f.set_result(5) f.add_done_callback(raising_fn) - # Verify the callback exception was caught self.assertIn('exception calling callback for', stderr.getvalue()) self.assertIn('doh!', stderr.getvalue()) diff --git a/Misc/NEWS.d/next/Library/2019-05-06-22-34-47.bpo-33110.rSJSCh.rst b/Misc/NEWS.d/next/Library/2019-05-06-22-34-47.bpo-33110.rSJSCh.rst index 3e94163fee7e21..f1e2460c4927c8 100644 --- a/Misc/NEWS.d/next/Library/2019-05-06-22-34-47.bpo-33110.rSJSCh.rst +++ b/Misc/NEWS.d/next/Library/2019-05-06-22-34-47.bpo-33110.rSJSCh.rst @@ -1 +1 @@ -Adds error handling to concurrent.futures add_done_callback, so that callbacks triggered on futures added to already completed futures are handled in the same manner as callbacks attached to running futures are, when the future subsequently finishes. \ No newline at end of file +Handle exceptions raised by functions added by concurrent.futures add_done_callback correctly when the Future has already completed.