gh-153364: Make frame, coroutine, and task-waiter chain walks iterative and bounded - #153365
Merged
pablogsal merged 31 commits intoOct 4, 2026
Merged
Conversation
maurycy
commented
Jul 8, 2026
maurycy
commented
Jul 8, 2026
| #define MAX_STACK_CHUNK_SIZE (16 * 1024 * 1024) /* 16 MB max for stack chunks */ | ||
| #define MAX_LONG_DIGITS 64 /* Allows values up to ~2^1920 */ | ||
| #define MAX_SET_TABLE_SIZE (1 << 20) /* 1 million entries max for set iteration */ | ||
| #define MAX_FRAME_CHAIN_DEPTH (1024 + 512) /* Iteration bound for frame chain walks */ |
Contributor
Author
There was a problem hiding this comment.
Maybe MAX_THREADS, MAX_TLBC_SIZE, MAX_STACK_CHUNKS, MAX_LINETABLE_SIZE or MAX_ITERATIONS are worth keeping here too?
Contributor
Author
There was a problem hiding this comment.
While we're at it, should MAX_TLBC_SIZE be in sync with MAX_THREADS?
maurycy
changed the title
get_async_stack_trace() and parse_coro_chain()
pablogsal
requested changes
Jul 11, 2026
|
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 phrase |
pablogsal
reviewed
Sep 25, 2026
| set_exception_cause(unwinder, PyExc_RuntimeError, "Failed to read set entry ref count"); | ||
| uintptr_t key_addr = (uintptr_t)entry.key; | ||
| if (key_addr != 0 && entry.hash != -1) { | ||
| if (parse_task(unwinder, key_addr, awaited_by) < 0) { |
Member
There was a problem hiding this comment.
This still parses the complete awaited_by set before checking the waiter limit. Can we pass the remaining budget into iterate_set_entries() and check it before parse_task()?
| PyObject *task_info = PyList_GET_ITEM(result, i); | ||
| PyObject *waiters = PyStructSequence_GET_ITEM(task_info, 3); | ||
| for (Py_ssize_t j = 0; j < PyList_GET_SIZE(waiters); j++) { | ||
| if (PyList_GET_SIZE(result) >= MAX_TASK_WAITER_WALK_TASKS) { |
Member
There was a problem hiding this comment.
This still does not bound the total work, no? process_single_task_node() parses the complete awaited_by set via iterate_set_entries() (up to MAX_SET_TABLE_SIZE entries, each doing create_task_result() + a coro chain walk) before we get back to this check. Can we share the remaining budget with iterate_set_entries() and decrement it before every parse_task()? I am also fine doing this in a follow-up if you prefer.
This was referenced Oct 4, 2026
pablogsal
enabled auto-merge (squash)
October 4, 2026 23:57
pablogsal
added
needs backport to 3.14
|
Thanks @maurycy for the PR, and @pablogsal for merging it 🌮🎉.. I'm working now to backport this PR to: 3.14, 3.15. |
|
Sorry, @maurycy and @pablogsal, I could not cleanly backport this to Please backport manually with cherry_picker, see the devguide for more information. |
|
GH-158813 is a backport of this pull request to the 3.15 branch. |
bedevere-app
Bot
removed
the
needs backport to 3.15
|
GH-158814 is a backport of this pull request to the 3.14 branch. |
pablogsal
pushed a commit
that referenced
this pull request
Oct 5, 2026
…iterative and bounded (GH-153365) (#158813) gh-153364: Make frame, coroutine, and task-waiter chain walks iterative and bounded (GH-153365) * let me declare single limit * use our new limit in process_frame_chain() * add it in parse_async_frame_chain() * parse_coro_chain() * NEWS * async in the message? * test * no race * process_task_awaited_by * process_task_awaited_by limit test * NEWS * MAX_TASK_WAITER_CHAIN_DEPTH * TASK_WAITER_CHAIN_DEPTH in test * TASK_WAITER_CHAIN_DEPTH 256 * prevent the drift with the comment * better naming, better style * MAX_TASK_WAITER_CHAIN_DEPTH comment * task-waiter iterative bfs walk * iterative coro-walk * nicer news * 1 << 14 * comment * unused read_Py_ssize_t * fix tombstones * simplify * correct msg * better test * news for tombstones * left-over from when testing buggy version * redundant new line (cherry picked from commit e0861c6) Co-authored-by: Maurycy Pawłowski-Wieroński <maurycy@maurycy.com>
pablogsal
added a commit
that referenced
this pull request
Oct 5, 2026
…iterative and bounded (GH-153365) (#158814) gh-153364: Make frame, coroutine, and task-waiter chain walks iterative and bounded (#153365) * let me declare single limit * use our new limit in process_frame_chain() * add it in parse_async_frame_chain() * parse_coro_chain() * NEWS * async in the message? * test * no race * process_task_awaited_by * process_task_awaited_by limit test * NEWS * MAX_TASK_WAITER_CHAIN_DEPTH * TASK_WAITER_CHAIN_DEPTH in test * TASK_WAITER_CHAIN_DEPTH 256 * prevent the drift with the comment * better naming, better style * MAX_TASK_WAITER_CHAIN_DEPTH comment * task-waiter iterative bfs walk * iterative coro-walk * nicer news * 1 << 14 * comment * unused read_Py_ssize_t * fix tombstones * simplify * correct msg * better test * news for tombstones * left-over from when testing buggy version * redundant new line (cherry picked from commit e0861c6) Co-authored-by: Maurycy Pawłowski-Wieroński <maurycy@maurycy.com>
|
pablogsal
added a commit
to pablogsal/cpython
that referenced
this pull request
Oct 5, 2026
…r chain walks iterative and bounded (pythonGH-153365) (python#158813)" This reverts commit 9e401cf.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The PR hardens and cleans up frame, coro and task-waiter walks a bit, by making them iterative and bounded.
I'm addressing two issues here:
process_frame_chain()already had1024 + 512limit, but the PR extracts it asMAX_FRAME_CHAIN_DEPTHand applies in theparse_coro_chain(), used by both sync and async paths, andparse_async_frame_chain(). The task-waiter walk inget_async_stack_trace()gets a separate limit (MAX_TASK_WAITER_WALK_TASKS), on the total number of tasks visited (including duplicates), since its' a graph, not a chain.SIZEOF_TASK_OBJ) 256 was enough to overflow a 1MiB stack.The obvious context here is avoiding infinite loops. Truth be told, I think that in some places torn reads were also responsible for avoiding them and early exits. :-)
_remote_debugging: No frame limit inget_async_stack_trace()#153364