In multiprocessing.Pool documentation it's written "When the pool object is garbage collected terminate() will be called immediately.":

https://docs.python.org/3.7/library/multiprocessing.html#multiprocessing.pool.Pool.terminate

A. This does not happen, creating a Pool, deleting it and collecting the garbage, does not call terminate.

B. The documentation for Pool itself does not specify it has a context manager (but the examples show it).

C. This bug is both in Python 3 & 2.

> A. This does not happen, creating a Pool, deleting it and collecting the garbage, does not call terminate.

Would you give me an example how you delete the Pool and collecting the garbage? If you use context manager, It will call terminate() function.

> B. The documentation for Pool itself does not specify it has a context manager (but the examples show it).

You can find this info on the same page:

New in version 3.3: Pool objects now support the context management protocol – see Context Manager Types. __enter__() returns the pool object, and __exit__() calls terminate().

>>> from multiprocessing import Pool
>>> import gc
>>> a = Pool(10)
>>> del a
>>> gc.collect()
0
>>>

After this, there are still left behind Process (Pool) or Dummy (ThreadPool) and big _cache data (If you did something with it) which lingers till the process dies.

You are correct on the other issue (I'm using and reading the Python 2 documentation which does not have that...).

A patch would just add 

    def __del__(self):
        self.terminate()

in the Pool object.

But alas that does not work...

Add a __del__ method in the Pool class should work. But I'm not sure we should do this.

It would be sufficient to modify the documentation to reflect the code. 

There are other objects like `file` that recommend[0] calling a method to release resources without depending on implementation-specific details like garbage collection.

[0] https://docs.python.org/3/tutorial/inputoutput.html#reading-and-writing-files

Indeed, I think this simply needs a documentation fix.

What other object in the standard lib, leaks resources when deleted in CPython ? Even that documentation says the garbage collector will eventually destroy it, just like here... I think there is an implementation bug.

I think I've found the code bug causing the leak:

https://github.com/python/cpython/blob/caa331d492acc67d8f4edd16542cebfabbbe1e79/Lib/multiprocessing/pool.py#L180

There is a circular reference between the Pool object, and the self._worker_handler Thread object (and it's also saved in the frame locals for the thread object, which prevents it from being garbage collected).

> What other object in the standard lib, leaks resources when deleted in CPython ?

Threads come to mind, for example:

>>> import time, threading, weakref
>>> t = threading.Thread(target=time.sleep, args=(100000,))
>>> t.start()
>>> wr = weakref.ref(t)
>>> del t
>>> wr()
<Thread(Thread-1, started 139937234327296)>

Note I'm not against fixing this issue, just saying it's not that surprising for Pool to keep lingering around when you lost any user-visible reference to it.

It actually makes tons of sense that while the thread is running, that the object representing it is alive. After the thread finishes its work, the object dies.

>>> import time, threading, weakref, gc
>>> t = threading.Thread(target=time.sleep, args=(10,))
>>> wr = weakref.ref(t)
>>> t.start()
>>> del t
>>> gc.collect()
>>> wr()
<Thread(Thread-1, started 139937234327296)>
Wait 10 seconds...
>>> gc.collect()
>>> wr()

The thread is gone (which doesn't happen with the pool).

Anyhow, I've submitted a patch to fix the bug that was introduced 9 years ago on GH, feel free to check it.

Thanks a lot tzickle, I'll take a look.

New changeset 97bfe8d3ebb0a54c8798f57555cb4152f9b2e1d0 by Antoine Pitrou (tzickel) in branch 'master':
bpo-34172: multiprocessing.Pool leaks resources after being deleted (GH-8450)
https://github.com/python/cpython/commit/97bfe8d3ebb0a54c8798f57555cb4152f9b2e1d0

New changeset 97f998a4dfd6db6d867f446daa62445d0782bf39 by Antoine Pitrou (Miss Islington (bot)) in branch '3.7':
bpo-34172: multiprocessing.Pool leaks resources after being deleted (GH-8450) (GH-9676)
https://github.com/python/cpython/commit/97f998a4dfd6db6d867f446daa62445d0782bf39

New changeset 07b96a95db78eff3557d1bfed1df9ebecc40815b by Antoine Pitrou (Miss Islington (bot)) in branch '3.6':
bpo-34172: multiprocessing.Pool leaks resources after being deleted (GH-8450) (GH-9677)
https://github.com/python/cpython/commit/07b96a95db78eff3557d1bfed1df9ebecc40815b

Thanks tzickler for the report and pull request, and sorry for the delay.

This is now fixed in all 3.x branches.  I will close this now as multiprocessing in 2.7 diverges quite a bit from 3.x.  If you want to fix the issue in 2.7 as well, please say so and I'll reopen.

(tzickel, sorry for mistyping your handle :-/)

Its ok, you only did it twice :) I've submitted a manual 2.7 fix on GH.

New changeset 4a7dd30f5810e8861a3834159a222ab32d5c97d0 by Antoine Pitrou (tzickel) in branch '2.7':
[2.7] bpo-34172: multiprocessing.Pool leaks resources after being deleted (GH-9686)
https://github.com/python/cpython/commit/4a7dd30f5810e8861a3834159a222ab32d5c97d0

multiprocessing.Pool.imap hangs in MacOs after applying this commit:

import multiprocessing

def the_test():
    print("Begin")
    for x in multiprocessing.Pool().imap(int,
            ["4", "3"]):
        print(x)
    print("End")

the_test()

This also happens in the backported branches.

Bisecting between 3.7.0(good) and 3.7.1(bad) with the code in my previous commit points to:

97f998a4dfd6db6d867f446daa62445d0782bf39 is the first bad commit
commit 97f998a4dfd6db6d867f446daa62445d0782bf39
Author: Miss Islington (bot) <31488909+miss-islington@users.noreply.github.com>
Date:   Tue Oct 2 14:17:04 2018 -0700

    bpo-34172: multiprocessing.Pool leaks resources after being deleted (GH-8450) (GH-9676)

    Fix a reference issue inside multiprocessing.Pool that caused the pool to remain alive if it was deleted without being closed or terminated explicitly.
    (cherry picked from commit 97bfe8d3ebb0a54c8798f57555cb4152f9b2e1d0)

    Co-authored-by: tzickel <tzickel@users.noreply.github.com>

:040000 040000 90e0af29e82d0fcc1c1d7e19b3659f9602596e3e d997cfb00c1c44bbd87ce15edfd391203362a1d7 M      Lib
:040000 040000 6aa4273821c2c563ea69370a59284ed48576416f b6d46f14b6bb36cc5ae62b3ca74025c24d683bb5 M      Misc
bisect run success

This also happens on Linux, hanging in different moments:

❯ ./python test.py
Begin
4
[hangs]

❯ ./python test.py
Begin
4
3
[hangs]

❯ ./python test.py
[hangs]

A. It would be nice to add a test that tests this.
B. Now that Pool is cleaning up properly, any of it's functions which return another object (like imap's IMapIterator) need to hold a reference to the Pool, so it won't get cleanedup before computing.

here is something quick I did to check if it works (it works) but I'm not fluent in multiprocessing code, so If i'm missing something or doing something wrong feel free to tell me:

https://github.com/tzickel/cpython/commit/ec63a43706f3bf615ab7ed30fb095607f6101e26

As this issue was somehow still present previous to this commit, I am going to track this problem in a new issue:

issue35378

I suggest to revert this change, since it caused a regression: 
"multiprocessing.Pool.imap hangs in MacOs after applying this commit: (...)"

"""
def the_test():
    print("Begin")
    for x in multiprocessing.Pool().imap(int,
            ["4", "3"]):
        print(x)
    print("End")
"""

Side-note: Is it correct to use a pool without calling terminate() nor close()? Should we start to emit a ResourceWarning if a pool is not closed explicitly, as we did for files, sockets, asyncio event loops and subprocess.Popen objects?

Víctor, I have a PR fixing this in:

issue35378

Even if is not correct to not call close or join on the Pool, this behaviour was working before while now it hangs. The fix is really simple, si I think we should fix it and not revert the change on this case.

The previous posts here touch all this subjects:
A. The documentation explicitly says: "When the pool object is garbage collected terminate() will be called immediately." (Happened till a code refactor 9 years ago introduced this bug).

B. Large amount of code was developed for this technique:
https://github.com/python/cpython/blob/master/Lib/multiprocessing/util.py#L147 (Till the end of the file almost)

C. The reason I opened this bug was because I was called to see why a long running process crashes after a while, and found out it leaked tons of subprocesses / pool._cache memory.

D. The quoted code, will currently simply leak each invocation lots of subprocesses... 

I too, think we should push for the said fix.

tzickel:
> A. The documentation explicitly says: "When the pool object is garbage collected terminate() will be called immediately." (Happened till a code refactor 9 years ago introduced this bug).

It is a *very bad* practice to rely on __del__. Don't do that. That's why we introduced ResourceWarning.


tzickel:
> https://github.com/python/cpython/blob/master/Lib/multiprocessing/util.py#L147 (Till the end of the file almost)

Is this API *incompatible* with pool.close()? Explicitly release resources?


Pablo:
> Víctor, I have a PR fixing this in: (...) The fix is really simple, so I think we should fix it and not revert the change on this case.

I'm not comfortable with the fix. I cannot explain why but I feel like adding a strong dependency from a child to its parent is going to lead to more bugs, not less. It sounds like a recipe for reference cycles. Maybe I'm just plain wrong.

At this point, I would like that someone explains me what the problem is. https://github.com/python/cpython/pull/10852 is a solution, ok, but what is the problem? What does the code hangs, whereas previously it was fine? Is the example code really correct? Do we want to support such usage?

I understand that msg330864 rely on black magic to expect that it's going to be fine. The lifetime of the pool is implicit and it sounds like a bad design. I don't want to endorse that.

> I'm not comfortable with the fix. I cannot explain why but I feel like adding a strong dependency from a child to its parent is going to lead to more bugs, not less. It sounds like a recipe for reference cycles. Maybe I'm just plain wrong.

The pool child objects (imap iterators, async results...etc) need to keep a reference to the parent because if not, the caller is in charge of doing that and that may lead to bugs. Is the same scenario as if I get a dictionary iterator and then I destroy my reference to the dictionary: if the iterator does not keep a reference to the parent (the dictionary) it will not be possible to be used in the future. Indeed, we can see that this is what happens:

I'm not comfortable with the fix. I cannot explain why but I feel like adding a strong dependency from a child to its parent is going to lead to more bugs, not less. It sounds like a recipe for reference cycles. Maybe I'm just plain wrong.

>>> x = {1:2}
>>> y = iter(x)
>>> gc.get_referrers(x)
[<dict_keyiterator object at 0x0000024447D6D598>, 
{'__name__': '__main__', '__doc__': None, '__package__': None, '__loader__': <class '_frozen_importlib.BuiltinImporter'>, '__spec__': None, '__annotations__': {}, '__builtins__': <module 'builtins' (built-in)>, 'y': <dict_keyiterator object at 0x0000024447D6D598>, 'gc': <module 'gc' (built-in)>, 'x': {1: 2}}
]

We can see that the dict_keyiterator refers to the dictionary, keeping it alive.

Here we have the same situation: if we do not keep the pool alive, the iterator will hang when iterating because the jobs won't get finished.

>At this point, I would like that someone explains to me what the problem is. https://github.com/python/cpython/pull/10852 is a solution, ok, but what is the problem? What does the code hangs, whereas >previously it was fine? Is the example code really correct? Do we want to support such usage?

The code hangs because the pool was not being destroyed before due to the reference cycle between the pool and an internal object (a Thread). Now it hangs because the worker thread is destroyed with the pool as no references are kept to it and the job that the iterator is waiting on is never finished.

>I understand that msg330864 rely on black magic to expect that it's going to be fine. The lifetime of the pool is implicit and it sounds like a bad design. I don't want to endorse that.

I found the weird code in the example in several projects. I have corrected it to use the pool as a context manager or to call close(), but this means that users are doing this and it used to work and not it does not: technically is a regression.

> I found the weird code in the example in several projects. I have corrected it to use the pool as a context manager or to call close(), but this means that users are doing this and it used to work and not it does not: technically is a regression.

That's why I'm asking for a revert :-) I suggest to revert this change immediately from 2.7, 3.6 and 3.7, and later see what can be done for master.

