classification
Title: multiprocessing: Add a stop() method to ForkServer
Type: Stage: resolved
Components: Library (Lib) Versions: Python 3.9
process
Status: closed Resolution: rejected
Dependencies: Superseder:
Assigned To: Nosy List: davin, pablogsal, pitrou, vstinner
Priority: normal Keywords:

Created on 2019-07-05 14:22 by vstinner, last changed 2019-10-01 10:33 by vstinner. This issue is now closed.

Messages (6)
msg347351 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-07-05 14:22
While working on bpo-37421, I fixed multiprocessing tests to explicitly call _run_finaliers(), so temporary directories are removed:

New changeset 039fb49c185570ab7b02f13fbdc51c859cfd831e by Victor Stinner in branch 'master':
bpo-37421: multiprocessing tests call _run_finalizers() (GH-14527)
https://github.com/python/cpython/commit/039fb49c185570ab7b02f13fbdc51c859cfd831e

Problem: some tests started to fail when run more than once, which happens using ./python -m test -R 3:3 (hunt reference leaks). So I made a first change:

New changeset 9d40554e0da09a44a8547f3f3a2b9dedfeaf7928 by Victor Stinner in branch 'master':
bpo-37421: Fix multiprocessing get_temp_dir() finalizer (GH-14572)
https://github.com/python/cpython/commit/9d40554e0da09a44a8547f3f3a2b9dedfeaf7928

But I also had to explicitly stop the ForkServer:

New changeset 9d40554e0da09a44a8547f3f3a2b9dedfeaf7928 by Victor Stinner in branch 'master':
bpo-37421: Fix multiprocessing get_temp_dir() finalizer (GH-14572)
https://github.com/python/cpython/commit/9d40554e0da09a44a8547f3f3a2b9dedfeaf7928


I propose to add a public ForkServer.stop() method to stop the server. The method would block until the server stops. The server doesn't track its children, so if there are running child processes, they will continue to run after stop() completes. Child processes are tracked by APIs like multiprocessing Pool.

I dislike relying on implicit resource management. I prefer to handle them explicitly, to ensure that errors are properly reported if something goes wrong. Implicit resource management rely on Python destructor which may be called very late during Python finalization, while other modules are already cleaned up, and so some function may fail silently (the machineny to report issues is broken as well).


In short, I propose to rename the new _stop() method as stop() and document it properly :-)
msg347441 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2019-07-06 19:33
There is no reason to expose "stop the forkserver" to users. How the forkserver process works, what tasks it handles, is an implementation detail. If users start "stopping the forkserver" in their applications for no good to reason, they might get bugs now or later that they wouldn't get otherwise.

Side note: if you're modifying multiprocessing, asking for a review doesn't hurt ;-)
msg347488 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-07-08 08:20
> There is no reason to expose "stop the forkserver" to users. How the forkserver process works, what tasks it handles, is an implementation detail. If users start "stopping the forkserver" in their applications for no good to reason, they might get bugs now or later that they wouldn't get otherwise.

Well, we already had this conversation before. I really dislike APIs which hold resources but disallow to "release resources", especially when a resource is not a few bytes of memory, but a whole process.

Since there is a method to start the server (ensure_running), I would like to control when it's stopped "to ensure that errors are properly reported if something goes wrong."

That's why I added Process.close() in Python 3.7 and ResourceWarning in Pool in Python 3.8.

Here I don't propose to add a ResourceWarning if the server is not stopped explicitly. I only propose to add a method if the developer wants to control when it's stopped.
msg347489 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-07-08 08:27
> Side note: if you're modifying multiprocessing, asking for a review doesn't hurt ;-)

I only modified tests (I only added a *private* _stop() method which is only used by tests). When I touch the multiprocessing module directly, I wait for a review.

Right now, I'm embarrassed since my "multiprocessing tests call _run_finalizers()" change which was supposed to be simple and safe broke Refleak buildbots. I had to fix test_concurrent_futures to also explicitly closes the forkserver there:
https://github.com/python/cpython/pull/14643

I was quite surprised to discover that a test leaves a child process in background. IMHO tests should have zero side effects on following tests (at least a test file must clear all its resources when it completes).

Python finalization is *very* fragile (not reliable at all), so I prefer to release resources as soon as possible, to get errors reported. Multiprocessing tests failures are still common, and very likely caused by these small details like a server which isn't stopped properly (IMHO). Tests should get a fresh and reliable environment.

Maybe each test should ensure that the forkserver is stopped, but I chose the easy fix: only stop it once at (test file) exit. Start/stop the server in each test might make these tests even slower, whereas multiprocessing tests are almost part of the slowest tests.
msg347491 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-07-08 08:29
Previous related discussion on python-dev:
https://mail.python.org/archives/list/python-dev@python.org/thread/NJCLS7IOKKO6W7LBGPQI2VFAO57EJEPW/#DIOMN256E24NUBSPKWFQA556F43IOWXR
msg353662 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-10-01 10:33
_test_multiprocessing.py calls:

        # Stop the ForkServer process if it's running
        from multiprocessing import forkserver
        forkserver._forkserver._stop()

It seems like multiprocessing tests are the main consumer of this method. So let's keep it private for now.
History
Date User Action Args
2019-10-01 10:33:10vstinnersetstatus: open -> closed
resolution: rejected
messages: + msg353662

stage: resolved
2019-07-08 08:29:59vstinnersetmessages: + msg347491
2019-07-08 08:27:56vstinnersetmessages: + msg347489
2019-07-08 08:20:36vstinnersetmessages: + msg347488
2019-07-06 19:33:17pitrousetmessages: + msg347441
2019-07-05 14:22:18vstinnercreate