Repository navigation
C extensions can't swap out live frames anymore #90248
Description
| BPO | 46090 |
|---|---|
| Nosy | @gvanrossum, @warsaw, @markshannon, @jamadden, @pablogsal, @brandtbucher |
| PRs | PyThreadState.datastack_* members to be NULL #30234 |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
assignee = 'https://github.com/brandtbucher'
closed_at = <Date 2021-12-28.17:50:32.798>
created_at = <Date 2021-12-15.20:55:31.006>
labels = ['interpreter-core', '3.11']
title = "C extensions can't swap out live frames anymore"
updated_at = <Date 2021-12-28.17:50:32.798>
user = 'https://github.com/brandtbucher'bugs.python.org fields:
activity = <Date 2021-12-28.17:50:32.798>
actor = 'brandtbucher'
assignee = 'brandtbucher'
closed = True
closed_date = <Date 2021-12-28.17:50:32.798>
closer = 'brandtbucher'
components = ['Interpreter Core']
creation = <Date 2021-12-15.20:55:31.006>
creator = 'brandtbucher'
dependencies = []
files = []
hgrepos = []
issue_num = 46090
keywords = ['patch']
message_count = 9.0
messages = ['408646', '408802', '408803', '408819', '408825', '408826', '409005', '409058', '409262']
nosy_count = 6.0
nosy_names = ['gvanrossum', 'barry', 'Mark.Shannon', 'jmadden', 'pablogsal', 'brandtbucher']
pr_nums = ['30234']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue46090'
versions = ['Python 3.11']
Activity
brandtbucher commentedon Dec 15, 2021
I'm specifically concerned about Greenlet here (since it's a dependency of pyperformance), but this discussion is equally relevant to any library like it that dynamically swaps out the currently executing frame.
CPython 3.11 makes several major changes to how frame objects are used and represented internally:
These changes break Greenlet's hot-swapping of frame objects (https://github.com/python-greenlet/greenlet/blob/be41e1a24925326b72a02ef5cb6d1ed9643eb062/src/greenlet/greenlet_greenlet.hpp#L768-L811) in a pretty serious way, and it's not immediately clear what the best fix is. A fairly high-level overview of the new design:
When a frame is executing, the current thread state points to the current CFrame, which itself points to a C-level InterpreterFrame. This interpreter frame is located within a "data stack" that is managed by the thread state. If a PyFrameObject for the currently executing frame is requested (using PyThreadState_GetFrame), the new PyFrameObject points into the InterpreterFrame, where all of the important data is still stored. So far so good.
The issue is what happens next. If the InterpreterFrame is replaced, or exits in any way, its memory may be reused, or even freed entirely. The PyFrameObject is smart enough to copy the data elsewhere (into itself) when this happens, but the new design means that there is no obvious way for a third-party library to "reactivate" a frame in a way analogous to assigning to the old tstate->frame member.
While I'm pretty sure that we don't officially support this "feature", it's probably used often enough that we should at least brainstorm a workaround (if not an unstable C-API function) for affected projects. I suspect that a potential solution likely involves creating an entirely new InterpreterFrame the normal way, and copying the PyFrameObject's InterpreterFrame data into it? Not sure what would happen if the old InterpreterFrame was still alive on the data stack, though...
In any case, it probably makes sense to discuss here with affected library maintainers.
brandtbucher commentedon Dec 17, 2021
I'm going to take a stab at making this work today. The idea is to do something like this when setting a new frame:
Anyone see any obvious flaws here? I'm new to a lot of this code, so any guidance would be appreciated.
brandtbucher commentedon Dec 17, 2021
Also, it appears that in earlier versions of CPython, it was okay to set tstate->frame = NULL. Some of Greenlet's tests *seem* to be doing this (although it also possible that my current scaffolding has just broken Greenlet).
Does anybody know off the top of their head what it means to set a NULL frame? I can dig around through the old code and see what the interpreter does when that happens, but it seems pretty nonsensical to me. Is it something that only happens during interpreter/thread initialization/shutdown? Or maybe it keeps that thread from becoming active?
gvanrossum commentedon Dec 17, 2021
A thread without a frame looks like a natural end case when you consider the frames as a linked list. Could it be that exec()/eval() run in the current frame, and if there isn't a frame, they refuse to run? (Sorry, I don't recall anything more specific without going hunting through old code.)
pablogsal commentedon Dec 18, 2021
That is weird, but can happen for example if some pthread of some c extensión for example picks up the Gil, causing a new PyThreadState to be created and linked but has not called into Python yet.
Basically, an external native thread that calls
PyGILState_Ensure leaves a thread state with these characteristics.
pablogsal commentedon Dec 18, 2021
Relevant section:
https://docs.python.org/3/c-api/init.html#non-python-created-threads
brandtbucher commentedon Dec 22, 2021
Fixing this actually ended up being a lot simpler than I anticipated (and it requires no changes on our end). All that's really required is saving and restoring:
You can see my proposed fix for Greenlet here (this PR also contains some other 3.11-related fixes):
python-greenlet/greenlet#280
There is one weird edge case in Greenlet's use that I don't *entirely* understand, where it sometimes needs to update a thread with a NULL frame (as discussed above). We need to replace the thread's datastack with *something* in this case: setting it to NULL crashes the interpreter when it tries to allocate frames later, but leaving it as-is puts us in a situation where two threads are sharing the same datastack (also a recipe for crashes). My solution is to set it to a statically allocated zero-size "dummy chunk" in this case. It seems to work fine, since the zero-size forces the interpreter to allocate a "real" chunk on the next frame push.
If this solution seems too hack-y, a very simple change on our end could be to start treating NULL as a valid datastack value, and have the thread state check for it when pushing new chunks. That would make for a relatively clean upgrade path for libraries like Greenlet, and doesn't require adding any new APIs.
brandtbucher commentedon Dec 23, 2021
After further discussion with the team this morning, we decided that the proposed "dummy chunk" workaround is unacceptable (the interpreter may end up trying free it when shutting down a thread).
Allowing the tstate->datastack_* members to be NULL is very easy on our end, and even simplifies thread creation a tad (since we can defer the allocation of the datastack until the first frame is actually needed).
I'll have a PR up in a few minutes.
brandtbucher commentedon Dec 28, 2021
New changeset 77195cd by Brandt Bucher in branch 'main':
bpo-46090: Allow PyThreadState.datastack_* members to be NULL (GH-30234)
77195cd