Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions Lib/concurrent/futures/process.py
Original file line number Diff line number Diff line change
Expand Up @@ -477,6 +477,15 @@ def _terminate_broken(self, cause):
if cause is not None:
bpe.__cause__ = _RemoteTraceback(
f"\n'''\n{''.join(cause)}'''")
else:
# No cause known, so try to report some helpful info about
# which process(es) terminated and with what exit code
errors = []
for p in self.processes.values():
if p.exitcode: # Report any nonzero exit codes
errors.append(f"Process {p.pid} terminated abruptly with exit code {p.exitcode}")
if errors:
bpe.__cause__ = _RemoteTraceback("\n".join(errors))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

without having swapped in my context on this code... the above cause is not None case surrounds the value within with \n''' joined_value ''' - should we do the same?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

another way to think about this... can this logic just set cause to be this errors list so there's only a single _RemoteTraceback construction?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call - initially I was just aiming to modify as little code as possible 😄 but you're right that this is worth a small refactor. Let me know what you think.

Only possible downside is that now the traceback looks a little more funky. Do you know why we have the ''' and the newlines this way?

Lib.concurrent.futures.process._RemoteTraceback: 
'''
Process 54534 terminated abruptly with exit code 99'''

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should add that I'm more than happy to keep the formatting like this. Consistency is good, and I'm sure that there is at least one user out there directly parsing _RemoteTraceback strings whose use case would break if we made any modifications :)

Just want to confirm that this all looks good to you.


# Mark pending tasks as failed.
for work_id, work_item in self.pending_work_items.items():
Expand Down
18 changes: 18 additions & 0 deletions Lib/test/test_concurrent_futures/test_process_pool.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import ctypes
import os
import queue
import sys
Expand Down Expand Up @@ -106,6 +107,23 @@ def test_traceback(self):
self.assertIn('raise RuntimeError(123) # some comment',
f1.getvalue())

@staticmethod
def _segfault():
ctypes.string_at(0)

def test_traceback_when_child_process_segfaults(self):
# gh-139462 enhancement - BrokenProcessPool exceptions
# should describe which process terminated.
future = self.executor.submit(self._segfault)
with self.assertRaises(Exception) as cm:
future.result()

bpe = cm.exception
self.assertIs(type(bpe), BrokenProcessPool)
cause = bpe.__cause__
self.assertIs(type(cause), futures.process._RemoteTraceback)
self.assertIn("terminated abruptly with exit code", cause.tb)

@warnings_helper.ignore_fork_in_thread_deprecation_warnings()
@hashlib_helper.requires_hashdigest('md5')
def test_ressources_gced_in_workers(self):
Expand Down
Loading