bpo-19675: Terminate processes if construction of a pool is failing. #5614
Conversation
Lib/multiprocessing/pool.py
Outdated
| self._repopulate_pool() | ||
| try: | ||
| self._repopulate_pool() | ||
| except OSError: |
There was a problem hiding this comment.
Simply except Exception sounds ok to me.
There was a problem hiding this comment.
Sounds OK, it does not make sense to avoid leaking process only on known exceptions, and we're not hiding it.
Lib/multiprocessing/pool.py
Outdated
| if p.exitcode is None: | ||
| p.terminate() | ||
| for p in self._pool: | ||
| if p.is_alive(): |
There was a problem hiding this comment.
I don't think you need the process to be alive to join it.
There was a problem hiding this comment.
Look ok not to test it first.
There was a problem hiding this comment.
Thanks for the PR @JulienPalard. See the comments I posted.
|
When you're done making the requested changes, leave the comment: |
|
I have made the requested changes; please review again. |
|
Thanks for making the requested changes! @pitrou: please review the changes made to this pull request. |
|
Thank you @JulienPalard. Do you think you could easily add a test? |
|
@pitrou I'm testing it by changing rlimit, by throwing random "low" value at it until "some" forks succeed and some are failing. It's really bad for a test case, I can try to produce a stable test case, maybe not with rlimit though (will probably be random when running parallel tests?). |
|
@pitrou Tried to write a unit test. It works but I'm not fond of playing with context this way. Does it look OK for someone? |
Lib/test/_test_multiprocessing.py
Outdated
| def is_alive(self): | ||
| return self.state == 'started' | ||
|
|
||
| try: |
The MagicMock thing looks fragile to me. Perhaps you can use a real context and temporarily swap out its |
# Conflicts: # Lib/test/_test_multiprocessing.py
|
Wouldn't this need to be backported to 2.7 as well? |
|
@JulienPalard maybe you could update it with the last master and submit it again. |
# Conflicts: # Lib/multiprocessing/pool.py
|
@matrixise, merged again. |
|
Thanks @taleinat |
|
Oh thanks for bringing it back, forgot it þ |
|
Thanks @JulienPalard for the PR 🌮🎉.. I'm working now to backport this PR to: 3.6, 3.7. |
|
Sorry, @JulienPalard, I could not cleanly backport this to |
|
Sorry, @JulienPalard, I could not cleanly backport this to |
Trying another implementation of #57, avoiding to terminate all processes of a pool if a single one is failing to fork during the normal execution of the pool.
https://bugs.python.org/issue19675