Skip to content

Avoid GIL deadlocks by calling PyGILState_GetThisThreadState when using gil_scoped_acquire. #1211

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Dec 1, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions include/pybind11/pybind11.h
Original file line number Diff line number Diff line change
Expand Up @@ -1776,6 +1776,15 @@ class gil_scoped_acquire {
auto const &internals = detail::get_internals();
tstate = (PyThreadState *) PYBIND11_TLS_GET_VALUE(internals.tstate);

if (!tstate) {
/* Check if the GIL was acquired using the PyGILState_* API instead (e.g. if
calling from a Python thread). Since we use a different key, this ensures
we don't create a new thread state and deadlock in PyEval_AcquireThread
below. Note we don't save this state with internals.tstate, since we don't
create it we would fail to clear it (its reference count should be > 0). */
tstate = PyGILState_GetThisThreadState();
}

if (!tstate) {
tstate = PyThreadState_New(internals.istate);
#if !defined(NDEBUG)
Expand Down
1 change: 1 addition & 0 deletions tests/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ set(PYBIND11_TEST_FILES
test_eval.cpp
test_exceptions.cpp
test_factory_constructors.cpp
test_gil_scoped.cpp
test_iostream.cpp
test_kwargs_and_defaults.cpp
test_local_bindings.cpp
Expand Down
43 changes: 43 additions & 0 deletions tests/test_gil_scoped.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
/*
tests/test_gil_scoped.cpp -- acquire and release gil

Copyright (c) 2017 Borja Zarco (Google LLC) <[email protected]>

All rights reserved. Use of this source code is governed by a
BSD-style license that can be found in the LICENSE file.
*/

#include "pybind11_tests.h"
#include <pybind11/functional.h>


class VirtClass {
public:
virtual void virtual_func() {}
virtual void pure_virtual_func() = 0;
};

class PyVirtClass : public VirtClass {
void virtual_func() override {
PYBIND11_OVERLOAD(void, VirtClass, virtual_func,);
}
void pure_virtual_func() override {
PYBIND11_OVERLOAD_PURE(void, VirtClass, pure_virtual_func,);
}
};

TEST_SUBMODULE(gil_scoped, m) {
py::class_<VirtClass, PyVirtClass>(m, "VirtClass")
.def(py::init<>())
.def("virtual_func", &VirtClass::virtual_func)
.def("pure_virtual_func", &VirtClass::pure_virtual_func);

m.def("test_callback_py_obj",
[](py::object func) { func(); });
m.def("test_callback_std_func",
[](const std::function<void()> &func) { func(); });
m.def("test_callback_virtual_func",
[](VirtClass &virt) { virt.virtual_func(); });
m.def("test_callback_pure_virtual_func",
[](VirtClass &virt) { virt.pure_virtual_func(); });
}
80 changes: 80 additions & 0 deletions tests/test_gil_scoped.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
import multiprocessing
import threading
from pybind11_tests import gil_scoped as m


def _run_in_process(target, *args, **kwargs):
"""Runs target in process and returns its exitcode after 1s (None if still alive)."""
process = multiprocessing.Process(target=target, args=args, kwargs=kwargs)
process.daemon = True
try:
process.start()
# Do not need to wait much, 1s should be more than enough.
process.join(timeout=1)
return process.exitcode
finally:
if process.is_alive():
process.terminate()


def _python_to_cpp_to_python():
"""Calls different C++ functions that come back to Python."""
class ExtendedVirtClass(m.VirtClass):
def virtual_func(self):
pass

def pure_virtual_func(self):
pass

extended = ExtendedVirtClass()
m.test_callback_py_obj(lambda: None)
m.test_callback_std_func(lambda: None)
m.test_callback_virtual_func(extended)
m.test_callback_pure_virtual_func(extended)


def _python_to_cpp_to_python_from_threads(num_threads, parallel=False):
"""Calls different C++ functions that come back to Python, from Python threads."""
threads = []
for _ in range(num_threads):
thread = threading.Thread(target=_python_to_cpp_to_python)
thread.daemon = True
thread.start()
if parallel:
threads.append(thread)
else:
thread.join()
for thread in threads:
thread.join()


def test_python_to_cpp_to_python_from_thread():
"""Makes sure there is no GIL deadlock when running in a thread.

It runs in a separate process to be able to stop and assert if it deadlocks.
"""
assert _run_in_process(_python_to_cpp_to_python_from_threads, 1) == 0


def test_python_to_cpp_to_python_from_thread_multiple_parallel():
"""Makes sure there is no GIL deadlock when running in a thread multiple times in parallel.

It runs in a separate process to be able to stop and assert if it deadlocks.
"""
assert _run_in_process(_python_to_cpp_to_python_from_threads, 8, parallel=True) == 0


def test_python_to_cpp_to_python_from_thread_multiple_sequential():
"""Makes sure there is no GIL deadlock when running in a thread multiple times sequentially.

It runs in a separate process to be able to stop and assert if it deadlocks.
"""
assert _run_in_process(_python_to_cpp_to_python_from_threads, 8, parallel=False) == 0


def test_python_to_cpp_to_python_from_process():
"""Makes sure there is no GIL deadlock when using processes.

This test is for completion, but it was never an issue.
"""
assert _run_in_process(_python_to_cpp_to_python) == 0