Data race in intern_common when interning str objects in the free threading build
#129701
Comments
|
I hit this while testing PyYAML (though the TSAN stack trace was slightly different). Are you going to work on this @colesbury? |
|
Yes, I'll probably put up a PR today. Would you please share the TSAN stack trace? |
…ded build * Use a mutex to avoid potentially returning a non-immortalized string, because immortalization happens after the insertion into the interned dict. * Use `Py_DECREF()` calls instead of `Py_SET_REFCNT(s, Py_REFCNT(s) - 2)` for thread-safety. This code path isn't performance sensistive, so just use `Py_DECREF()` unconditionally for simplicity.
|
Sure, here it is. |
|
Thanks. Is this is with 3.13? I think that one was fixed in #128196, but it's not backported to 3.13. That particular PR changes the data types of public fields, so I don't think we can backport it. It might be worth doing the thread sanitizer tests on 3.14. cc @ngoldbaum |
|
I think I've seen similar tracebacks on 3.13 with PyO3 about interning strings. See this line in the gist from this PyO3 issue. I want to re-do the TSAN PyO3 tests with 3.14, but I need to update the FFI definitions first. Generally PyO3 doesn't add support for new python versions until beta 1 to avoid dealing with churn. |
…ded build * Use a mutex to avoid potentially returning a non-immortalized string, because immortalization happens after the insertion into the interned dict. * Use `Py_DECREF()` calls instead of `Py_SET_REFCNT(s, Py_REFCNT(s) - 2)` for thread-safety. This code path isn't performance sensistive, so just use `Py_DECREF()` unconditionally for simplicity.
Yes, it's with 3.13.2. I'll try 3.14 later today or early tomorrow. |
|
FWIW I did verify that the |
…ild (GH-130089) * gh-129701: Fix a data race in `intern_common` in the free threaded build * Use a mutex to avoid potentially returning a non-immortalized string, because immortalization happens after the insertion into the interned dict. * Use `Py_DECREF()` calls instead of `Py_SET_REFCNT(s, Py_REFCNT(s) - 2)` for thread-safety. This code path isn't performance sensistive, so just use `Py_DECREF()` unconditionally for simplicity.
|
I'm going to close this as the only remaining issue is that _Py_SetImmortal is not thread-safe in some circumstances, which is tracked in: |
Bug report
When running
./python -m test test_exceptions --parallel-threads=10in a TSAN build:There are a few thread-safety issues with
intern_common:internedbefore we mark the string as interned. This can be fixed with some additional locking.Py_SET_REFCNT(s, Py_REFCNT(s) - 2)modification is not thread-safe with respect to other reference count modifications in the free threading build_Py_SetImmortalis not thread-safe in some circumstances (see_Py_SetImmortalmust be run on allocating thread (no-gil) #113956)The
_Py_SetImmortal()issue is tricky and I think it's unlikely to cause problems in practice, so I think we can defer dealing with that for now.Linked PRs
intern_commonin the free threaded build #130089The text was updated successfully, but these errors were encountered: