From 4336d12e15e85059241c8424aa5a39f5a237b3ca Mon Sep 17 00:00:00 2001
From: vfdev-5 <vfdev.5@gmail.com>
Date: Wed, 6 Nov 2024 13:49:54 +0100
Subject: [PATCH] Addressed PR comments

---
 include/pybind11/detail/type_caster_base.h |  3 +--
 include/pybind11/pybind11.h                |  6 +++---
 tests/pybind11_tests.h                     |  1 +
 tests/test_class.py                        | 24 +++++-----------------
 4 files changed, 10 insertions(+), 24 deletions(-)

diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h
index e7d4c6cdaf..097287874a 100644
--- a/include/pybind11/detail/type_caster_base.h
+++ b/include/pybind11/detail/type_caster_base.h
@@ -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;
 }
 
 /**
diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h
index d54108052d..b4f93f1a6a 100644
--- a/include/pybind11/pybind11.h
+++ b/include/pybind11/pybind11.h
@@ -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;
diff --git a/tests/pybind11_tests.h b/tests/pybind11_tests.h
index 1264a21412..0eb0398df0 100644
--- a/tests/pybind11_tests.h
+++ b/tests/pybind11_tests.h
@@ -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;
diff --git a/tests/test_class.py b/tests/test_class.py
index 8ce7dcb3c4..268aeb4d85 100644
--- a/tests/test_class.py
+++ b/tests/test_class.py
@@ -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):