Fix #392: Port to Python 3.13 #396
Conversation
vstinner
commented
Feb 14, 2024
vstinner
commented
- Replace C_RECURSION_LIMIT with Py_C_RECURSION_LIMIT.
- Add Py_C_RECURSION_LIMIT for Python 3.12 and older.
- Disable GREENLET_USE_CFRAME on Python 3.13.
- Define Py_BUILD_CORE to include pycore_frame.h.
* Replace C_RECURSION_LIMIT with Py_C_RECURSION_LIMIT. * Add Py_C_RECURSION_LIMIT for Python 3.12 and older. * Disable GREENLET_USE_CFRAME on Python 3.13. * Define Py_BUILD_CORE to include pycore_frame.h.
|
The second commit adds a Python 3.13 CI to GitHub Actions. It's to prove that my change works as expected. I can easily remove that change if you want to drop the 3.13 test for now. Well, IMO it's good to test as soon as possible ;-) |
|
Ah. While "tox -e py313" pass locally, it fails on the GitHub Action: I'm not sure about this change: + #if GREENLET_USE_CFRAME
tstate->cframe->current_frame = this->current_frame;
+ #endifWe should save/restore the current_frame, but I'm not sure how. |
|
Thanks for looking into this @vstinner ! I haven't had any time to keep up with Python 3.13 changes, but I can at least offer some insight into that crash. This is because that same test crashes immediately for me on my M2 Mac. Assuming the crash is the same, here's the backtrace: The invalid address causing the segfault is interesting... The crashing code in question: #if GREENLET_PY311
#if GREENLET_PY312
tstate->py_recursion_remaining = tstate->py_recursion_limit - this->py_recursion_depth;
tstate->c_recursion_remaining = Py_C_RECURSION_LIMIT - this->c_recursion_depth;
this->unexpose_frames(); // <- CRASH; line 205
#else // \/ 3.11
#if GREENLET_PY312
void GREENLET_NOINLINE(PythonState::unexpose_frames)()
{
if (!this->top_frame()) {
return;
}
// See GreenletState::expose_frames() and the comment on frames_were_exposed
// for more information about this logic.
_PyInterpreterFrame *iframe = this->_top_frame->f_frame;
while (iframe != nullptr) {
_PyInterpreterFrame *prev_exposed = iframe->previous;
assert(iframe->frame_obj);
memcpy(&iframe->previous, &iframe->frame_obj->_f_frame_data[0],
sizeof(void *));
iframe = prev_exposed;
}
}
#else
void PythonState::unexpose_frames()
{}
#endif IIRC, this all has to do with the way frames are "lazily" allocated now. We have to turn them into real frames so that we can get the complete stacks of suspended greenlets; that was added in greenlet 3.0.3 (previously, I'd just punted on that problem). I didn't write much of If I disable that functionality by ifdefing out |
|
I don't know how to move forward on this issue :-( |
|
The following code of PyFrameObject *frame = PyThreadState_GetFrame((PyThreadState *)tstate);
Py_XDECREF(frame); // PyThreadState_GetFrame gives us a new
// reference.
this->_top_frame.steal(frame);Why does it call Exactly: |
|
I hacked greenlet to hold a strong reference to the frame, but then another assertion failed... |
Exactly. IIRC, in most places we keep borrowed references, it's specifically so they can be deallocated. greenlets aren't required to ever finish or get switched back to before they get deallocated (this is surprisingly easy to arrange to happen). That means that any code on the C call stack that was going to decref something like a frame may never get a chance to run. So, while a greenlet is switched out, we essentially transfer ownership out of the C stack and into us. If we kept an extra strong reference, and the C stack code never runs again, we leak. This exposes implicit assumptions about the objects like that: we assume that "someone else" is keeping a strong reference to it, someone who may not run again to decref; or, put another way, the code that originally "owned" these objects won't run again to decref them until we switch the greenlet back in. It seems like something in this area has changed; for example, we're getting back fresh (transient) objects that have no other owner, or the place in the C stack that was keeping a strong reference is now keeping a borrowed reference. |
|
With Python 3.13 in beta, should we try to get this across? |
|
I just tested this as a patch for the Fedora package, but (when built for Python 3.13) there is still a problem with trying to access the |
|
I checked |
I updated my PR to use |
|
With this patch: diff --git a/src/greenlet/TPythonState.cpp b/src/greenlet/TPythonState.cpp
index bfb40ca..98855c5 100644
--- a/src/greenlet/TPythonState.cpp
+++ b/src/greenlet/TPythonState.cpp
@@ -146,8 +146,8 @@ void PythonState::operator<<(const PyThreadState *const tstate) noexcept
this->datastack_limit = tstate->datastack_limit;
PyFrameObject *frame = PyThreadState_GetFrame((PyThreadState *)tstate);
- Py_XDECREF(frame); // PyThreadState_GetFrame gives us a new
- // reference.
+ //Py_XDECREF(frame); // PyThreadState_GetFrame gives us a new
+ // // reference.
this->_top_frame.steal(frame);
#if GREENLET_PY313
this->delete_later = Py_XNewRef(tstate->delete_later);I get an assertion error that I don't know how to debug: Python is built in debug mode. |
|
I updated my PR and now the whole test suite pass on Python 3.13! Problem: GitHub Action updated macos-latest to arm64 arch which doesn't support old Python versions. I wrote #410 for that. |
|
This seems to work as a downstream patch in Fedora. Thank you! |
* https://www.python.org/download/pre-releases * https://github.com/actions/setup-python/blob/main/docs/advanced-usage.md#allow-pre-releases Blocked by: * python-greenlet/greenlet#396 Signed-off-by: Christian Clauss <cclauss@me.com>
|
What would be needed to move this forward? |
|
I have a feeling @jamadden is terribly busy |
|
I think we need to consider what to do if this repository is unmaintained. |
|
I am getting this error for green let for 3.13 is it related with the above mentioned : " pycore_frame.h(8): fatal error C1189: #error: "this header requires Py_BUILD_CORE define" |
|
I see @oremanj has committed to this repo in the past year. Maybe they can offer some options. |
|
Thank's everyone for your help on this, with a special thanks to @vstinner. I'll do my best to get a release out to PyPI (even if only an RC) by the end of the week. |
