Fix thread safety for pybind11 loader_life_support #3237

laramiel · 2021-09-02T22:13:45Z

Description

Fixes issue: #2765

This converts the vector of PyObjects to either a single void* or a per-thread void* depending on the WITH_THREAD define.

The new void* field is used to construct a stack of loader_life_support frames that can extend the life of python objects.

The pointer is updated when the loader_life_support object is allocated (which happens before a call) as well as on release.

Each loader_life_support frame maintains a set of PyObject references that need to be lifetime extended; this is done by storing them in a c++ std::unordered_set and clearing the references when the object goes out of scope when the call has completed.

Suggested changelog entry:

Fixes thread safety for some pybind11::type_caster which require lifetime extension, such as for std::string_view.

Fixes issue: #2765 This converts the vector of PyObjects to either a single void* or a per-thread void* depending on the WITH_THREAD define. The new field is used by each thread to construct a stack of loader_life_support frames that can extend the life of python objects. The pointer is updated when the loader_life_support object is allocated (which happens before a call) as well as on release. Each loader_life_support maintains a set of PyObject references that need to be lifetime extended; this is done by storing them in a c++ std::unordered_set and clearing the references when the method completes.

… compatible

laramiel · 2021-09-02T22:58:59Z

Sure, a PyList or a vector or some other container would work as well.

The idea of using an unordered_set is intended to also address #1251
A PySet would probably also work for that case.

Alternatively we could allocate a simple empty list and then use the other patients mechanism to maintain the lifetime.

daniel-smith · 2021-09-03T00:18:03Z

Is it worth adding a python test similar to #2765 (comment) somewhere?

It may be appropriate to run this under msan/tsan/etc.

for more information, see https://pre-commit.ci

laramiel · 2021-09-03T01:29:12Z

I added a similar version; perhaps I should use a convertible reference rather than a string view. Let me know what you think.

daniel-smith · 2021-09-03T01:52:00Z

I added a similar version; perhaps I should use a convertible reference rather than a string view. Let me know what you think.

I believe that going via the string_view caster would exhibit the same issues - though I've not tested this out myself. I think as long as the test exercises the relevant code path, and verifies that things are thread-safe, then we're good.

daniel-smith · 2021-09-03T06:16:16Z

Actually @laramiel, one consideration of using string views for the test is that this is a C++17 feature - if we want to support compilation of tests on older C++ standards (?)

include/pybind11/detail/type_caster_base.h

std::string_view, as that's a C++ 17 feature.

for more information, see https://pre-commit.ci

laramiel · 2021-09-03T16:40:21Z

@daniel-smith Converted test to use lifetime-extended references.

include/pybind11/detail/common.h

tests/test_thread.py

for more information, see https://pre-commit.ci

Skylion007 · 2021-09-03T18:16:32Z

Also please remove unused imports from the added test python file and ensure clang-tidy is happy.

tests/test_thread.py

henryiii · 2021-09-03T18:17:42Z

include/pybind11/detail/internals.h

@@ -154,7 +157,7 @@ struct type_info {
 };

 /// Tracks the `internals` and `type_info` ABI version independent of the main library version
-#define PYBIND11_INTERNALS_VERSION 4
+#define PYBIND11_INTERNALS_VERSION 5


If we are bumping the ABI, we should probably move through several of the PRs sitting round that bump the ABI and get them in too. Also, this is going to create a massive mess, as we'll have to start a conda-forge migration, and quite a few packages don't use the conda-forge's pybind11, so... It will be a mess. But I guess it's going to have to happen eventually.

There may be a non-abi bumping strategy available; if you like I can take that approach.

tests/test_thread.py

Skylion007 · 2021-09-03T18:38:45Z

Clang-tidy is still unhappy about a missing comment: (see the check failure) and the tests are failing on 2.7 and PyPy

tests/test_thread.cpp

rwgk

My approval is based on Jeremy's and Daniel's reviews.

@Skylion007 or @henryiii, could you please also weigh in?

(One explicit needs to be added before we merge.)

Skylion007 · 2021-09-10T16:29:13Z

Merging this in as a stop-gap. Definitely should be refactored later as per the above discussion.

