GeezerVison

All commit authors signed the Contributor License Agreement.

The Modules/_heapqmodule.c change looks good to me. I haven't looked through the tests yet. I don't think should change the implementation to avoid borrowed references: I'd rather keep the change small and limited to the thread safety fix rather than try to "clean" things up, and I'm not really convinced that avoiding borrowed references here would make things better.

@yoney - would you please add add a NEWS entry via blurb. You can use uvx blurb, if you have the uv tools, or pip install it, or blurb-it.

I'd rather keep the change small and limited to the thread safety fix rather than try to "clean" things up, and I'm not really convinced that avoiding borrowed references here would make things better.

Ok, but we should definitely do this in a follow-up (possibly only for 3.15). There are definitely some things here that aren't safe. For example:

lastelt = PyList_GET_ITEM(heap, n-1) ;
Py_INCREF(lastelt);
if (PyList_SetSlice(heap, n-1, n, NULL)) {
    Py_DECREF(lastelt);
    return NULL;
}
n--;

if (!n)
    return lastelt;
returnitem = PyList_GET_ITEM(heap, 0);

A finalizer could either release the critical section or explicitly clear the list, which could cause that PyList_GET_ITEM call to return NULL. I guess that's not related to borrowed references, though--more of a problem with PyList_GET_ITEM not doing validation.

There's also some incredibly horrible things going on, like this:

returnitem = PyList_GET_ITEM(heap, 0);
PyList_SET_ITEM(heap, 0, lastelt);

returnitem starts out as a borrowed reference, but then has the ownership implicitly handed off through PyList_SET_ITEM, which doesn't decref it.

Ok, but we should definitely do this in a follow-up (possibly only for 3.15). There are definitely some things here that aren't safe.

@ZeroIntensity I agree that there are things we should follow up on. I initially tried to address some of them as part of the free-threading change, but it introduces complexity to the review and makes the free-threading change harder to review, so I decided to follow up on those issues separately.

There is a lot of duplication in tests because of min/max heaps, why not organize with subtests?

@StanFromIreland Thank you so much for your review! I've already refactored the code and moved some repeated parts into separate functions while addressing the other comments. I'm not sure if subtests will provide more code reuse here. What do you think?

Looks great! Will merge on Monday.

Just FYI: Made a minor update to remove the forgotten type hint.

Thank you!

Thanks @yoney for the PR, and @mpage for merging it 🌮🎉.. I'm working now to backport this PR to: 3.14.
🐍🍒⛏🤖

GH-135309 is a backport of this pull request to the 3.14 branch.