[Python-checkins] r75247 - in python/branches/release31-maint: Doc/c-api/init.rst Include/import.h Lib/test/test_fork1.py Modules/posixmodule.c Python/import.c

Sun Oct 4 22:35:31 CEST 2009

Author: benjamin.peterson
Date: Sun Oct 4 22:35:30 2009
New Revision: 75247
Log:
Merged revisions 75246 via svnmerge from 
svn+ssh://pythondev@svn.python.org/python/branches/py3k
................
 r75246 | benjamin.peterson | 2009年10月04日 15:32:25 -0500 (2009年10月04日) | 29 lines
 
 Merged revisions 74841 via svnmerge from 
 svn+ssh://pythondev@svn.python.org/python/trunk
 
 ........
 r74841 | thomas.wouters | 2009年09月16日 14:55:54 -0500 (2009年9月16日) | 23 lines
 
 
 Fix issue #1590864, multiple threads and fork() can cause deadlocks, by
 acquiring the import lock around fork() calls. This prevents other threads
 from having that lock while the fork happens, and is the recommended way of
 dealing with such issues. There are two other locks we care about, the GIL
 and the Thread Local Storage lock. The GIL is obviously held when calling
 Python functions like os.fork(), and the TLS lock is explicitly reallocated
 instead, while also deleting now-orphaned TLS data.
 
 This only fixes calls to os.fork(), not extension modules or embedding
 programs calling C's fork() directly. Solving that requires a new set of API
 functions, and possibly a rewrite of the Python/thread_*.c mess. Add a
 warning explaining the problem to the documentation in the mean time.
 
 This also changes behaviour a little on AIX. Before, AIX (but only AIX) was
 getting the import lock reallocated, seemingly to avoid this very same
 problem. This is not the right approach, because the import lock is a
 re-entrant one, and reallocating would do the wrong thing when forking while
 holding the import lock.
 
 Will backport to 2.6, minus the tiny AIX behaviour change.
 ........
................
Modified:
 python/branches/release31-maint/ (props changed)
 python/branches/release31-maint/Doc/c-api/init.rst
 python/branches/release31-maint/Include/import.h
 python/branches/release31-maint/Lib/test/test_fork1.py
 python/branches/release31-maint/Modules/posixmodule.c
 python/branches/release31-maint/Python/import.c
Modified: python/branches/release31-maint/Doc/c-api/init.rst
==============================================================================

--- python/branches/release31-maint/Doc/c-api/init.rst	(original)
+++ python/branches/release31-maint/Doc/c-api/init.rst	Sun Oct 4 22:35:30 2009
@@ -520,6 +520,22 @@
 :cfunc:`Py_NewInterpreter`), but mixing multiple interpreters and the
 :cfunc:`PyGILState_\*` API is unsupported.
 
+Another important thing to note about threads is their behaviour in the face
+of the C :cfunc:`fork` call. On most systems with :cfunc:`fork`, after a
+process forks only the thread that issued the fork will exist. That also
+means any locks held by other threads will never be released. Python solves
+this for :func:`os.fork` by acquiring the locks it uses internally before
+the fork, and releasing them afterwards. In addition, it resets any
+:ref:`lock-objects` in the child. When extending or embedding Python, there
+is no way to inform Python of additional (non-Python) locks that need to be
+acquired before or reset after a fork. OS facilities such as
+:cfunc:`posix_atfork` would need to be used to accomplish the same thing.
+Additionally, when extending or embedding Python, calling :cfunc:`fork`
+directly rather than through :func:`os.fork` (and returning to or calling
+into Python) may result in a deadlock by one of Python's internal locks
+being held by a thread that is defunct after the fork.
+:cfunc:`PyOS_AfterFork` tries to reset the necessary locks, but is not
+always able to.
 
 .. ctype:: PyInterpreterState
 