rainwoodman · 2021-09-10T19:30:17Z

tests/test_thread.py

+    def run(self):
+        try:
+            for i in range(10):
+                self.fn(i, i)


Can I avoid implicit cast (and triggering life support) by changing this line to self.fn(i, m.IntStruct(i))?

daniel-smith · 2021-09-11T00:26:15Z

Thanks everyone. Looking forward to seeing any future changes w.r.t. the issues @jbms raised.

This revises the existing fix for pybind#2765 in pybind#3237 to reduce the amount of TLS storage used. The shared TLS key is stored in two different ways, depending on `PYBIND11_INTERNALS_VERSION`. If `PYBIND11_INTERNALS_VERSION == 4` (as is currently set), the TLS key is stored in the `internal::shared_data` map to avoid breaking ABI compatibility. If `PYBIND11_INTERNALS_VERSION > 4`, the TLS key is stored directly in the `internals` struct.

This revises the existing fix for pybind#2765 in pybind#3237 to reduce the amount of TLS storage used. The shared TLS key is stored in two different ways, depending on `PYBIND11_INTERNALS_VERSION`. If `PYBIND11_INTERNALS_VERSION == 4` (as is currently set), the TLS key is stored in the `internal::shared_data` map to avoid breaking ABI compatibility. If `PYBIND11_INTERNALS_VERSION > 4`, the TLS key is stored directly in the `internals` struct. Fix test_pytypes.py::test_issue2361 failure on PyPy3.7 Add github actions tests for unstable ABI

* Eliminate duplicate TLS keys for loader_life_support stack This revises the existing fix for #2765 in #3237 to reduce the amount of TLS storage used. The shared TLS key is stored in two different ways, depending on `PYBIND11_INTERNALS_VERSION`. If `PYBIND11_INTERNALS_VERSION == 4` (as is currently set), the TLS key is stored in the `internal::shared_data` map to avoid breaking ABI compatibility. If `PYBIND11_INTERNALS_VERSION > 4`, the TLS key is stored directly in the `internals` struct. * Fix test_pytypes.py::test_issue2361 failure on PyPy3.7 * Add github actions tests for unstable ABI

The proximate reason for the upgrade is to pick up a thread-safety fix (pybind/pybind11#3237) for JAX, but we might as well just upgrade to the current version for everyone. Remove a backport of a patch now present in the released version (pybind/pybind11#2864). Patch out the "VERSION" file from nsync to avoid conflicts with the <version> C++ header included by pybind11. PiperOrigin-RevId: 420856622 Change-Id: Ied007ed62b93d96cf008d6ec6336017af1fa10fd

The reason for the downgrade is that v2.9.x is causing a segmentation fault in the TF Lite interpreter in TF Text conversion tests. The original reason for the 2.9.0 upgrade was to include a thread-safety fix (pybind/pybind11#3237, pybind/pybind11@0e59958) for JAX that was added in 2.8.0 and above, so performing this downgrade is not problematic for its intention. PiperOrigin-RevId: 442586909

laramiel added 2 commits September 2, 2021 15:03

Also update the internals version as the internal struct is no longer…

caa974f

… compatible

Add test demonstrating threading works correctly.

9179d60

It may be appropriate to run this under msan/tsan/etc.

laramiel requested a review from henryiii as a code owner September 3, 2021 01:26

[pre-commit.ci] auto fixes from pre-commit.com hooks

0c2bf55

for more information, see https://pre-commit.ci

daniel-smith reviewed Sep 3, 2021

View reviewed changes

include/pybind11/detail/type_caster_base.h Outdated Show resolved Hide resolved

laramiel and others added 5 commits September 3, 2021 09:21

Update test to use lifetime-extended references rather than

4b7dc7a

std::string_view, as that's a C++ 17 feature.

Merge branch 'master' of https://github.com/laramiel/pybind11

fdfff88

[pre-commit.ci] auto fixes from pre-commit.com hooks

d91a4a7

for more information, see https://pre-commit.ci

Make loader_life_support members private

c6720ca

Merge branch 'master' of https://github.com/laramiel/pybind11

2c07c0d