Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 33.9k
GH-127058: Make PySequence_Tuple safer and probably faster.#127758
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Uh oh!
There was an error while loading. Please reload this page.
Conversation
markshannon commented Dec 9, 2024 • edited by bedevere-app bot
Loading Uh oh!
There was an error while loading. Please reload this page.
edited by bedevere-app bot
Uh oh!
There was an error while loading. Please reload this page.
PySequence_Tuple safer and probably faster.PySequence_Tuple safer and probably faster.Uh oh!
There was an error while loading. Please reload this page.
| Fail: | ||
| Py_XDECREF(result); | ||
| PyObject*res=_PyList_AsTupleAndClear(list); | ||
| Py_DECREF(list); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the name _PyList_AsTupleAndClear might be confusing, because if you called Py_CLEAR on the list you wouldn't need to decref it.
markshannonDec 11, 2024 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But if you called PyList_Clear on it, you would need to Py_DECREF it.
It is the list being cleared, not the variable holding a reference to it.
iritkatriel commented Dec 11, 2024
Did you run refleak tests? |
Co-authored-by: Irit Katriel <1055913+iritkatriel@users.noreply.github.com>
bedevere-bot commented Dec 11, 2024
🤖 New build scheduled with the buildbot fleet by @markshannon for commit 8badf7b 🤖 If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again. |
| self->ob_item=NULL; | ||
| Py_SET_SIZE(self, 0); | ||
| ret=_PyTuple_FromArraySteal(items, size); | ||
| free_list_items(items, false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If _PyTuple_FromArraySteal, do we still want to free the list items?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. _PyTuple_FromArraySteal steals the references, but doesn't do anything with the memory
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typically it is passed a stack-allocated array, so it can't free it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I meant if _PyTuple_FromArraySteal fails.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. _PyTuple_FromArraySteal consumes the all references regardless of whether it succeeds or fails.
It would be a pain to use otherwise.
5a23994 into python:mainUh oh!
There was an error while loading. Please reload this page.
…ython#127758) * Use a small buffer, then list when constructing a tuple from an arbitrary sequence.
user202729 commented Oct 18, 2025
note that this pull request modifies the behavior of the following code importtimeclassA: def__len__(self): raiseNotImplementedError("infinite sequence") def__iter__(self): whileTrue: time.sleep(1) yield0tuple(A())before this change, the above raises an error. Now it enters an infinite loop. by itself it isn't a problem because the fact that the same thing can be said about |
Instead of allocating a tuple, then running arbitrary code with an incomplete tuple, this PR:
_PyList_AsTupleAndClearfunction which creates a tuple from a list while clearing the list. This avoids unnecessary reference count manipulation.Since the vast majority of tuples are small,
PySequence_Tuplenow only does one allocation and no resizing in the common case.