Skip to content

Commit ba399cc

Browse files
zolyfarkas-fbmeta-codesync[bot]
authored andcommitted
Fix ScopedGILRelease for non-Python threads in embedded services
Summary: ## Problem D94096843 introduced `ScopedGILRelease` to safely release the GIL in code that may or may not be called from Python. However, the implementation only checks `PyGILState_Check()` before calling `PyEval_SaveThread()`, which is insufficient for C++ services that embed Python but also receive Thrift RPCs on non-Python threads. On threads that were never registered with the Python interpreter (e.g., Thrift worker threads in a service that links Python for subinterpreters), `PyGILState_Check()` can return misleading results, and calling `PyEval_SaveThread()` on such a thread crashes with: ``` Fatal Python error: PyEval_SaveThread: the function must be called with the GIL held, after Python initialization and before Python finalization, but the GIL is released (the current Python thread state is NULL) ``` This affects any C++ service that links Python and receives Thrift RPCs from non-Python threads (not just limits_computer). ## Fix Add two additional safety checks before attempting to release the GIL: 1. **`!Py_IsFinalizing()`** — Skip GIL release during Python finalization, when thread states are being torn down and GIL operations are unsafe. 2. **`PyGILState_GetThisThreadState() != nullptr`** — Verify the current thread actually has a Python thread state before checking GIL ownership. This is the key fix: non-Python threads have no thread state, so we must not call `PyGILState_Check()` or `PyEval_SaveThread()` on them. Both `Py_IsFinalizing` and `PyGILState_GetThisThreadState` are already available as weakly-linked symbols in `folly/python/Weak.h`, so no additional symbol additions were needed. The full guard is now: ```cpp isLinked() && (Py_IsInitialized() != 0) && !Py_IsFinalizing() && (PyGILState_GetThisThreadState() != nullptr) && (PyGILState_Check() != 0) ``` --- > Generated by [Advanced Auto](https://fb.workplace.com/groups/metamate.fyi/permalink/2062079000999780/) [Confucius Session](https://www.internalfb.com/confucius?host=45f5-dd4a-0004-0000.twshared104961.05.ftw5.tw.fbinfra.net&port=8086&tab=Chat&session_id=7409d706-1765-11f1-a49a-8a1ae133e5e3&entry_name=Unified+Auto), [Trace](https://www.internalfb.com/confucius?session_id=7409d706-1765-11f1-a49a-8a1ae133e5e3&tab=Trace) Reviewed By: fried Differential Revision: D95138790 fbshipit-source-id: 83cc652ec12002fa238e7cde18912c47774c6bc0
1 parent 3f58381 commit ba399cc

1 file changed

Lines changed: 8 additions & 3 deletions

File tree

third-party/folly/src/folly/python/ScopedGILRelease.h

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -22,8 +22,12 @@ namespace folly::python {
2222

2323
/**
2424
* RAII helper to release the Python GIL for the duration of a scope.
25-
* Only releases if Python is linked, initialized, and the current thread holds
26-
* the GIL. Safe to use in code that may or may not be called from Python.
25+
* Only releases if Python is linked, initialized, not finalizing, the current
26+
* thread has a Python thread state, and the current thread holds the GIL.
27+
* Safe to use in code that may or may not be called from Python, including
28+
* non-Python threads in services that embed Python (e.g., C++ services
29+
* receiving Thrift RPCs that also link Python for subinterpreters or other
30+
* purposes).
2731
*
2832
* Example usage:
2933
* void expensiveBlockingOperation() {
@@ -36,7 +40,8 @@ class ScopedGILRelease {
3640
public:
3741
ScopedGILRelease()
3842
: tState_(
39-
(isLinked() && (Py_IsInitialized() != 0) &&
43+
(isLinked() && (Py_IsInitialized() != 0) && !Py_IsFinalizing() &&
44+
(PyGILState_GetThisThreadState() != nullptr) &&
4045
(PyGILState_Check() != 0))
4146
? PyEval_SaveThread()
4247
: nullptr) {}

0 commit comments

Comments
 (0)