GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

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

Conversation

@JulienPalard

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

Uh oh!

There was an error while loading. Please reload this page.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Feb 10, 2018
@bedevere-bot bedevere-bot added the awaiting merge label Feb 10, 2018
pitrou
pitrou reviewed Mar 13, 2018
View reviewed changes
Comment thread Lib/multiprocessing/pool.py Outdated

@pitrou pitrou Mar 13, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@JulienPalard JulienPalard Apr 2, 2018

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
pitrou
pitrou reviewed Mar 13, 2018
View reviewed changes
Comment thread Lib/multiprocessing/pool.py Outdated

@pitrou pitrou Mar 13, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@JulienPalard JulienPalard Apr 2, 2018

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
pitrou
pitrou requested changes Mar 13, 2018
View reviewed changes

@pitrou pitrou left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added awaiting changes and removed awaiting merge labels Mar 13, 2018
@pitrou pitrou added needs backport to 3.6 labels Mar 13, 2018
@JulienPalard
JulienPalard force-pushed the bpo-19675 branch from 4a99c0a to 6f973af Compare April 2, 2018 09:52
@JulienPalard
bpo-19675: Terminate processes if construction of a pool is failing.
53c89fb
@JulienPalard
JulienPalard force-pushed the bpo-19675 branch from 6f973af to 53c89fb Compare April 2, 2018 09:53
@JulienPalard

JulienPalard commented Apr 2, 2018

Copy link
Copy Markdown
Member Author

I have made the requested changes; please review again.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added awaiting change review and removed awaiting changes labels Apr 2, 2018
@bedevere-bot

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
pitrou
pitrou approved these changes Apr 2, 2018
View reviewed changes
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting change review labels Apr 2, 2018
@pitrou

pitrou commented Apr 2, 2018

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@JulienPalard

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?).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@JulienPalard
bpo-19675: Add a test.
a8b3924
@JulienPalard

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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
pitrou
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 pitrou Apr 13, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Why not assertRaises?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@pitrou

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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
taleinat added 2 commits September 10, 2018 21:54
@taleinat
Merge remote-tracking branch 'upstream/master' into bpo-19675
3bdbf79 
# Conflicts:
#	Lib/test/_test_multiprocessing.py
@taleinat
bpo-19675: clean up and fix typos in test and NEWS
f569cae
@taleinat

taleinat commented Sep 10, 2018

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@matrixise

matrixise commented Nov 4, 2018

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@taleinat
Merge remote-tracking branch 'upstream/master' into bpo-19675
7670d4a 
# Conflicts:
#	Lib/multiprocessing/pool.py
@taleinat

taleinat commented Nov 4, 2018

Copy link
Copy Markdown
Contributor

@matrixise, merged again.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@matrixise

matrixise commented Nov 4, 2018

Copy link
Copy Markdown
Member

Thanks @taleinat
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@JulienPalard

JulienPalard commented Nov 4, 2018

Copy link
Copy Markdown
Member Author

Oh thanks for bringing it back, forgot it þ
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@JulienPalard
JulienPalard merged commit 5d236ca into python:master Nov 4, 2018
@miss-islington

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.
🐍🍒⛏🤖
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the awaiting merge label Nov 4, 2018
@miss-islington

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@miss-islington miss-islington assigned JulienPalard Nov 4, 2018
@miss-islington

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@JulienPalard JulienPalard mentioned this pull request Nov 4, 2018
bpo-19675: Fixes pool cleanup issue #57
Closed
@JulienPalard
JulienPalard deleted the bpo-19675 branch June 16, 2019 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pitrou pitrou pitrou approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@JulienPalard JulienPalard

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@JulienPalard @bedevere-bot @pitrou @taleinat @matrixise @miss-islington @the-knights-who-say-ni
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.