FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

bpo-19675: Terminate processes if construction of a pool is failing. by JulienPalard · Pull Request #5614 · python/cpython · GitHub

/ cpython Public

bpo-19675: Terminate processes if construction of a pool is failing. - #5614

Merged
JulienPalard merged 5 commits into
python:masterfrom
JulienPalard:bpo-19675
Nov 4, 2018
ConversationCommits (5)ChecksFiles changed
Merged

bpo-19675: Terminate processes if construction of a pool is failing.#5614
JulienPalard merged 5 commits into
python:masterfrom
JulienPalard:bpo-19675

Conversation

JulienPalard commented Feb 10, 2018 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

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

pitrou reviewed Mar 13, 2018
View reviewed changes
Comment thread Lib/multiprocessing/pool.py Outdated

pitrou Mar 13, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Simply except Exception sounds ok to me.

JulienPalard Apr 2, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Sounds OK, it does not make sense to avoid leaking process only on known exceptions, and we're not hiding it.

pitrou reviewed Mar 13, 2018
View reviewed changes
Comment thread Lib/multiprocessing/pool.py Outdated

pitrou Mar 13, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I don't think you need the process to be alive to join it.

JulienPalard Apr 2, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Look ok not to test it first.

pitrou requested changes Mar 13, 2018
View reviewed changes

pitrou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks for the PR @JulienPalard. See the comments I posted.

bedevere-bot commented Mar 13, 2018

Copy link
Copy Markdown

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

JulienPalard commented Apr 2, 2018

Copy link
Copy Markdown
Member Author

I have made the requested changes; please review again.

bedevere-bot commented Apr 2, 2018

Copy link
Copy Markdown

Thanks for making the requested changes!

@pitrou: please review the changes made to this pull request.

pitrou commented Apr 2, 2018

Copy link
Copy Markdown
Member

Thank you @JulienPalard. Do you think you could easily add a test?

JulienPalard commented Apr 4, 2018

Copy link
Copy Markdown
Member Author

@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?).

JulienPalard commented Apr 7, 2018

Copy link
Copy Markdown
Member Author

@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?

pitrou reviewed Apr 13, 2018
View reviewed changes
Comment thread Lib/test/_test_multiprocessing.py Outdated
def is_alive(self):
return self.state == 'started'

try:

pitrou Apr 13, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Why not assertRaises?

pitrou commented Apr 13, 2018

Copy link
Copy Markdown
Member

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?

taleinat commented Sep 10, 2018

Copy link
Copy Markdown
Contributor

Wouldn't this need to be backported to 2.7 as well?

matrixise commented Nov 4, 2018

Copy link
Copy Markdown
Member

@JulienPalard maybe you could update it with the last master and submit it again.

taleinat commented Nov 4, 2018

Copy link
Copy Markdown
Contributor

@matrixise, merged again.

matrixise commented Nov 4, 2018

Copy link
Copy Markdown
Member

Thanks @taleinat

JulienPalard commented Nov 4, 2018

Copy link
Copy Markdown
Member Author

Oh thanks for bringing it back, forgot it þ

JulienPalard merged commit 5d236ca into python:master Nov 4, 2018

miss-islington commented Nov 4, 2018

Copy link
Copy Markdown
Contributor

Thanks @JulienPalard for the PR 🌮🎉.. I'm working now to backport this PR to: 3.6, 3.7.
🐍🍒⛏🤖

miss-islington commented Nov 4, 2018

Copy link
Copy Markdown
Contributor

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

miss-islington commented Nov 4, 2018

Copy link
Copy Markdown
Contributor

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

JulienPalard mentioned this pull request Nov 4, 2018
Closed
JulienPalard deleted the bpo-19675 branch June 16, 2019 14:01
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

pitrou pitrou approved these changes

Assignees

JulienPalard

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL