Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

gh-123471: Make concurrent iteration over itertools.cycle safe under free-threading#131212

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
kumaraditya303 merged 10 commits intopython:mainfromeendebakpt:cycle_ft
Jun 2, 2025

Conversation

eendebakpt
Copy link
Contributor

@eendebakpteendebakpt commentedMar 13, 2025•
edited by bedevere-appbot
Loading

See#124397

  • In the FT build we cannot clear the iteratorcycle->it. Instead we usecycle->index as a check whether the iterator has been exhausted or not.
  • We remove dead code related tocycle->firstpass
  • The unit test is combined withitertools.batched in a single file.

@rhettinger
Copy link
Contributor

In the "Strategy for Free Threading" umbrella issue, principle 3 is:

Other iterators implemented in C will get only the minimal changes necessary to cause them to not crash in a free-threaded build. The edits should be made in a way that does not impact existing semantics or performance (i.e. do not damage the standard GIL build). Concurrent access is allowed to return duplicate values, skip values, or raise an exception.

Given that plan, can you please articulate reason for this PR. Is there currently a risk of a crash?

Also what is specifically is meant by "safe under free-threading"? AFAICT there is no concurrent access use case for cycle() that wouldn't be met by the planned options to either wrap withserialize() ortee() depending on whether the user wants atomic/consecutive semantics or parallel/independent semantics. That is what we would do for an equivalent generator.

The itertools code was highly refined and micro-optimized with painful tooling (valgrind, vtune, etc), so I'm a bit reluctant to change long stable code unless it is really necessary. Also, I was waiting to see how the essential builtin functions/classes evolved so that itertools could follow a proven model rather than being on the bleeding edge where misadventures might persist for a long time before being noticed.

@eendebakpt
Copy link
ContributorAuthor

