gh-154570: Fix itertools.accumulate() silently corrupting its total on reentrancy#154571
gh-154570: Fix itertools.accumulate() silently corrupting its total on reentrancy#154571PhysicistJohn wants to merge 3 commits into
Conversation
…otal on reentrancy
A source iterable or func= callable that calls back into next() on
the same accumulate object mid-step silently corrupted the running
total instead of erroring.
Adds a running guard field to accumulateobject, mirroring the
existing pattern already used by teedataobject for the identical
class of bug (see teedataobject_getitem_lock_held). accumulate now
raises RuntimeError("cannot re-enter the accumulate iterator") on
reentrant access instead of silently corrupting state, matching
tee's existing behavior for the same defect class.
Adds a regression test (test_accumulate_reenter) in the same style
as the existing test_tee_reenter.
|
All commit authors signed the Contributor License Agreement. |
| # A source iterable (or binop) that calls back into next() on the | ||
| # same accumulate object must not be allowed to silently corrupt | ||
| # the running total. |
There was a problem hiding this comment.
This comment seems unnecessary given that this PR is more about preventing reentrancy in itertools.accumulate than fixing a specific bug.
| # A source iterable (or binop) that calls back into next() on the | |
| # same accumulate object must not be allowed to silently corrupt | |
| # the running total. |
| Fix :func:`itertools.accumulate` silently corrupting its running total | ||
| when the source iterable (or the ``func`` callable) re-enters the same | ||
| :func:`!accumulate` iterator via a nested call to :func:`!next`. It now | ||
| raises :exc:`RuntimeError` on re-entry instead of producing wrong, | ||
| silently-dropped values, matching the existing behavior of | ||
| :func:`itertools.tee`. |
There was a problem hiding this comment.
I don't think that you need to say all of this information. Something like this is probably better:
| Fix :func:`itertools.accumulate` silently corrupting its running total | |
| when the source iterable (or the ``func`` callable) re-enters the same | |
| :func:`!accumulate` iterator via a nested call to :func:`!next`. It now | |
| raises :exc:`RuntimeError` on re-entry instead of producing wrong, | |
| silently-dropped values, matching the existing behavior of | |
| :func:`itertools.tee`. | |
| Now, :func:`itertools.accumulate` raises a :exc:`RuntimeError` during reentrancy. |
| PyObject *binop; | ||
| PyObject *initial; | ||
| itertools_state *state; | ||
| int running; |
There was a problem hiding this comment.
uint8_t uses less memory than int:
| int running; | |
| uint8_t running; |
…-path reentrancy test Per brijkapadia's review on pythonGH-154571: - Remove the explanatory comment above test_accumulate_reenter. - Change accumulateobject.running from int to uint8_t (no call sites needed changes; assigning 0/1 to a uint8_t requires no source edit). - Shorten the NEWS entry, keeping one clause on what was actually broken (silent corruption) rather than only the new behavior. Also add test_accumulate_reenter_func, covering reentrancy triggered via the func callable rather than the source iterable -- the guard protects both paths but only the iterable path had a test. teedataobject.running (same file, same purpose, for itertools.tee) is deliberately left as int rather than also narrowed, to avoid scope creep beyond what was requested.
|
Thanks for the review! Applied all three: removed the comment, running Also added a test for reentrancy through the func callable, not just Didn't touch teedataobject.running, it's still int, so there's a size |
…nrelated to this change Signed-off-by: PhysicistJohn <54456354+PhysicistJohn@users.noreply.github.com>
|
I don't think we should treat this as fixing a bug, |
2 participants
A source iterable or
func=callable that calls back intonext()onthe same
accumulateobject mid-step silently corrupted the runningtotal instead of erroring. Fixes gh-154570.
Adds a
runningguard field toaccumulateobject, mirroring theexisting pattern already used by
teedataobjectfor the identicalclass of bug (see
teedataobject_getitem_lock_held).accumulatenow raises
RuntimeError("cannot re-enter the accumulate iterator")on reentrant access instead of silently corrupting state, matching
tee's existing behavior for the same defect class.Adds a regression test (
test_accumulate_reenter) in the same styleas the existing
test_tee_reenter.