gh-132063: ProcessPoolExecutor swallows falsy Exceptions #132129
Conversation
| @@ -0,0 +1 @@ | |||
| Fix tests of exception attribute in :method:`process_result_item`of :class:`_ExecutorManagerThread` and :method:`__get_result` of :class:`Future` in module :lib:`concurrent`. | |||
There was a problem hiding this comment.
Love the effort, but this is too many technical details for a blurb entry.
| Fix tests of exception attribute in :method:`process_result_item`of :class:`_ExecutorManagerThread` and :method:`__get_result` of :class:`Future` in module :lib:`concurrent`. | |
| Prevent exceptions that evaluate as falsey from being ignored | |
| by :class:`concurrent.futures.ProcessPoolExecutor`. |
There was a problem hiding this comment.
This is not limited to __bool__, it can be anything that is falsey, including a class which implements __len__ as well (https://docs.python.org/3/reference/datamodel.html#object.__bool__)
| class MyException(Exception): | ||
| def __bool__(self): | ||
| return False | ||
|
|
||
| @classmethod | ||
| def raiser(cls): | ||
| raise cls.MyException("foo") |
There was a problem hiding this comment.
We should isolate these to the test case, not the test class. It should be something like this:
def test_swallows_falsy_exceptions(self):
class MyException(Exception):
...
def raiser():
raise MyException(...)
...There was a problem hiding this comment.
Can you also add a class which does not specify __bool__ but specifies __len__ and returns 0? TiA
| raise cls.MyException("foo") | ||
|
|
||
| def test_swallows_falsy_exceptions(self): | ||
| # fix gh-132063 issue |
There was a problem hiding this comment.
Could you give a brief description of the problem in this comment?
| def test_swallows_falsy_exceptions(self): | ||
| # fix gh-132063 issue | ||
| with self.assertRaisesRegex(self.MyException, "foo"): | ||
| with self.executor_type(max_workers=1) as executor: |
There was a problem hiding this comment.
Also, this test case should go in the ExecutorTest base class.
| @@ -112,6 +112,19 @@ def log_n_wait(ident): | |||
| # ident='third' is cancelled because it remained in the collection of futures | |||
| self.assertListEqual(log, ["ident='first' started", "ident='first' stopped"]) | |||
|
|
|||
| class MyException(Exception): | |||
There was a problem hiding this comment.
Yeah, we don't need to repeat the test case. Let's move it to ExecutorTest.
|
All commit authors signed the Contributor License Agreement. |
| @@ -0,0 +1 @@ | |||
| Fix tests of exception attribute in :method:`process_result_item`of :class:`_ExecutorManagerThread` and :method:`__get_result` of :class:`Future` in module :lib:`concurrent`. | |||
There was a problem hiding this comment.
This is not limited to __bool__, it can be anything that is falsey, including a class which implements __len__ as well (https://docs.python.org/3/reference/datamodel.html#object.__bool__)
| class MyException(Exception): | ||
| def __bool__(self): | ||
| return False | ||
|
|
||
| @classmethod | ||
| def raiser(cls): | ||
| raise cls.MyException("foo") |
There was a problem hiding this comment.
Can you also add a class which does not specify __bool__ but specifies __len__ and returns 0? TiA
ProcessPoolExecutor and ThreadPoolExecutor swallows falsy exception.
Fix tests about exception variable in 2 files of /Lib/concurrent/futures
Changes 2 tests as below:
Replace
if exception:withif exception is not None: