Fix #131: Port to Py3.7: handle tstate->exc_info #132
Conversation
Fix Python 3.7 support: handle also tstate->exc_info, not only tstate->exc_state.
|
@markshannon: Would you mind to review my greenlet fix for Python 3.7, please? |
|
cc @hroncok |
There was a problem hiding this comment.
Looks good. I'm a little worried about correctness of exc_info links, but I guess since you can't enter generators twice it's good. There's no visiting of exceptions that are on generator chains, but it's all good since gc doesn't work for live greenlets anyway.
Might be better if we actually make greenlet's exc_state root of the hierarchy instead of tstate->exc_state, then we would only need to save/restore the exc_info pointer, not actual exception objects, although it may be harder to prove that it's always correct.
greenlet.c
Outdated
| #define GREENLET_tp_is_gc 0 | ||
| #endif /* !GREENLET_USE_GC */ | ||
|
|
||
| static void green_clear_exc(PyGreenlet *g) |
There was a problem hiding this comment.
Could you please change it to PyGreenlet* g?
greenlet.h
Outdated
| int recursion_depth; | ||
| PyObject* weakreflist; | ||
| #ifdef GREENLET_USE_EXC_INFO | ||
| _PyErr_StackItem *exc_info; |
There was a problem hiding this comment.
Here as well, _PyErr_StackItem* exc_info
greenlet.c
Outdated
| tstate->exc_state.exc_traceback = target->exc_traceback; | ||
| #ifdef GREENLET_USE_EXC_INFO | ||
| tstate->exc_state = target->exc_state; | ||
| if (target->exc_info != NULL) { |
There was a problem hiding this comment.
I think it might be better as a single line: tstate->exc_info = target->exc_info ? target->exc_info : &tstate->exc_state.
|
Oh, by the way, I tested manually my PR on Python 3.6 and 3.7 using virtual environments and the commands: where dagpool_test.py comes from: https://bugs.python.org/issue33996 dagpool_test.py is the highly simplified code of eventlet test "tests.dagpool_test.test_propagate_exc". |
|
Ha ha, the 3.7 job of Travis CI failed... on downloading Python! Can someone rebuild this job please? |
|
I just tested the master branch eventlet with the master branch of greenlet: "python -m nose -v tests" no longer crash with this greenlet change. In short, this change fixes eventlet/eventlet#475 |
|
I closed/reopened my PR just to re-trigger the Travis CI job. I would like to see a green job for Python 3.7 on Travis CI :-) |
|
Oh. 3.7 still fails with a download error on Travis CI, but it seems like the issue comes from Travis, not greenlet: |
|
Only alpha or beta on Travis with 3.7-dev |
Let me try with "3.7-dev" in .travis.yml. It's possible to get 3.7 final with more complex config, but I don't think that it's worth it here. 3.7-dev should be fine until Travis CI officially support 3.7 final. https://twitter.com/codewithanthony/status/1014168756342251521 |
|
Say hello to green 3.7 job on Travis CI! |
|
The two Python 3.7 jobs (32 and 64 bits) on Windows (AppVeyor) as also green ;-) |
Unfortunately neither Travis nor AppVeyor currently support Python 3.7 release. AppVeyor job actually lies, as it's currently testing and building using Python 2.7 (which is in PATH by default). :-/ |
|
Thanks for the patch, I will postpone release until AppVeyor releases new images (appveyor/ci#2475) |
See gevent/gevent#1069 Note that greenlet 0.4.13 is missing a change required for Python 3.7 [1] but we are still using Python 3.6 in production, so this shouldn't be a problem. [1] python-greenlet/greenlet#132
3 participants
Fix Python 3.7 support: handle also tstate->exc_info, not only
tstate->exc_state.