Repository navigation
gh-156995: Add caution around API behavior change - #157292
cmaloney wants to merge 2 commits into
Conversation
Sorry, something went wrong.
The subtle change between free-threading and GIL-enabled builds resulted in the `Py_REFCNT(op) == 1` `bytes` object inside `bytearray` was considered non-unique during `_PyBytes_Resize` which resulted in it being substitued for an immortal global rather than keeping the allocation only in free-threaded builds. The behavior has been fixed in `_PyBytes_Resize` but the hazard exists and was a part of a release blocking bug so urge caution around that case when porting.
Documentation build overview6 files changed · ± 6 modified ± Modified |
Sorry, something went wrong.
| For objects where :c:expr:`Py_REFCNT(op) == 1` is always true this | ||
| function will return false when checked in a different thread than the | ||
| allocation. This can lead to subtle behavior change bugs between the | ||
| free-threaded and GIL-enabled builds (:gh:`156995`). |
There was a problem hiding this comment.
Can you elaborate "subtle behavior change bugs"? The current note is vague, so I'm not sure that it's useful.
Sorry, something went wrong.
There was a problem hiding this comment.
Agreed. I'm generally not a huge fan of linking GitHub issues in docs.
Sorry, something went wrong.
There was a problem hiding this comment.
GH-140062 ported _PyBytes_Resize from Py_REFCNT(result) == 1 to the 3.14 recommended addition *_IsUniquelyReferenced. That change led, only in free-threaded builds, to _PyBytes_Resize returning an immutable bytes if the object was allocated in one thread than resized in a different thread. Resize is used in contexts which assume the bytes is still editable leading to accidental modification of constants. That was only discovered as part of other fixes to a related in 3.15 object bytearray. Looking at 3.14 the _PyBytes_Resize part of gh-156995 should be backported as the bug exists there as well.
The first sentence lays out the case that changes from True to False. Technically a restatement of the earlier docs but not obvious to infer. It isn't exactly equivalent as crossing thread boundaries in free-threaded builds causes the return to change from True to False.
The second sentence depends what the code is using _IsUniquelyReferenced for, my leaning is just dropping it. I'm used to "uniquely referenced" enabling modify in place as an optimization. In this case because of the specific immutability of bytes led to returning an immortal constant in an editable context. For other objects it likely will lead to new allocations. Open to suggestions on better wording.
Sorry, something went wrong.
There was a problem hiding this comment.
i meant that you should elaborate the comment, so the reader doesn't need to access GitHub.
Sorry, something went wrong.
| On a :term:`free-threaded build`, this checks if *op*'s | ||
| :term:`reference count` is equal to one and additionally checks if *op* | ||
| is only used by this thread. :c:expr:`Py_REFCNT(op) == 1` is **not** | ||
| thread-safe on free-threaded builds; prefer this function. |
There was a problem hiding this comment.
this checks if op’s reference count is equal to one and additionally checks if op is only used by this thread
It's uneasy for me to understand the "additionally checks if op is only used by this thread" part of the sentence. Maybe it should be rephrased to explain that the test is false if the calling thread is different than the thread which created the object.
Sorry, something went wrong.
The subtle change between free-threading and GIL-enabled builds resulted in the Py_REFCNT(op) == 1 bytes object inside bytearray was considered non-unique during _PyBytes_Resize. That led to it being substituted for an immortal global rather than keeping the allocation only in free-threaded builds.
The behavior has been fixed in _PyBytes_Resize but the hazard exists and was a part of a release blocking bug so urge caution around that case when porting.