GeezerVison

This runs arbitrary code which may result in the deallocation of anything which the function doesn't have a strong reference to.

That introduces a number of failure cases (the code can modify the grouper object, code could call this function again...)

Yep, it seems obvious to me now but I didn't realise that when I made the PR... I'll take another look.

I just checked the code and it's not as bad as I assumed (which reassures me on my first version 😁), there are guardrails around that rich compare but indeed it seems like there are cases where they're not enough.

In the loop, all the paths that return NULL are safe as well since they don't read anything.

The lines left after that are the ones that run after a break...

Py_INCREF(gbo->currkey);
Py_XSETREF(gbo->tgtkey, gbo->currkey);

grouper = _grouper_create(gbo, gbo->tgtkey);
if (grouper == NULL)
    return NULL;

return _PyTuple_FromPairSteal(Py_NewRef(gbo->currkey), grouper);

gbo itself shouldn't ever be NULL since it's kept as a strong reference throughout the method call. My understanding though is that gbo->currkey could be (e.g. if some Python code ends up calling _grouper_next which will do Py_CLEAR(gbo->currkey)).

In my latest commit, I changed the code so that we (1) keep a strong reference to gbo->currkey throughout and (2) avoid re-reading the field and use "our" reference instead.

I also updated the test to detach the CS in Rich Compare which means it should now flex the fix.

This should probably go in https://github.com/python/cpython/blob/main/Lib/test/test_free_threading/test_itertools.py

For gh- references typically do at the start,

# gh-150791: Concurrent next calls on a shared groupby object and its groupers
# should not crash even with arbitrary code __eq__.

Is the __slots__ critical to the test?

don't do type annotations in cpython source with very a few exceptions currently. Only a few internal-only bits of cpython code use type annotations and are checked with mypy, elsewhere don't add currently.

Fix :func:itertools.groupby data races in :term:free-threading.

Default to Py_XSETREF and/or Py_SETREF for cases like this, there are lots of edge cases in other options.

I think this will be able to turn into threading_helper.run_concurrently

I don't think this is needed / necessary

This can change while the critical section is released during __eq__ if another thread calls next() on the groupby object. I think re-checking it would be good? (although doesn't crash at least)

items = [("a", 1), ("a", 2), ("b", 3), ("b", 4)]
g = itertools.groupby(items, key=lambda it: Key(it[0]))   # Key.__eq__ can block on an Event
ka, grp_a = next(g)

# thread t1:
next(grp_a)        # ('a', 1)
next(grp_a)        # compares Key('a')#1 == Key('a')#2 -> blocks inside __eq__

# main thread, while t1 is blocked:
kb, grp_b = next(g)
# then release t1