Ship free-threaded wheels

[ngoldbaum]

My real goal with #100 was to enable shipping free-threaded wheels. I'm actively working on improving the state of free-threaded support for popular packages on PyPI and this package is one of them. I'll be linking to this issue from https://py-free-threading.github.io/tracking/. See that website for lots more information on supporting free-threaded Python.

Looking at the existing tests, they don't use the python threading module at all and there aren't any rust-level tests. Is that correct?

One thing we could try without writing tests for explicit multithreaded use is running pytest-run-parallel on the full test suite. That wouldn't catch thread safety issues caused by sharing data structures provided by rpds between threads, but it would catch issues due to use of global state.

I see this library already uses new_sync to create the rpds data structures, so according to the rpds docs everything should be thread safe. Do you agree with that assessment?

But also I don't actually see any multithreaded tests in the rust rpds library itself, so I'm not sure how carefully validated that is besides the fact that cargo parallelizes tests automatically.

Do you have any ideas for ways to validate the thread safety of the python library before we upload wheels and declare the rpds module is thread-safe?

[Julian]

Looking at the existing tests, they don't use the python threading module at all and there aren't any rust-level tests. Is that correct?

Correct.

I see this library already uses new_sync to create the rpds data structures, so according to the rpds docs everything should be thread safe. Do you agree with that assessment?

Yes that was my understanding as well, though I did less work than it sounds like you did already to check how well this is tested in rpds itself.

Do you have any ideas for ways to validate the thread safety of the python library before we upload wheels and declare the rpds module is thread-safe?

I like your pytest-run-parallel idea certainly -- I'd otherwise have likely added at least one test which explicitly spins up a few threads and then perhaps uses hypothesis to randomly add data across them -- but I don't think it should block calling the package thread-safe if we're relying on rpds meeting what it claims.

Said more "brutally", I'm personally OK with letting someone tell us if it turns out there are thread safety issues.

(And again sincere thanks for all the help so far and for the work you're doing more broadly!)

[ngoldbaum]

OK great, I'll keep the hypothesis idea on the backburner, I don't think I want to take on setting up hypothesis to do that since I've never set up hypothesis from scratch, let alone in a multithreaded context. That said, I bet we could leverage hypothesis to make it easier to productively start from scratch on writing multithreaded tests for sharing data structures here and in many other packages. I'll bring this up at our next internal sync on our ecosystem compatibility team.

[bollwyvl]

hypothesis

Hm, while I love hypothesis this, might lead to a bunch of work, considering https://hypothesis.readthedocs.io/en/latest/details.html#thread-safety-policy

[ngoldbaum]

this, might lead to a bunch of work, considering https://hypothesis.readthedocs.io/en/latest/details.html#thread-safety-policy

Yup, ran into that elsewhere today in the python-zstandard test suite.

[ngoldbaum]

I'm seeing test failures on Linux if I run the tests with pytest-run-parallel. See this actions run on my fork.

The failures look like:

FAILED tests/test_hash_trie_map.py::test_views_abc[Set-values] - TypeError: 'dict' object cannot be converted to 'HashTrieMap'
FAILED tests/test_hash_trie_map.py::test_views_abc[MappingView-keys] - TypeError: descriptor 'keys' for 'rpds.HashTrieMap' objects doesn't apply to a 'dict' object
FAILED tests/test_hash_trie_map.py::test_views_abc[MappingView-items] - TypeError: descriptor 'items' for 'rpds.HashTrieMap' objects doesn't apply to a 'dict' object
FAILED tests/test_hash_trie_map.py::test_views_abc[KeysView-items] - TypeError: descriptor 'items' for 'dict' objects doesn't apply to a 'rpds.HashTrieMap' object
FAILED tests/test_hash_trie_map.py::test_views_abc[ValuesView-values] - TypeError: descriptor 'values' for 'dict' objects doesn't apply to a 'rpds.HashTrieMap' object
FAILED tests/test_hash_trie_map.py::test_views_abc[ItemsView-values] - TypeError: descriptor 'values' for 'dict' objects doesn't apply to a 'rpds.HashTrieMap' object
FAILED tests/test_hash_trie_map.py::test_views_abc[ItemsView-items] - TypeError: descriptor 'items' for 'rpds.HashTrieMap' objects doesn't apply to a 'dict' object

I can reproduce this on my Ubuntu 22.04 developer machine using a pyenv-built 3.13.0t and the python-build-standalone python3.13t that uv installs (I was thinking that might be related, it's not).

The error is coming from CPython internals and sort of looks like some sort of thread safety issue in PyO3. HashTrieMapPy uses pyclass(mapping) which I don't think I ran across when I was working with PyO3, that could be related.

[ngoldbaum]

Oh actually I wonder if this is hitting the thread-safety issues in collections.abc that were fixed in CPython: python/cpython#125415

[ngoldbaum]

No, I still see if it I build Python using the latest version of the CPython 3.13 branch.

Some more info:

The errors are coming from the isinstance calls in this test:

rpds/tests/test_hash_trie_map.py

Lines 364 to 380 in 2597aa4

	@pytest.mark.parametrize(
	"view",
	[pytest.param(methodcaller(p), id=p) for p in ["keys", "values", "items"]],
	)
	@pytest.mark.parametrize(
	"cls",
	[
	abc.Set,
	abc.MappingView,
	abc.KeysView,
	abc.ValuesView,
	abc.ItemsView,
	],
	)
	def test_views_abc(view, cls):
	m, d = HashTrieMap(), {}
	assert isinstance(view(m), cls) == isinstance(view(d), cls)

And here's what the call stack looks like from inside gdb - somehow a corrupt PyObject struct is being passed into descr_check: https://gist.github.com/ngoldbaum/c855789fefc7f15496907b114afc44d1

Ping @davidhewitt and @colesbury in case this rings any bells as a PyO3 or CPython bug related to collections.abc protocols.

I'm going to see if I can make a small reproducer using only PyO3 tomorrow.

[colesbury]

No, sorry, doesn't ring any bells. For what it's worth, it looks to me like the error happens during the execution of view(m) or view(d), before isinstance() is actually invoked.

[ngoldbaum]

I can trigger this issue if I build a free-threaded wheel using the current main branch of rpds and run this script with -Xgil=0:

from operator import methodcaller

from concurrent.futures import ThreadPoolExecutor

from rpds import HashTrieMap

num_workers=1000

views = [methodcaller(p) for p in ["keys", "values", "items"]]

def work(view):
    m, d = HashTrieMap(), {}
    view(m)
    view(d)

iterations = 10

for _ in range(iterations):
    
    executor = ThreadPoolExecutor(max_workers=num_workers)

    for view in views:
        futures = [executor.submit(work, view) for _ in range(num_workers)]
        results = [future.result() for future in futures]

Notably it doesn't happen if work is defined like this:

def work(view):
    m = HashTrieMap()
    view(m)

So it looks like collections.abc being involved is a red herring. Maybe something about how pyo3 implements the mapping protocol for pyclasses is causing heap corruption in CPython?

[ngoldbaum]

Also worth noting that if I insert a threading.Barrier in between creating the dict and HashTrieMap and creating the views, the issue doesn't trigger.

[colesbury]

I think I was able to reproduce this in just pure Python (using a modification of your code). So I think it's a bug in CPython, not PyO3 or rpds:

https://gist.github.com/colesbury/0d3fa451b4b47e661952d2a1b88be461

Once every 5-10 runs, I get:

TypeError: descriptor 'keys' for 'dict' objects doesn't apply to a 'HashTrieMap' object

[ngoldbaum]

OK, in that case I think we're OK to move forward with a PR setting up pytest-run-parallel in CI and declaring free-threading support. I'll just go ahead and skip the test that fails on the free-threaded build, with the expectation that 3.13.1 or another future python release will fix it.

[ngoldbaum]

...except it looks like I'll need to do some work in maturin to make it possible to build free-threaded wheels:

$ maturin build --target aarch64-apple-darwin --release --out dist --interpreter 3.9 3.10 3.11 3.12 3.13 3.13t pypy3.9 pypy3.10
📦 Including license file "/Users/goldbaum/Documents/rpds-py/LICENSE"
🔗 Found pyo3 bindings
⚠️  Warning: skipped unavailable python interpreter 'pypy3.10' from pyenv
💥 maturin failed
  Caused by: Invalid python interpreter minor version '13t', expect a digit
  Caused by: invalid digit found in string