classification
Title: multiprocessing should use sys.exit() where possible
Type: enhancement Stage: resolved
Components: Library (Lib) Versions: Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: brandjon, jnoller, neologix, pitrou, python-dev, sbt
Priority: normal Keywords: needs review, patch

Created on 2012-01-23 18:58 by brandjon, last changed 2012-06-14 19:05 by sbt. This issue is now closed.

Files
File name Uploaded Description Edit
multiprocessing_exit.diff neologix, 2012-02-08 20:48 review
sys_exit.patch sbt, 2012-06-13 17:41 review
Messages (8)
msg151835 - (view) Author: Jon Brandvein (brandjon) Date: 2012-01-23 18:58
Currently the multiprocessing library calls a hard exit function (os._exit under unix, ExitProcess under Windows) to terminate the child process.

Under unix, this is necessary because the child is forked without exec-ing. Calling sys.exit() would make it possible for the child to run code on the part of the stack inherited from the parent, via the exception handling mechanism. It might also allow the child to accidentally destroy shared state through an object destructor, even when that object was not explicitly relied on by the child.

Under Windows, I do not see any benefit besides symmetry. Processes are not forked, and the only way control can pass to user code after executing the target function, is if the process is frozen and the user puts the call to freeze_support() inside a try block. This special case can be taken care of by advising the user not to do that. (We already tell the user where freeze_support() should be located.)

Changing the multiprocessing exit routine from ExitProcess to sys.exit on Windows would ensure that all objects holding resources get properly destroyed. In particular, it would ensure that all file streams (including standard output and standard error) are flushed. This is especially important under Python 3, since the new IO system uses its own buffering which cannot be flushed by ExitProcess -- from the user's point of view, a potential regression from Python 2.x.

Related issues:
  - #13812 would not have been a problem under windows.
  - If #8713 gets adopted, unix can use sys.exit() as well.
msg151896 - (view) Author: Richard Oudkerk (sbt) * (Python committer) Date: 2012-01-24 11:49
Currently, on both Windows and Unix, when the main thread of a child process exits:

* atexit callbacks are NOT run (although multiprocessing.util._exit_function IS run),

* the main thread does NOT wait for non-daemonic background threads.

A simple replacement of ExitProcess() by sys.exit() would change this.  Whether that would be a good thing, I don't know.
msg151919 - (view) Author: Jon Brandvein (brandjon) Date: 2012-01-24 17:15
Good point. I don't think those particular behaviors are documented, so I'm not sure whether we need to worry about breaking them.
msg151936 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2012-01-25 08:30
> * atexit callbacks are NOT run (although multiprocessing.util._exit_function IS run),

It may be a good thing after a fork() (e.g. you don't want to remove
the same file twice), but it most definitely looks wrong for a new
interpreter process (à la Windows / fork() + exec()).

> * the main thread does NOT wait for non-daemonic background threads.

Same thing here.
msg152908 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2012-02-08 20:48
Here's a trivial patch.
I run the testsuite on one of the Windows buildbots, and there was one failure, in test_concurrent_futures:

"""
======================================================================
FAIL: test_interpreter_shutdown (test.test_concurrent_futures.ProcessPoolShutdownTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "D:\cygwin\home\db3l\buildarea\custom.bolen-windows7\build\lib\test\test_concurrent_futures.py", line 109, in test_interpreter_shutdown
    self.assertFalse(err)
AssertionError: b'[68326 refs]\n[68326 refs]\n[68326 refs]\n[68326 refs]\n[106059 refs]' is not false

----------------------------------------------------------------------
"""

That's simply because test.support.strip_python_stderr only strips the first occurrence of the refs count printed in debug mode at output (which didn't get printed upon ExitProcess).

The patch changes this.
msg152912 - (view) Author: Richard Oudkerk (sbt) * (Python committer) Date: 2012-02-08 22:05
I think the patch makes multiprocessing.util._exit_function() run twice in non-main processes because it is registered with atexit, and is also called in Process._bootstrap().

_exit_function() does the following:

 * terminate active daemon processes;

 * join active non-daemon processes;

 * run finalizers with non-None exit priority and clear finalizer registry.

So calling _exit_function() twice is probably harmless but should perhaps be fixed.

P.S. It also appears that _exit_function() should set the global _exiting to True, since it declares the variable as a global but does not use it.  As a result util.is_exiting() always returns False.
msg162716 - (view) Author: Richard Oudkerk (sbt) * (Python committer) Date: 2012-06-13 17:41
The trivial patch of replacing exit() by sys.exit() caused manager processes to be terminated after a short timeout.  (It is inconvenient that in Python there is no way for a non-main thread to request immediate shutdown of the process.)

This new patch makes the manager process shutdown cleanly.
msg162796 - (view) Author: Roundup Robot (python-dev) Date: 2012-06-14 14:49
New changeset d31e83497c5a by Richard Oudkerk in branch 'default':
Issue #13841: Make child processes exit using sys.exit() on Windows
http://hg.python.org/cpython/rev/d31e83497c5a
History
Date User Action Args
2012-06-14 19:05:41sbtsetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2012-06-14 14:49:09python-devsetnosy: + python-dev
messages: + msg162796
2012-06-13 17:41:04sbtsetfiles: + sys_exit.patch

messages: + msg162716
2012-02-08 22:05:57sbtsetmessages: + msg152912
2012-02-08 20:48:12neologixsetkeywords: + patch, needs review
files: + multiprocessing_exit.diff
messages: + msg152908

stage: test needed -> patch review
2012-01-27 23:08:44terry.reedysetstage: test needed
versions: - Python 3.2
2012-01-25 08:30:37neologixsetmessages: + msg151936
2012-01-24 17:15:42brandjonsetmessages: + msg151919
2012-01-24 11:49:37sbtsetnosy: + sbt
messages: + msg151896
2012-01-23 18:58:51brandjoncreate