Modified: python/branches/release31-maint/Include/import.h
==============================================================================
--- python/branches/release31-maint/Include/import.h	(original)
+++ python/branches/release31-maint/Include/import.h	Sun Oct 4 22:35:30 2009
@@ -27,6 +27,14 @@
 PyAPI_FUNC(void) PyImport_Cleanup(void);
 PyAPI_FUNC(int) PyImport_ImportFrozenModule(char *);
 
+#ifdef WITH_THREAD
+PyAPI_FUNC(void) _PyImport_AcquireLock(void);
+PyAPI_FUNC(int) _PyImport_ReleaseLock(void);
+#else
+#define _PyImport_AcquireLock()
+#define _PyImport_ReleaseLock() 1
+#endif
+
 PyAPI_FUNC(struct filedescr *) _PyImport_FindModule(
 	const char *, PyObject *, char *, size_t, FILE **, PyObject **);
 PyAPI_FUNC(int) _PyImport_IsScript(struct filedescr *);
Modified: python/branches/release31-maint/Lib/test/test_fork1.py
==============================================================================
--- python/branches/release31-maint/Lib/test/test_fork1.py	(original)
+++ python/branches/release31-maint/Lib/test/test_fork1.py	Sun Oct 4 22:35:30 2009
@@ -1,8 +1,14 @@
 """This test checks for correct fork() behavior.
 """
 
+import errno
+import imp
 import os
+import signal
+import sys
 import time
+import threading
+
 from test.fork_wait import ForkWait
 from test.support import run_unittest, reap_children, get_attribute
 
@@ -23,6 +29,41 @@
 self.assertEqual(spid, cpid)
 self.assertEqual(status, 0, "cause = %d, exit = %d" % (status&0xff, status>>8))
 
+ def test_import_lock_fork(self):
+ import_started = threading.Event()
+ fake_module_name = "fake test module"
+ partial_module = "partial"
+ complete_module = "complete"
+ def importer():
+ imp.acquire_lock()
+ sys.modules[fake_module_name] = partial_module
+ import_started.set()
+ time.sleep(0.01) # Give the other thread time to try and acquire.
+ sys.modules[fake_module_name] = complete_module
+ imp.release_lock()
+ t = threading.Thread(target=importer)
+ t.start()
+ import_started.wait()
+ pid = os.fork()
+ try:
+ if not pid:
+ m = __import__(fake_module_name)
+ if m == complete_module:
+ os._exit(0)
+ else:
+ os._exit(1)
+ else:
+ t.join()
+ # Exitcode 1 means the child got a partial module (bad.) No
+ # exitcode (but a hang, which manifests as 'got pid 0')
+ # means the child deadlocked (also bad.)
+ self.wait_impl(pid)
+ finally:
+ try:
+ os.kill(pid, signal.SIGKILL)
+ except OSError:
+ pass
+
 def test_main():
 run_unittest(ForkTest)
 reap_children()
Modified: python/branches/release31-maint/Modules/posixmodule.c
==============================================================================
--- python/branches/release31-maint/Modules/posixmodule.c	(original)
+++ python/branches/release31-maint/Modules/posixmodule.c	Sun Oct 4 22:35:30 2009
@@ -3831,11 +3831,21 @@
 static PyObject *
 posix_fork1(PyObject *self, PyObject *noargs)
 {
-	pid_t pid = fork1();
+	pid_t pid;
+	int result;
+	_PyImport_AcquireLock();
+	pid = fork1();
+	result = _PyImport_ReleaseLock();
 	if (pid == -1)
 		return posix_error();
 	if (pid == 0)
 		PyOS_AfterFork();
+	if (result < 0) {
+		/* Don't clobber the OSError if the fork failed. */
+		PyErr_SetString(PyExc_RuntimeError,
+				"not holding the import lock");
+		return NULL;
+	}
 	return PyLong_FromPid(pid);
 }
 #endif
@@ -3850,11 +3860,21 @@
 static PyObject *
 posix_fork(PyObject *self, PyObject *noargs)
 {
-	pid_t pid = fork();
+	pid_t pid;
+	int result;
+	_PyImport_AcquireLock();
+	pid = fork();
+	result = _PyImport_ReleaseLock();
 	if (pid == -1)
 		return posix_error();
 	if (pid == 0)
 		PyOS_AfterFork();
+	if (result < 0) {
+		/* Don't clobber the OSError if the fork failed. */
+		PyErr_SetString(PyExc_RuntimeError,
+				"not holding the import lock");
+		return NULL;
+	}
 	return PyLong_FromPid(pid);
 }
 #endif
@@ -3957,14 +3977,22 @@
 static PyObject *
 posix_forkpty(PyObject *self, PyObject *noargs)
 {
-	int master_fd = -1;
+	int master_fd = -1, result;
 	pid_t pid;
 
+	_PyImport_AcquireLock();
 	pid = forkpty(&master_fd, NULL, NULL, NULL);
+	result = _PyImport_ReleaseLock();
 	if (pid == -1)
 		return posix_error();
 	if (pid == 0)
 		PyOS_AfterFork();
+	if (result < 0) {
+		/* Don't clobber the OSError if the fork failed. */
+		PyErr_SetString(PyExc_RuntimeError,
+				"not holding the import lock");
+		return NULL;
+	}
 	return Py_BuildValue("(Ni)", PyLong_FromPid(pid), master_fd);
 }
 #endif
Modified: python/branches/release31-maint/Python/import.c
==============================================================================
--- python/branches/release31-maint/Python/import.c	(original)
+++ python/branches/release31-maint/Python/import.c	Sun Oct 4 22:35:30 2009
@@ -261,8 +261,8 @@
 static long import_lock_thread = -1;
 static int import_lock_level = 0;
 
-static void
-lock_import(void)
+void
+_PyImport_AcquireLock(void)
 {
 	long me = PyThread_get_thread_ident();
 	if (me == -1)
@@ -286,8 +286,8 @@
 	import_lock_level = 1;
 }
 
-static int
-unlock_import(void)
+int
+_PyImport_ReleaseLock(void)
 {
 	long me = PyThread_get_thread_ident();
 	if (me == -1 || import_lock == NULL)
@@ -302,23 +302,16 @@
 	return 1;
 }
 
-/* This function is called from PyOS_AfterFork to ensure that newly
- created child processes do not share locks with the parent. */
+/* This function used to be called from PyOS_AfterFork to ensure that newly
+ created child processes do not share locks with the parent, but for some
+ reason only on AIX systems. Instead of re-initializing the lock, we now
+ acquire the import lock around fork() calls. */
 
 void
 _PyImport_ReInitLock(void)
 {
-#ifdef _AIX
-	if (import_lock != NULL)
-		import_lock = PyThread_allocate_lock();
-#endif
 }
 
-#else
-
-#define lock_import()
-#define unlock_import() 0
-
 #endif
 
 static PyObject *
@@ -335,7 +328,7 @@
 imp_acquire_lock(PyObject *self, PyObject *noargs)
 {
 #ifdef WITH_THREAD
-	lock_import();
+	_PyImport_AcquireLock();
 #endif
 	Py_INCREF(Py_None);
 	return Py_None;
@@ -345,7 +338,7 @@
 imp_release_lock(PyObject *self, PyObject *noargs)
 {
 #ifdef WITH_THREAD
-	if (unlock_import() < 0) {
+	if (_PyImport_ReleaseLock() < 0) {
 		PyErr_SetString(PyExc_RuntimeError,
 				"not holding the import lock");
 		return NULL;
@@ -2200,9 +2193,9 @@
 			 PyObject *fromlist, int level)
 {
 	PyObject *result;
-	lock_import();
+	_PyImport_AcquireLock();
 	result = import_module_level(name, globals, locals, fromlist, level);
-	if (unlock_import() < 0) {
+	if (_PyImport_ReleaseLock() < 0) {
 		Py_XDECREF(result);
 		PyErr_SetString(PyExc_RuntimeError,
 				"not holding the import lock");
