cpython icon indicating copy to clipboard operation
cpython copied to clipboard

gh-117657: Fix itertools.count thread safety

Open wiggin15 opened this issue 1 year ago • 4 comments

Thread safety in count_next. count_next has two modes. slow mode (obj->cnt set to PY_SSIZE_T_MAX), which now uses the object mutex (only if GIL is disabled) and fast mode, which is either simple cnt++ if GIL is enabled, or uses atomic_compare_exchange if GIL is disabled.

  • Issue: gh-117657

wiggin15 avatar May 20 '24 20:05 wiggin15

All commit authors signed the Contributor License Agreement.
CLA signed

ghost avatar May 20 '24 20:05 ghost

Can you arrange the #ifdef logic so that the traditional GIL build has EXACTLY the same code that it does now. That shouldn't be difficult since count_next() is only three lines of code.

In general, we don't want to damage the traditional build if we don't have to.

Also, we would like maintainers to be able to easily look at the GIL path to understand what the function does and how it works. After the PR, I personally find it challenging to just read this code that used to be simple and obvious.

rhettinger avatar May 20 '24 20:05 rhettinger

Can you arrange the #ifdef logic so that the traditional GIL build has EXACTLY the same code that it does now. That shouldn't be difficult since count_next() is only three lines of code.

Just to be clear, are you suggesting to bring back the original code under an ifdef, like so?

#ifdnef Py_GIL_DISABLED
// the original 3 lines, unmodified
#else
// free-threading code only
#endif

?

wiggin15 avatar May 20 '24 22:05 wiggin15

Just to be clear, are you suggesting to bring back the original code under an ifdef, like so?

+1 That would be a simple and reasonable way to preserve readability, maintainability, and performance. It would also make clear exactly what the no-gil code should be doing.

rhettinger avatar May 20 '24 22:05 rhettinger

Thank you @colesbury and @rhettinger for the constructive feedback. I pushed a revised commit. Please share your comments.

wiggin15 avatar May 20 '24 23:05 wiggin15

The path for the traditional build seems fine to me. The no-gil path is much more readable than before but I'm not expert enough to sign-off on that path.

rhettinger avatar May 21 '24 01:05 rhettinger

Oops, forgot to backport this to 3.13

colesbury avatar Jun 03 '24 22:06 colesbury

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

miss-islington-app[bot] avatar Jun 03 '24 22:06 miss-islington-app[bot]

Sorry, @wiggin15 and @DinoV, I could not cleanly backport this to 3.13 due to a conflict. Please backport using cherry_picker on command line.

cherry_picker 87939bd5790accea77c5a81093f16f28d3f0b429 3.13

miss-islington-app[bot] avatar Jun 03 '24 22:06 miss-islington-app[bot]

GH-120007 is a backport of this pull request to the 3.13 branch.

bedevere-app[bot] avatar Jun 03 '24 22:06 bedevere-app[bot]