bpo-19675: Terminate processes if construction of a pool is failing. by JulienPalard · Pull Request #5614 · python/cpython
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simply except Exception sounds ok to me.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds OK, it does not make sense to avoid leaking process only on known exceptions, and we're not hiding it.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you need the process to be alive to join it.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look ok not to test it first.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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?
| def is_alive(self): | ||
| return self.state == 'started' | ||
|
|
||
| try: |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not assertRaises?
It works but I'm not fond of playing with context this way. Does it look OK for someone?
The MagicMock thing looks fragile to me. Perhaps you can use a real context and temporarily swap out its Process attribute?
# 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
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 3.7 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 5d236cafd7126e640fb25541fcc7e0a494450143 3.7
Sorry, @JulienPalard, I could not cleanly backport this to 3.6 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 5d236cafd7126e640fb25541fcc7e0a494450143 3.6