Even if we design carefully an API, there will be always someone to misuse it :-) I would prefer to stop promoting such bad code and find a transition to more correct code.

I disagree that a child should keep its parent alive.

I would be ok to use a *weak reference* from the child to the parent to detect when the parent goes away, and maybe trigger an action in that case. For example, raise an exception or log a warning.

> I disagree that a child should keep its parent alive.

But this is normal across the standard library. For example, here is how a deque iterator keeps the deque alive:

>>> x = deque([1,2,3])
>>> deque_iter = iter(x)
>>> deque_weakref = weakref.ref(x)
>>> del x
>>> gc.collect()
>>> gc.get_referrers(deque_weakref())
[<_collections._deque_iterator object at 0x0000024447ED6EA8>]

Here, the deque iterator is the *only* reference to the deque. When we destroy it, the deque is destroyed:
>>> del deque_iter
>>> gc.collect()
>>> deque_weakref()
None

Reverting the code will cause another class of problems, like the reason I fixed it. Programs written such as the example that Pablo gave (and what I've seen) will quietly leak child processes, file descriptors (for the pipes) and memory to a variety degree might not be detected, or in the end detected in a big error or crash.

Also in some ResourceWarnings (if not all), the resources are closed in the end (like in sockets), here without this code patch you cannot implicitly reclaim the resources (because there is a Thread involved here), which I think is a high bar for the user to think about.

You can also enable multiprocessing's debug logging to see how the code behaves with and without the fix:
https://stackoverflow.com/a/1353037

I also agree with Pablo that there is code in the stdlib that holdes reference between child and parent. There is also code that has circular reference (for example Python 2's OrderedDict) and that is ok as well (not that this is the situation here).

> that there is code in the stdlib that holdes reference between child and parent

Just to clarify: is not that is just code in the stdlib that keeps a reference between child and parent. The examples I have given are the exact same situation that we have here: the iterator object associated with another needs to keep its parent alive to work correctly.

multiprocessing should help the developer to detect when the API is misused. For example, emit a ResourceWarning if a pool is not released explicitly. It might help to detect such bugs:

* bpo-33676
* bpo-35413

Another example of complex issue related to object lifetime, resources (file descriptors) and multiprocessing: bpo-30966, add SimpleQueue.close().

New changeset 3c6b0d967eb4c95e06c4f1beddfca4f6300d92ce by Victor Stinner in branch '3.7':
[3.7] Revert "bpo-34172: multiprocessing.Pool leaks resources after being deleted (GH-8450) (GH-9676)" (#10968)
https://github.com/python/cpython/commit/3c6b0d967eb4c95e06c4f1beddfca4f6300d92ce

New changeset eb38ee052e2273568d0041e969aa851ee44e43ce by Victor Stinner in branch '3.6':
[3.6] Revert "bpo-34172: multiprocessing.Pool leaks resources after being deleted (GH-8450) (GH-9677)" (GH-10969)
https://github.com/python/cpython/commit/eb38ee052e2273568d0041e969aa851ee44e43ce

New changeset 358fc87f53cf97a1768d5b1ded08f2a564f9fd85 by Victor Stinner in branch '2.7':
Revert "[2.7] bpo-34172: multiprocessing.Pool leaks resources after being deleted (GH-9686)" (GH-10970)
https://github.com/python/cpython/commit/358fc87f53cf97a1768d5b1ded08f2a564f9fd85

New changeset 9dfc754d61c55a526304e10a328bad448efa9ee9 by Victor Stinner in branch 'master':
Revert "bpo-34172: multiprocessing.Pool leaks resources after being deleted (GH-8450)" (GH-10971)
https://github.com/python/cpython/commit/9dfc754d61c55a526304e10a328bad448efa9ee9

I reverted the change in 2.7, 3.6, 3.7 and master branches because it introduces a regression and we are very close to a release:
https://mail.python.org/pipermail/python-dev/2018-December/155920.html

I don't want to have the pressure to push a quick fix. I would like to make sure that we have enough time to design a proper fix. I'm not saying that Pablo's fix is not correct, it's just bad timing.

This bug is likely here for a long time, so I think that it's ok to still have it in the next 3.6 and 3.7 bugfix releases.

I suggest to open a discussion on the python-dev mailing list about multiprocessing relying on the garbage collector and lifetime of multiprocessing objects (Pool, Process, result, etc.). It seems like I disagree with Pablo and tzickel, whereas Armin Rigo (PyPy which has a different GC) is more on my side (release explicitly resources) :-) I would prefer to move towards explicit resource managment instead of relying on destructors and the garbage collector. For example, it's a bad practice to rely on these when using PyPy.

See my previous comments about issues related to multiprocessing objects lifetime.

I agree that reverting in bugfix branches was the right thing to do.  I think the fix should have remained in master, though.

See also bpo-35424: "multiprocessing.Pool: emit ResourceWarning".

I wrote 10986 to fix 2 tests which leak resources.

I have a question. Why do tests have to call "pool.join()" after "with pool:"? When I use a file, I know that the resources are released after "with file:".

Should Pool.__exit__() call Pool.join()?

This question reminds me my fix in socketserver (bpo-31151 and bpo-31233) which leaked processes and threads, and my bug bpo-34037 (asyncio leaks threads).

I started a thread on python-dev to discuss these issues:
https://mail.python.org/pipermail/python-dev/2018-December/155946.html

The new test_del_pool() test of the fix failed on a buildbot: bpo-35413 "test_multiprocessing_fork: test_del_pool() leaks dangling threads and processes on AMD64 FreeBSD CURRENT Shared 3.x".

multiprocessing.Pool destructor now emits a ResourceWarning if it is still running: if .close() nor .terminate() have been called, see bpo- 35424.

It is a first alarm that the problematic example is wrong.

Should reconsider to fix this bug in the master branch? If yes, we should carefully document this backward incompatible change.

Pablo fixed bpo-35378 with:

New changeset 3766f18c524c57784eea7c0001602017d2122156 by Pablo Galindo in branch 'master':
bpo-35378: Fix multiprocessing.Pool references (GH-11627)
https://github.com/python/cpython/commit/3766f18c524c57784eea7c0001602017d2122156

Does this change also fix this issue? If not, can we attempt again to fix this issue?

Moreover, should we do something in Python 3.7? Sadly, I don't think that we can do anything for 3.7 and 2.7.

What's the status of this issue?

Pablo's fix looks like a superset of the original fix applied here, so I'm assuming it fixes this issue as well.

It should probably be backport to all supported 3.x branches though, so people aren't required to move to 3.8 to benefit from it.