bpo-35424: emit ResourceWarning at multiprocessing.Pool destruction #10974

vstinner · 2018-12-06T01:01:45Z

If a multiprocessing.Pool is not joined explicitly, it now emits a
ResourceWarning warning.

Add INIT state
Pool state now initialized to INIT and only set to RUN when
Pool.init() completes successfully.
Initialize some attributes earlier int Pool.init() to ensure
that they exist in Pool.del()

https://bugs.python.org/issue35424

pablogsal · 2018-12-06T11:14:33Z

Lib/multiprocessing/pool.py

Should this be stopped instead of joined?

I don't know how to describe the state here... close() or terminate() has been called. "was not joined" means that .join() should be called and it has not been called.

"stopped" implies that there is stop() method, no? But there is no stop() method.

Sorry, I meant close() instead of stop()

close() doesn't ensure that all resources are released. Currently, join() must be called to ensure that all processes and threads completed.

I thing close is more clearly, isn't?

Read previous comments.

As long as the user has called terminate or close (or used a context manager), a warning shouldn't be emitted.

pablogsal · 2018-12-06T11:15:26Z

Lib/multiprocessing/pool.py

Is this "#" on purpose?

It's on purpose. '#' means 'size' or 'count': it's the number of items (processes or threads) in the pool. It's not the right syntax?

I understand now. I think I was confused about searching for some other meaning for the '#'.

I can remove '#' if you prefer, I don't really care :-) Or rename to 'pool_size' maybe?

I am happy with the '#' :)

I'd rather have "pool size".

Lib/multiprocessing/pool.py

vstinner · 2018-12-07T09:50:40Z

tzickel:

This only works if you've called pool.close() or pool.terminate(). just deleting the pool references will not call to del due to a resource leak.

I wrote this PR more to ask if it's ok to start to emit ResourceWarning in the multiprocessing module. This module has multiple times, we cannot fix them all at once. This PR is a first step towards leaking less resources in corner cases / when the API is misused :-)

pitrou

I'd like to see the semantics changed a bit. Legitimate uses shouldn't see a warning. Also I notice you didn't add any tests.

pitrou · 2018-12-11T17:05:41Z

Lib/multiprocessing/pool.py

As long as the user has called terminate or close (or used a context manager), a warning shouldn't be emitted.

pitrou · 2018-12-11T17:06:00Z

Lib/multiprocessing/pool.py

I'd rather have "pool size".

bedevere-bot · 2018-12-11T17:07:12Z

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

vstinner · 2018-12-14T12:42:23Z

It's now unclear to me if we should promote "with pool:" by issuing a ResourceWarning when terminate() is not called explicitly.

See this discussion: [Python-Dev] Usage of the multiprocessing API and object lifetime.

multiprocessing.Pool destructor now emits ResourceWarning if the pool is still running.

vstinner · 2018-12-18T23:18:13Z

Since I created this PR, multiple changes have been merged:

test_del_pool() has been removed
I added Pool.repr() and state constants are now strings
I fixed all multiprocessing tests to call .terminate() (use a context manager) and .join()

I rewrote this PR to rebase it on master.

I simplified my PR to only emit a warning if the pool is still running, but no longer if .join() has not been called.

@pitrou: Would you mind to have a new look at this change? I added an unit test :-)

pitrou · 2018-12-19T17:09:06Z

Lib/test/_test_multiprocessing.py

+        pool.terminate()
+        pool.join()
+
+        # force state to RUN to emit ResourceWarning in __del__()


Yuck. Why can't you test the actual situation? (start a pool then delete it)

If you don't cleanup properly a pool, bad things happens. You can leak processes and threads and it's not easy to write a test which wait properly until they complete. If the test doesn't wait until they complete, the test can be marked as ENV_CHANGED and the whole test suite fails on buildbots. Leaking threads/processes have side effects on following tests which cause random failures. There is a "coverage" job on Travis CI which runs the full test suite sequentially for example.

