Skip to content

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

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.

Sign up for GitHub

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
vstinner merged 2 commits into from
Dec 20, 2018
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b510934
bpo-35424: multiprocessing.Pool emits ResourceWarning
vstinner Dec 18, 2018
fe77e45
__del__() copies also RUN global
vstinner Dec 19, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 18 additions & 5 deletions Lib/multiprocessing/pool.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,14 @@
# Imports
#

import threading
import queue
import itertools
import collections
import itertools
import os
import queue
import threading
import time
import traceback
import warnings

# If threading is available then ThreadPool should be provided. Therefore
# we avoid top-level imports which are liable to fail on some systems.
Expand All @@ -30,6 +31,7 @@
# Constants representing the state of a pool
#

INIT = "INIT"
RUN = "RUN"
CLOSE = "CLOSE"
TERMINATE = "TERMINATE"
Expand Down Expand Up @@ -154,11 +156,15 @@ def Process(self, *args, **kwds):

def __init__(self, processes=None, initializer=None, initargs=(),
maxtasksperchild=None, context=None):
# Attributes initialized early to make sure that they exist in
# __del__() if __init__() raises an exception
self._pool = []
self._state = INIT

self._ctx = context or get_context()
self._setup_queues()
self._taskqueue = queue.SimpleQueue()
self._cache = {}
self._state = RUN
self._maxtasksperchild = maxtasksperchild
self._initializer = initializer
self._initargs = initargs
Expand All @@ -172,7 +178,6 @@ def __init__(self, processes=None, initializer=None, initargs=(),
raise TypeError('initializer must be a callable')

self._processes = processes
self._pool = []
try:
self._repopulate_pool()
except Exception:
Expand Down Expand Up @@ -216,6 +221,14 @@ def __init__(self, processes=None, initializer=None, initargs=(),
self._result_handler, self._cache),
exitpriority=15
)
self._state = RUN

# Copy globals as function locals to make sure that they are available
# during Python shutdown when the Pool is destroyed.
def __del__(self, _warn=warnings.warn, RUN=RUN):
if self._state == RUN:

@pitrou pitrou Dec 19, 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.

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

@vstinner vstinner Dec 19, 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.

done

_warn(f"unclosed running multiprocessing pool {self!r}",
ResourceWarning, source=self)

def __repr__(self):
cls = self.__class__
Expand Down
16 changes: 16 additions & 0 deletions Lib/test/_test_multiprocessing.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2577,6 +2577,22 @@ def test_enter(self):
pass
pool.join()

def test_resource_warning(self):
if self.TYPE == 'manager':
self.skipTest("test not applicable to manager")

pool = self.Pool(1)
pool.terminate()
pool.join()

# force state to RUN to emit ResourceWarning in __del__()

@pitrou pitrou Dec 19, 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.

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

@vstinner vstinner Dec 19, 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.

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.

@pitrou pitrou Dec 19, 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.

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

@vstinner vstinner Dec 20, 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.

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 :-)

@vstinner vstinner Dec 20, 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.

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

@pitrou pitrou Dec 20, 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.

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

pool._state = multiprocessing.pool.RUN

with support.check_warnings(('unclosed running multiprocessing pool',
ResourceWarning)):
pool = None
support.gc_collect()


def raising():
raise KeyError("key")
Expand Down
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Library/2018-12-06-02-02-28.bpo-35424.gXxOJU.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
:class:`multiprocessing.Pool` destructor now emits :exc:`ResourceWarning`
if the pool is still running.