The current implementation ofitertools.cycle can crash under the free-threading build for two reasons:

  • Concurrent iteration can result inlz->index being incremented by a thread to a valueofPyList_GET_SIZE(lz->saved)while in another thread the non thread-safePyList_GET_ITEM` is used.

item=PyList_GET_ITEM(lz->saved,lz->index);
lz->index++;
if (lz->index >=PyList_GET_SIZE(lz->saved))
lz->index=0;

  • One thread can exhaust the iteratorlz->it and clear it

Py_CLEAR(lz->it);
}

while another thread is still usinglz->it (since thecycleobject might be holding the only reference to the iterator this is not safe). The test added in this PR will often crash the interpreter in the free-threading build on my system (increasing thenumber_of_iterations will increase the odds).

By "safe under free-threading" I mean the free-threading build will not crash. No guarantees are given about any "correctness" of the results (I can adapt the title and news entry if desired). It is true that using the plannedserialize would be a good solution for someone planning to use thecycle with concurrent iteration. But I have not doubt some users will be using theitertools.cycle in a free-threading setting (perhaps even being unaware of this via 3rd party packages). I believe we should make the cpython free-threading safe against race conditions leading to crashes like this. These kind of bugs can be difficult to debug, and could potentially only trigger very rarely.

@rhettinger
Copy link
Contributor

rhettinger commentedMar 17, 2025•
edited
Loading

Okay, this sounds reasonable :-) Thanks for explaining the purpose of the edit.

Please do check that performance hasn't been negatively impacted (if so, the make anifdef so that the baseline isn't impacted; if not, then we're good).

@eendebakpt
Copy link
ContributorAuthor

@rhettinger In the benchmarks I did this PR is faster for the normal build. There are two reasons for this

  • The unused variablefirstpass is removed, making thecycleobject smaller in memory and avoiding the initialiation of the variable in the constructor. (note: this change is not related to FT)
  • Once the iterator is exhausted, we only accesslz->index andlz->saved. In current main in addition thelz->it is accessed.

The gain from this PR is small though (and I would not be surprised if for different platforms or benchmark tests the gain is zero).

Here are the results of one of the benchmarks I did:

Benchmark code
import timeimport gcfrom itertools import cyclet0=time.perf_counter()for ii in range(100):    c  = cycle( (1, 2, 3, 4))        for _ in range(200):        next(c)gc.collect() # make sure that in both the normal and free-threading build we clean up the constructed objectsdt=time.perf_counter()-t0print(dt)
Script to generate and plot the data
""" Interleaved benchmark for itertools.cycle@author: eendebakpt"""import subprocessimport matplotlib.pyplot as pltimport numpy as nptest_script = '/home/eendebakpt/python_testing/benchmarks/bm_itertools_cycle.py'cmds = ["/home/eendebakpt/cpython0/python {test_script", "/home/eendebakpt/cpython/python {test_script}" ]verbose = Falsett = []for ii in range(600):    print(f"run {ii}")    for cmd in cmds:        p = subprocess.run(cmd, shell=True, check=True, capture_output=True, encoding="utf-8")        if verbose:            print(f"Command {p.args} exited with {p.returncode} code, output: \n{p.stdout}")        tt.append(float(p.stdout))tt_main = tt[::2]tt_pr = tt[1::2]# %% Show resultsplt.figure(10)plt.clf()plt.plot(tt_main[::2], ".", label="Main")plt.plot(tt_pr[1::2], ".", label="PR")plt.axhline(np.mean(tt_main), color="C0", label="mean for main")plt.axhline(np.mean(tt_pr), color="C1", label="mean for PR")plt.ylabel("Execution time [s]")plt.legend()gain = np.mean(tt_main) / np.mean(tt_pr)plt.title(f"Performance gain: {gain:.3f}")

image

@kumaraditya303kumaraditya303 self-requested a reviewMay 28, 2025 15:31
@kumaraditya303kumaraditya303 merged commit26a1cd4 intopython:mainJun 2, 2025
43 checks passed
@bedevere-bot
Copy link

⚠️⚠️⚠️ Buildbot failure⚠️⚠️⚠️

Hi! The buildbotARM Raspbian 3.x (tier-3) has failed when building commit26a1cd4.

What do you need to do:

  1. Don't panic.
  2. Checkthe buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/#/builders/424/builds/10920) and take a look at the build logs.
  4. Check if the failure is related to this commit (26a1cd4) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/#/builders/424/builds/10920

Failed tests:

  • test.test_concurrent_futures.test_interpreter_pool

Failed subtests:

  • test_create_connection_local_addr_nomatch_family - test.test_asyncio.test_events.PollEventLoopTests.test_create_connection_local_addr_nomatch_family
  • test_map_buffersize_on_infinite_iterable - test.test_concurrent_futures.test_interpreter_pool.InterpreterPoolExecutorTest.test_map_buffersize_on_infinite_iterable
  • test_map_buffersize_on_multiple_infinite_iterables - test.test_concurrent_futures.test_interpreter_pool.InterpreterPoolExecutorTest.test_map_buffersize_on_multiple_infinite_iterables

Summary of the results of the build (if available):

==

Click to see traceback logs
Traceback (most recent call last):  File"/var/lib/buildbot/workers/3.x.gps-raspbian.nondebug/build/Lib/test/test_concurrent_futures/executor.py", line138, intest_map_buffersize_on_multiple_infinite_iterablesself.assertEqual(next(res,None),0)~~~~^^^^^^^^^^^  File"/var/lib/buildbot/workers/3.x.gps-raspbian.nondebug/build/Lib/concurrent/futures/_base.py", line666, inresult_iteratoryield _result_or_cancel(fs.pop())~~~~~~~~~~~~~~~~~^^^^^^^^^^  File"/var/lib/buildbot/workers/3.x.gps-raspbian.nondebug/build/Lib/concurrent/futures/_base.py", line311, in_result_or_cancelreturn fut.result(timeout)~~~~~~~~~~^^^^^^^^^  File"/var/lib/buildbot/workers/3.x.gps-raspbian.nondebug/build/Lib/concurrent/futures/_base.py", line450, inresultreturnself.__get_result()~~~~~~~~~~~~~~~~~^^  File"/var/lib/buildbot/workers/3.x.gps-raspbian.nondebug/build/Lib/concurrent/futures/_base.py", line395, in__get_resultraiseself._exceptionconcurrent.futures.interpreter.BrokenInterpreterPool:A thread initializer failed, the thread pool is not usable anymoreTraceback (most recent call last):  File"/var/lib/buildbot/workers/3.x.gps-raspbian.nondebug/build/Lib/test/test_asyncio/test_events.py", line832, intest_create_connection_local_addr_nomatch_familywithself.assertRaises(OSError):~~~~~~~~~~~~~~~~~^^^^^^^^^AssertionError:OSError not raisedTraceback (most recent call last):  File"/var/lib/buildbot/workers/3.x.gps-raspbian.nondebug/build/Lib/concurrent/futures/thread.py", line99, in_worker    ctx.initialize()~~~~~~~~~~~~~~^^  File"/var/lib/buildbot/workers/3.x.gps-raspbian.nondebug/build/Lib/concurrent/futures/interpreter.py", line115, ininitializeself.interpid= _interpreters.create(reqrefs=True)~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^interpreters.InterpreterError:interpreter creation failedTraceback (most recent call last):  File"/var/lib/buildbot/workers/3.x.gps-raspbian.nondebug/build/Lib/test/support/__init__.py", line829, ingc_collect    gc.collect()ResourceWarning:unclosed <socket.socket fd=5, family=10, type=1, proto=6, laddr=('::1', 35907, 0, 0), raddr=('::1', 35907, 0, 0)>Warning -- Unraisable exceptionException ignored while calling deallocator <function _SelectorTransport.__del__ at 0xf6524fa0>:Traceback (most recent call last):  File"/var/lib/buildbot/workers/3.x.gps-raspbian.nondebug/build/Lib/asyncio/selector_events.py", line873, in__del__    _warn(f"unclosed transport{self!r}",ResourceWarning,source=self)ResourceWarning:unclosed transport <_SelectorSocketTransport fd=5>Traceback (most recent call last):  File"/var/lib/buildbot/workers/3.x.gps-raspbian.nondebug/build/Lib/test/test_concurrent_futures/executor.py", line127, intest_map_buffersize_on_infinite_iterableself.assertEqual(next(res,None),"0")~~~~^^^^^^^^^^^  File"/var/lib/buildbot/workers/3.x.gps-raspbian.nondebug/build/Lib/concurrent/futures/_base.py", line666, inresult_iteratoryield _result_or_cancel(fs.pop())~~~~~~~~~~~~~~~~~^^^^^^^^^^  File"/var/lib/buildbot/workers/3.x.gps-raspbian.nondebug/build/Lib/concurrent/futures/_base.py", line311, in_result_or_cancelreturn fut.result(timeout)~~~~~~~~~~^^^^^^^^^  File"/var/lib/buildbot/workers/3.x.gps-raspbian.nondebug/build/Lib/concurrent/futures/_base.py", line450, inresultreturnself.__get_result()~~~~~~~~~~~~~~~~~^^  File"/var/lib/buildbot/workers/3.x.gps-raspbian.nondebug/build/Lib/concurrent/futures/_base.py", line395, in__get_resultraiseself._exceptionconcurrent.futures.interpreter.BrokenInterpreterPool:A thread initializer failed, the thread pool is not usable anymore

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@kumaraditya303kumaraditya303kumaraditya303 approved these changes

@rhettingerrhettingerAwaiting requested review from rhettingerrhettinger is a code owner

Assignees
No one assigned
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

4 participants
@eendebakpt@rhettinger@bedevere-bot@kumaraditya303
[8]ページ先頭
©2009-2025 Movatter.jp