Well, there was a PR to clean up pools properly, but AFAIR you reverted it...

I reverted https://bugs.python.org/issue34172 change because it introduced a backward incompatible change in stable branches. I also reverted the change in the master branch to open a more general discussion: https://mail.python.org/pipermail/python-dev/2018-December/155946.html

The reverted change isn't magic: it doesn't fix all issues. Even with this fix, the destructor still doesn't join many threads and so regrtest will still complain. The .join() method has to be called explicitly to really make sure that a test doesn't leak resources.

The reverted change had a test on "del pool" and this test caused issues on buildbots because it leaked resources (processes, threads). IMHO testing that the destructor calls terminate() deserves its own separated test.

In this PR, the test is restricted to check that the destructor emits a ResourceWarning.

Moreover, to come back to https://bugs.python.org/issue34172 maybe you haven't noticed, by this PR should also help https://bugs.python.org/issue34172 because the problematic example ("for x in multiprocessing.Pool().imap(int, ["4", "3"]): print(x)") should now emit a ResourceWarning (once the reference cycle will be broken again). It's better than nothing to warn developers that something is wrong :-) See also https://bugs.python.org/issue35378

... Sorry for the disgression, but it seems like you asked for it :-)

Do you want me to change the test, or is it ok written like that, with the explanation in my previous comment?

Fair enough, let's keep it like that :-)

pitrou · 2018-12-19T17:09:55Z

Lib/multiprocessing/pool.py

+        self._state = RUN
+
+    def __del__(self, _warn=warnings.warn):
+        if self._state == RUN:


Shouldn't you freeze RUN in the __del__ parameters as well?

vstinner · 2018-12-20T19:34:05Z

Thanks for the review @pitrou!

vstinner added the DO-NOT-MERGE label Dec 6, 2018

the-knights-who-say-ni added the CLA signed label Dec 6, 2018

bedevere-bot added the awaiting merge label Dec 6, 2018

pablogsal reviewed Dec 6, 2018

View reviewed changes

tzickel reviewed Dec 6, 2018

View reviewed changes

Lib/multiprocessing/pool.py Outdated Show resolved Hide resolved

vstinner changed the title ~~[WIP] bpo-35424: multiprocessing.Pool emits ResourceWarning~~ bpo-35424: multiprocessing.Pool emits ResourceWarning Dec 11, 2018

vstinner removed the DO-NOT-MERGE label Dec 11, 2018

vstinner requested a review from pitrou December 11, 2018 16:29

pitrou requested changes Dec 11, 2018

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting merge labels Dec 11, 2018

bpo-35424: multiprocessing.Pool emits ResourceWarning

b510934

multiprocessing.Pool destructor now emits ResourceWarning if the pool is still running.

pitrou requested changes Dec 19, 2018

View reviewed changes

__del__() copies also RUN global

fe77e45

renosbest approved these changes Dec 19, 2018

View reviewed changes

pitrou changed the title ~~bpo-35424: multiprocessing.Pool emits ResourceWarning~~ bpo-35424: emit ResourceWarning at multiprocessing.Pool destruction Dec 20, 2018

pitrou approved these changes Dec 20, 2018

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting changes labels Dec 20, 2018

vstinner merged commit 9a8d1d7 into python:master Dec 20, 2018

bedevere-bot removed the awaiting merge label Dec 20, 2018

vstinner deleted the mp_pool_reswarn branch December 20, 2018 19:33

Uh oh!

bpo-35424: emit ResourceWarning at multiprocessing.Pool destruction #10974

bpo-35424: emit ResourceWarning at multiprocessing.Pool destruction #10974

Uh oh!

Conversation

vstinner commented Dec 6, 2018 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vstinner commented Dec 7, 2018

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Dec 11, 2018

Uh oh!

vstinner commented Dec 14, 2018

Uh oh!

vstinner commented Dec 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner commented Dec 20, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

vstinner commented Dec 6, 2018 •

edited by bedevere-bot

Loading