Skip to content

bpo-38501: Add a warning section to multiprocessing.Pool docs about resource managing #19466

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 11, 2020

Conversation

pablogsal
Copy link
Member

@pablogsal pablogsal commented Apr 11, 2020


Note that is **not correct** to rely on the garbage colletor to destroy the pool
as CPython does not assure that the finalizer of the pool will be called
(see :meth:`object.__del__` for more information).
Copy link
Contributor

Choose a reason for hiding this comment

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

My potential fix is to use atexit.register when we can't use the context manager. Worth suggesting here? (Also let me know if it's clearly a bad idea for some reason!)

Copy link
Member Author

@pablogsal pablogsal Apr 11, 2020

Choose a reason for hiding this comment

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

My potential fix is to use atexit.register when we can't use the context manager. Worth suggesting here? (Also let me know if it's clearly a bad idea for some reason!)

That works, but the problem is that is still a lossy method of resource managing (you would not use that method for all your file objects for instance). Technically, someone that is not "the interpreter" should own the pool and should be in charge of destroying it correctly.

I understand that some architecture and designs make this challenging so things like atexit do work for those cases, but I think is better not to explicitly mention that here so users don't start to use that as a silver bullet and use more the context manager for instance.

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah that makes sense. Otherwise, text LGTM and reads nicely in the rendered artifact!

@pablogsal pablogsal merged commit 7ec43a7 into python:master Apr 11, 2020
@miss-islington
Copy link
Contributor

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

@pablogsal pablogsal deleted the bpo-38501 branch April 11, 2020 02:05
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Apr 11, 2020
…esource managing (pythonGH-19466)

(cherry picked from commit 7ec43a7)

Co-authored-by: Pablo Galindo <[email protected]>
@bedevere-bot
Copy link

GH-19467 is a backport of this pull request to the 3.8 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Apr 11, 2020
…esource managing (pythonGH-19466)

(cherry picked from commit 7ec43a7)

Co-authored-by: Pablo Galindo <[email protected]>
@bedevere-bot
Copy link

GH-19468 is a backport of this pull request to the 3.7 branch.

miss-islington added a commit that referenced this pull request Apr 11, 2020
…esource managing (GH-19466)

(cherry picked from commit 7ec43a7)

Co-authored-by: Pablo Galindo <[email protected]>
miss-islington added a commit that referenced this pull request Apr 11, 2020
…esource managing (GH-19466)

(cherry picked from commit 7ec43a7)

Co-authored-by: Pablo Galindo <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation in the Doc dir skip news
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants