Repository navigation
PySequence_Fast needs new macros to be safe in a nogil world #119247
Description
Activity
- added a commit that references this issue
on May 22, 2024 Hey @DinoV and @colesbury: Now that the base patch is merged, should I:
- Close this as complete and open new issue(s) to apply similar fixes to other uses of
PySequence_Fast*APIs? - Reuse this issue for similar changes?
- Skip working on this task specifically, and instead make more core Python objects work free-threaded (e.g. I started working on
bytearrayto make its use ofPySequence_Fast*stuff safe, but it's clearly not free-threaded safe in general yet, so maybe I just work on makingbytearrayas a whole free-threaded safe?).
- Close this as complete and open new issue(s) to apply similar fixes to other uses of
Hi Josh - thanks for fixing
str.join. We typically reuse the same issue for related changes. As to whether to continue withPySequence_Fast*or work other changes likebytearray, that's up to you -- both sound really useful.- added a commit that references this issue
on May 22, 2024 @MojoVampire @colesbury In the
_jsonmodule thePySequence_FastAPI is used in a way that is not thread-safe. I am working on a PR, but could use some advice. The new macroPy_BEGIN_CRITICAL_SECTION_SEQUENCE_FASTstarts with a{(I suspect to avoid name clashes), but that makes it hard to use when the critical section used return statements or goto statements.In #119438 I added an extra macro
Py_RETURN_CRITICAL_SECTION_SEQUENCE_FASTto address this, but I am not convinced this is a nice solution. Another option would be to refactor the code in question to eliminate the goto and return statements inside the critical part, but that will result in quite some churn of the code (and perhaps less readable code)
Feature or enhancement
Proposal:
Right now, most uses of
PySequence_Fastare invalid in a nogil context when it is passed an existinglist;PySequence_FAST_ITEMSreturns a reference to the internal array ofPyObject*s that can be resized at any time if other threads add or delete items,PySequence_FAST_GET_SIZEsimilarly reports a size that is invalid an instant after it's reported. Similarly, if individual items are replaced without changing size, you'd have similar issues.But when the argument passed is a
tuple(incref-ed and returned unchanged, but safe due to immutability) or any non-listtype (converted to newlist) no lock is needed. Per conversation with Dino, going to create macros, to be called after a call toPySequence_Fast, to conditionally lock and unlock the originallistwhen applicable, while avoiding locks in all other cases, before any otherPySequence*APIs are used.Preliminary (subject to bike-shedding) macro names are:
Py_BEGIN_CRITICAL_SECTION_SEQUENCE_FASTPy_END_CRITICAL_SECTION_SEQUENCE_FASTboth defined in
pycore_critical_section.h.Has this already been discussed elsewhere?
This is a minor feature, which does not need previous discussion elsewhere
Links to previous discussion of this feature:
Discussion occurred with Dino during CPython core sprints.
Linked PRs