diff options
| -rw-r--r-- | Doc/c-api/init.rst | 16 | ||||
| -rw-r--r-- | Include/import.h | 8 | ||||
| -rw-r--r-- | Lib/test/test_fork1.py | 41 | ||||
| -rw-r--r-- | Modules/posixmodule.c | 34 | ||||
| -rw-r--r-- | Python/import.c | 31 |
5 files changed, 108 insertions, 22 deletions
diff --git a/Doc/c-api/init.rst b/Doc/c-api/init.rst index d79c123..152cb13 100644 --- a/Doc/c-api/init.rst +++ b/Doc/c-api/init.rst @@ -520,6 +520,22 @@ supports the creation of additional interpreters (using :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 diff --git a/Include/import.h b/Include/import.h index 65657b4..b8de2fd 100644 --- a/Include/import.h +++ b/Include/import.h @@ -27,6 +27,14 @@ PyAPI_FUNC(PyObject *) PyImport_ReloadModule(PyObject *m); 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 *); diff --git a/Lib/test/test_fork1.py b/Lib/test/test_fork1.py index e036661..88f9fe9 100644 --- a/Lib/test/test_fork1.py +++ b/Lib/test/test_fork1.py @@ -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 @@ class ForkTest(ForkWait): 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() diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index f767798..22b637e 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -3721,11 +3721,21 @@ Return 0 to child process and PID of child to parent process."); 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 @@ -3740,11 +3750,21 @@ Return 0 to child process and PID of child to parent process."); 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 @@ -3847,14 +3867,22 @@ To both, return fd of newly opened pseudo-terminal.\n"); 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 diff --git a/Python/import.c b/Python/import.c index 2ae5fa7..62142ae 100644 --- a/Python/import.c +++ b/Python/import.c @@ -262,8 +262,8 @@ static PyThread_type_lock import_lock = 0; 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) @@ -287,8 +287,8 @@ lock_import(void) 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) @@ -303,23 +303,16 @@ unlock_import(void) 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 * @@ -336,7 +329,7 @@ static PyObject * imp_acquire_lock(PyObject *self, PyObject *noargs) { #ifdef WITH_THREAD - lock_import(); + _PyImport_AcquireLock(); #endif Py_INCREF(Py_None); return Py_None; @@ -346,7 +339,7 @@ static PyObject * 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; @@ -2201,9 +2194,9 @@ PyImport_ImportModuleLevel(char *name, PyObject *globals, PyObject *locals, 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"); |