Skip to content

Commit

Permalink
Addressed PR comments
Browse files Browse the repository at this point in the history
  • Loading branch information
vfdev-5 committed Nov 6, 2024
1 parent 68dfe28 commit 4336d12
Show file tree
Hide file tree
Showing 4 changed files with 10 additions and 24 deletions.
3 changes: 1 addition & 2 deletions include/pybind11/detail/type_caster_base.h
Original file line number Diff line number Diff line change
Expand Up @@ -175,8 +175,7 @@ PYBIND11_NOINLINE void all_type_info_populate(PyTypeObject *t, std::vector<type_
* The value is cached for the lifetime of the Python type.
*/
inline const std::vector<detail::type_info *> &all_type_info(PyTypeObject *type) {
auto ins = all_type_info_get_cache(type);
return ins.first->second;
return all_type_info_get_cache(type).first->second;
}

/**
Expand Down
6 changes: 3 additions & 3 deletions include/pybind11/pybind11.h
Original file line number Diff line number Diff line change
Expand Up @@ -2334,9 +2334,9 @@ all_type_info_get_cache(PyTypeObject *type) {
.emplace(type, std::vector<detail::type_info *>());
#endif
if (ins.second) {
// In free-threading this method should be called
// under pymutex lock to avoid other threads
// continue running with empty ins.first->second
// For free-threading mode, this call must be under
// the with_internals() mutex lock, to avoid that other threads
// continue running with the empty ins.first->second.
all_type_info_populate(type, ins.first->second);
}
return ins;
Expand Down
1 change: 1 addition & 0 deletions tests/pybind11_tests.h
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ void ignoreOldStyleInitWarnings(F &&body) {
py::dict(py::arg("body") = py::cpp_function(body)));
}

// See PR #5419 for background.
class TestContext {
public:
TestContext() = delete;
Expand Down
24 changes: 5 additions & 19 deletions tests/test_class.py
Original file line number Diff line number Diff line change
Expand Up @@ -513,35 +513,21 @@ def test_pr4220_tripped_over_this():

@pytest.mark.skipif(sys.platform.startswith("emscripten"), reason="Requires threads")
def test_all_type_info_multithreaded():
# Test data race in all_type_info method in free-threading mode.
# For example, we have 2 threads entering `all_type_info`.
# Both enter `all_type_info_get_cache`` function and
# there is a first one which inserts a tuple (type, empty_vector) to the map
# and second is waiting. Inserting thread gets the (iter_to_key, True) and non-inserting thread
# after waiting gets (iter_to_key, False).
# Inserting thread than will add a weakref and will then call into `all_type_info_populate`.
# However, non-inserting thread is not entering `if (ins.second) {` clause and
# returns `ins.first->second;`` which is just empty_vector.
# Finally, non-inserting thread is failing the check in `allocate_layout`:
# if (n_types == 0) {
# pybind11_fail(
# "instance allocation failed: new instance has no pybind11-registered base types");
# }
# See PR #5419 for background.
import threading

from pybind11_tests import TestContext

class Context(TestContext):
def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)
pass

num_runs = 4
num_threads = 5
num_runs = 10
num_threads = 4
barrier = threading.Barrier(num_threads)

def func():
barrier.wait()
with Context():
with Context() as ctx:
pass

for _ in range(num_runs):
Expand Down

0 comments on commit 4336d12

Please sign in to comment.