From 91e2a25f92e48a911e955fe49930c8b71d46a11f Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Mon, 29 Jan 2018 11:57:45 +0100 Subject: [PATCH] bpo-20891: Py_Initialize() now creates the GIL (#4700) The GIL is no longer created "on demand" to fix a race condition when PyGILState_Ensure() is called in a non-Python thread. Reenable test_capi.test_bpo20891(). (cherry picked from commit 2914bb32e2adf8dff77c0ca58b33201bc94e398c) --- Doc/c-api/init.rst | 63 +++++++------------ Doc/whatsnew/3.6.rst | 4 ++ Lib/test/test_capi.py | 3 - .../2017-12-04-18-34-11.bpo-20891.C2TsfR.rst | 3 + Python/ceval.c | 25 ++++---- Python/pylifecycle.c | 3 + 6 files changed, 46 insertions(+), 55 deletions(-) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2017-12-04-18-34-11.bpo-20891.C2TsfR.rst diff --git a/Doc/c-api/init.rst b/Doc/c-api/init.rst index c8d429de51bb3a..ff7144c8d225cc 100644 --- a/Doc/c-api/init.rst +++ b/Doc/c-api/init.rst @@ -478,15 +478,14 @@ This is so common that a pair of macros exists to simplify it:: The :c:macro:`Py_BEGIN_ALLOW_THREADS` macro opens a new block and declares a hidden local variable; the :c:macro:`Py_END_ALLOW_THREADS` macro closes the -block. These two macros are still available when Python is compiled without -thread support (they simply have an empty expansion). +block. -When thread support is enabled, the block above expands to the following code:: +The block above expands to the following code:: PyThreadState *_save; _save = PyEval_SaveThread(); - ...Do some blocking I/O operation... + ... Do some blocking I/O operation ... PyEval_RestoreThread(_save); .. index:: @@ -609,36 +608,24 @@ code, or when embedding the Python interpreter: This is a no-op when called for a second time. + .. versionchanged:: 3.6.5 + This function is now called by :c:func:`Py_Initialize()`, so you don't + have to call it yourself anymore. + .. versionchanged:: 3.2 This function cannot be called before :c:func:`Py_Initialize()` anymore. .. index:: module: _thread - .. note:: - - When only the main thread exists, no GIL operations are needed. This is a - common situation (most Python programs do not use threads), and the lock - operations slow the interpreter down a bit. Therefore, the lock is not - created initially. This situation is equivalent to having acquired the lock: - when there is only a single thread, all object accesses are safe. Therefore, - when this function initializes the global interpreter lock, it also acquires - it. Before the Python :mod:`_thread` module creates a new thread, knowing - that either it has the lock or the lock hasn't been created yet, it calls - :c:func:`PyEval_InitThreads`. When this call returns, it is guaranteed that - the lock has been created and that the calling thread has acquired it. - - It is **not** safe to call this function when it is unknown which thread (if - any) currently has the global interpreter lock. - - This function is not available when thread support is disabled at compile time. - .. c:function:: int PyEval_ThreadsInitialized() Returns a non-zero value if :c:func:`PyEval_InitThreads` has been called. This function can be called without holding the GIL, and therefore can be used to - avoid calls to the locking API when running single-threaded. This function is - not available when thread support is disabled at compile time. + avoid calls to the locking API when running single-threaded. + + .. versionchanged:: 3.6.5 + The :term:`GIL` is now initialized by :c:func:`Py_Initialize()`. .. c:function:: PyThreadState* PyEval_SaveThread() @@ -646,8 +633,7 @@ code, or when embedding the Python interpreter: Release the global interpreter lock (if it has been created and thread support is enabled) and reset the thread state to *NULL*, returning the previous thread state (which is not *NULL*). If the lock has been created, - the current thread must have acquired it. (This function is available even - when thread support is disabled at compile time.) + the current thread must have acquired it. .. c:function:: void PyEval_RestoreThread(PyThreadState *tstate) @@ -655,8 +641,7 @@ code, or when embedding the Python interpreter: Acquire the global interpreter lock (if it has been created and thread support is enabled) and set the thread state to *tstate*, which must not be *NULL*. If the lock has been created, the current thread must not have - acquired it, otherwise deadlock ensues. (This function is available even - when thread support is disabled at compile time.) + acquired it, otherwise deadlock ensues. .. c:function:: PyThreadState* PyThreadState_Get() @@ -748,7 +733,7 @@ example usage in the Python source distribution. This macro expands to ``{ PyThreadState *_save; _save = PyEval_SaveThread();``. Note that it contains an opening brace; it must be matched with a following :c:macro:`Py_END_ALLOW_THREADS` macro. See above for further discussion of this - macro. It is a no-op when thread support is disabled at compile time. + macro. .. c:macro:: Py_END_ALLOW_THREADS @@ -756,29 +741,29 @@ example usage in the Python source distribution. This macro expands to ``PyEval_RestoreThread(_save); }``. Note that it contains a closing brace; it must be matched with an earlier :c:macro:`Py_BEGIN_ALLOW_THREADS` macro. See above for further discussion of - this macro. It is a no-op when thread support is disabled at compile time. + this macro. .. c:macro:: Py_BLOCK_THREADS This macro expands to ``PyEval_RestoreThread(_save);``: it is equivalent to - :c:macro:`Py_END_ALLOW_THREADS` without the closing brace. It is a no-op when - thread support is disabled at compile time. + :c:macro:`Py_END_ALLOW_THREADS` without the closing brace. .. c:macro:: Py_UNBLOCK_THREADS This macro expands to ``_save = PyEval_SaveThread();``: it is equivalent to :c:macro:`Py_BEGIN_ALLOW_THREADS` without the opening brace and variable - declaration. It is a no-op when thread support is disabled at compile time. + declaration. Low-level API ------------- -All of the following functions are only available when thread support is enabled -at compile time, and must be called only when the global interpreter lock has -been created. +All of the following functions must be called after :c:func:`Py_Initialize`. + +.. versionchanged:: 3.6.5 + :c:func:`Py_Initialize()` now initializes the :term:`GIL`. .. c:function:: PyInterpreterState* PyInterpreterState_New() @@ -848,8 +833,7 @@ been created. If this thread already has the lock, deadlock ensues. :c:func:`PyEval_RestoreThread` is a higher-level function which is always - available (even when thread support isn't enabled or when threads have - not been initialized). + available (even when threads have not been initialized). .. c:function:: void PyEval_ReleaseThread(PyThreadState *tstate) @@ -861,8 +845,7 @@ been created. reported. :c:func:`PyEval_SaveThread` is a higher-level function which is always - available (even when thread support isn't enabled or when threads have - not been initialized). + available (even when threads have not been initialized). .. c:function:: void PyEval_AcquireLock() diff --git a/Doc/whatsnew/3.6.rst b/Doc/whatsnew/3.6.rst index 59329fa7069408..b45157ee7979f8 100644 --- a/Doc/whatsnew/3.6.rst +++ b/Doc/whatsnew/3.6.rst @@ -2359,3 +2359,7 @@ Notable changes in Python 3.6.5 The :func:`locale.localeconv` function now sets temporarily the ``LC_CTYPE`` locale to the ``LC_NUMERIC`` locale in some cases. (Contributed by Victor Stinner in :issue:`31900`.) + +:c:func:`Py_Initialize` now creates the GIL. The GIL is no longer created "on +demand" to fix a race condition when PyGILState_Ensure() is called in a +non-Python thread. (Contributed by Victor Stinner in :issue:`20891`.) diff --git a/Lib/test/test_capi.py b/Lib/test/test_capi.py index 848a5289bab714..6e4286ed881aa2 100644 --- a/Lib/test/test_capi.py +++ b/Lib/test/test_capi.py @@ -494,9 +494,6 @@ def test_pre_initialization_api(self): self.assertEqual(out, '') self.assertEqual(err, '') - @unittest.skipIf(True, - "FIXME: test fails randomly because of a race conditon, " - "see bpo-20891") def test_bpo20891(self): """ bpo-20891: Calling PyGILState_Ensure in a non-Python thread before diff --git a/Misc/NEWS.d/next/Core and Builtins/2017-12-04-18-34-11.bpo-20891.C2TsfR.rst b/Misc/NEWS.d/next/Core and Builtins/2017-12-04-18-34-11.bpo-20891.C2TsfR.rst new file mode 100644 index 00000000000000..abf9c3c2e390df --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2017-12-04-18-34-11.bpo-20891.C2TsfR.rst @@ -0,0 +1,3 @@ +Py_Initialize() now creates the GIL. The GIL is no longer created "on demand" +to fix a race condition when PyGILState_Ensure() is called in a non-Python +thread. diff --git a/Python/ceval.c b/Python/ceval.c index 9ad582b15c572f..f334f262ff53ed 100644 --- a/Python/ceval.c +++ b/Python/ceval.c @@ -351,9 +351,9 @@ PyEval_SaveThread(void) if (tstate == NULL) Py_FatalError("PyEval_SaveThread: NULL tstate"); #ifdef WITH_THREAD - if (gil_created()) - drop_gil(tstate); + assert(gil_created()); #endif + drop_gil(tstate); return tstate; } @@ -363,18 +363,19 @@ PyEval_RestoreThread(PyThreadState *tstate) if (tstate == NULL) Py_FatalError("PyEval_RestoreThread: NULL tstate"); #ifdef WITH_THREAD - if (gil_created()) { - int err = errno; - take_gil(tstate); - /* _Py_Finalizing is protected by the GIL */ - if (_Py_Finalizing && tstate != _Py_Finalizing) { - drop_gil(tstate); - PyThread_exit_thread(); - assert(0); /* unreachable */ - } - errno = err; + assert(gil_created()); + + int err = errno; + take_gil(tstate); + /* _Py_Finalizing is protected by the GIL */ + if (_Py_Finalizing && _Py_Finalizing != tstate) { + drop_gil(tstate); + PyThread_exit_thread(); + assert(0); /* unreachable */ } + errno = err; #endif + PyThreadState_Swap(tstate); } diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index ecfdfee218dcc4..6e65e0e3aeecfd 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -360,6 +360,9 @@ _Py_InitializeEx_Private(int install_sigs, int install_importlib) _PyGILState_Init(interp, tstate); #endif /* WITH_THREAD */ + /* Create the GIL */ + PyEval_InitThreads(); + _Py_ReadyTypes(); if (!_PyFrame_Init())