Move exc state to generator. Fixes bpo-25612 by markshannon · Pull Request #1773 · python/cpython (original) (raw)
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 our terms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation51 Commits10 Checks0 Files changed
Conversation
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
[ Show hidden characters]({{ revealButtonHref }})
Hello, and thanks for your contribution!
I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).
Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.
Thanks again to your contribution and we look forward to looking at it!
Member
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this refactoring a lot. I'd go a step further and refactor tstate->*curexc_ too as well as add a couple of helper macros (not defined in limited API).
| @@ -115,6 +118,9 @@ gen_dealloc(PyGenObject *gen) |
|---|
| Py_CLEAR(gen->gi_code); |
| Py_CLEAR(gen->gi_name); |
| Py_CLEAR(gen->gi_qualname); |
| Py_CLEAR(gen->gi_exc_state.exc_type); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should add a macro PyExcState_CLEAR (and _VISIT)?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done (although I used inline functions)
| PyObject *exc_type; |
|---|
| PyObject *exc_value; |
| PyObject *exc_traceback; |
| PyExcState exc_state; |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that this is already quite an invasive change, maybe replace curexc_* fields with curexc_state?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PyExcState structs form a stack (hence the "previous" field), but there is only one current exception.
| @@ -150,6 +162,8 @@ typedef struct _ts { |
|---|
| PyObject *async_gen_firstiter; |
| PyObject *async_gen_finalizer; |
| PyExcState *exc_info; |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have exc_info, exc_state and curexc_* defined next to each other with comments explaining how we use each one of them? Since you've already spent time to figure this stuff out anyways.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| f->f_exc_type = NULL; |
|---|
| f->f_exc_value = NULL; |
| f->f_exc_traceback = NULL; |
| t = gen->gi_exc_state.exc_type; |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we had a PyExcState_CLEAR macro we could use it here.
| PyObject *exc_type; |
|---|
| PyObject *exc_value; |
| PyObject *exc_traceback; |
| /* The exception currently being handled, if no coroutines/generators are present. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are many lines that are longer than 79 (see PEP 7). Could you please fix this? </nitpicking>
| PyExcState *_PyErr_GetExcInfo(PyThreadState *tstate) { |
|---|
| PyExcState *exc_info = tstate->exc_info; |
| while ((exc_info->exc_type == NULL | |
| exc_info->exc_previous != NULL) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another nit, per PEP 7 we now want to use { even for blocks with one statement.
Aside from a few code formatting nits the patch looks good. I just want to test it on my machine before giving it a green light; will do that tomorrow or the day after.
| @@ -86,6 +93,19 @@ _PyGen_Finalize(PyObject *self) |
|---|
| PyErr_Restore(error_type, error_value, error_traceback); |
| } |
| static inline void PyExcState_clear(PyExcState *exc_state) { |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another minor nit: please rename to PyExcState_Clear.
| state inside the generator and the exception state of the calling |
|---|
| frame (which shouldn't be impacted when the generator "yields" |
| from an except handler). |
| These three fields exist exactly for that. */ |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this comment be left verbatim like this? The comment applied when these fields were part of the frame object, but they've been removed from the frame object completely. I don't claim to understand the patch completely but I assume the use of these fields have changed somewhat--otherwise what's the point of shuffling the data around?
| struct _excstate *exc_previous; |
| } PyExcState; |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you consider a longer name here? I always remember MvL saying "avoid abbreviations in identifiers, they are mystifying to non-English speakers". "PyExceptionState" isn't that awful is it?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this name is not public, it should be underscored.
A short name would LGTM, but the PyExc prefix is used only for exception classes. The PyException prefix is used for manipulating individual exception objects (PyExceptionClass_Check, PyException_GetTraceback). All other names related to exceptions and error handling have prefix PyErr.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PyExcState sounds ok to me.
@pitrou Antoine, could you please take a look? I'm inclined to merge this.
@markshannon Could you please rebase and add a NEWS entry? Would be cool if you could address the few review nits, but if you don't have time I can merge this as is and fix them myself.
| @@ -78,6 +78,7 @@ PyAPI_FUNC(void) PyErr_SetNone(PyObject *); |
|---|
| PyAPI_FUNC(void) PyErr_SetObject(PyObject *, PyObject *); |
| #ifndef Py_LIMITED_API |
| PyAPI_FUNC(void) _PyErr_SetKeyError(PyObject *); |
| PyExcState *_PyErr_GetExcInfo(PyThreadState *tstate); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PyAPI_FUNC()
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't intended to be an API function. It is only used in errors.c and sysmodule.c
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even for private functions we still use PyAPI_FUNC by convention (such as _PyErr_SetKeyError just above).
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it isn't meant to be part of an API. What does PyAPI_FUNC mean?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even for private functions we still use PyAPI_FUNC by convention (such as _PyErr_SetKeyError just above).
See _PyCoro_GetAwaitableIter and _PyGen_yf etc. I agree with @markshannon here, we don't really need PyAPI_FUNC.
| struct _excstate *exc_previous; |
| } PyExcState; |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this name is not public, it should be underscored.
A short name would LGTM, but the PyExc prefix is used only for exception classes. The PyException prefix is used for manipulating individual exception objects (PyExceptionClass_Check, PyException_GetTraceback). All other names related to exceptions and error handling have prefix PyErr.
| @@ -15,14 +15,22 @@ static char *NON_INIT_CORO_MSG = "can't send non-None value to a " |
|---|
| static char *ASYNC_GEN_IGNORED_EXIT_MSG = |
| "async generator ignored GeneratorExit"; |
| static inline int |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Py_LOCAL_INLINE(int)?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that we support C99, I think we ought to use standard C where possible.
| @@ -15,14 +15,22 @@ static char *NON_INIT_CORO_MSG = "can't send non-None value to a " |
|---|
| static char *ASYNC_GEN_IGNORED_EXIT_MSG = |
| "async generator ignored GeneratorExit"; |
| static inline int |
| PyExcState_traverse(PyExcState *exc_state, visitproc visit, void *arg) { |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move { on a separate line.
If PyExcState_traverse is private and local use the all-lowercase-letters style.
| @@ -86,6 +94,19 @@ _PyGen_Finalize(PyObject *self) |
|---|
| PyErr_Restore(error_type, error_value, error_traceback); |
| } |
| static inline void PyExcState_Clear(PyExcState *exc_state) { |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move static inline void (or Py_LOCAL_INLINE(int)?) and { on separate lines.
If PyExcState_Clear is private and local use the all-lowercase-letters style.
| @@ -52,6 +52,15 @@ PyErr_Restore(PyObject *type, PyObject *value, PyObject *traceback) |
|---|
| Py_XDECREF(oldtraceback); |
| } |
| PyExcState *_PyErr_GetExcInfo(PyThreadState *tstate) { |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move PyExcState * and { on separate lines.
| PyExcState *_PyErr_GetExcInfo(PyThreadState *tstate) { |
|---|
| PyExcState *exc_info = tstate->exc_info; |
| while ((exc_info->exc_type == NULL | |
| exc_info->exc_previous != NULL) { |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move { on a separate line (new PEP 7 rule for multiline conditions).
| Py_CLEAR(tstate->exc_state.exc_value); |
|---|
| Py_CLEAR(tstate->exc_state.exc_traceback); |
| assert(tstate->exc_info->exc_previous == NULL); |
| if (Py_VerboseFlag && tstate->exc_info != &tstate->exc_state) { |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Py_VerboseFlag only is used for import related tracing.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then that code should be fixed too.
| Py_CLEAR(tstate->exc_state.exc_traceback); |
|---|
| assert(tstate->exc_info->exc_previous == NULL); |
| if (Py_VerboseFlag && tstate->exc_info != &tstate->exc_state) { |
| fprintf(stderr, |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can PyErr_WarnEx() or PySys_WriteStderr() be used here?
| @@ -52,6 +52,15 @@ PyErr_Restore(PyObject *type, PyObject *value, PyObject *traceback) |
|---|
| Py_XDECREF(oldtraceback); |
| } |
| PyExcState *_PyErr_GetExcInfo(PyThreadState *tstate) { |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The result of this function is read-only, and exc_previous is not used. Wouldn't be better to return just three objects like in PyErr_GetExcInfo()? This would make the PyExcState structure more private.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I made it return three objects, it would be the same as PyErr_GetExcInfo()._PyErr_GetExcInfo() is provided as a way to handle the type, value, traceback trio in a reasonably modular fashion, without to much overhead.
As it is only used in errors.c and sysmodule.c, it is effectively private.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it would be the same as PyErr_GetExcInfo(), we should just use PyErr_GetExcInfo().
| Py_CLEAR(tstate->exc_state.exc_type); |
|---|
| Py_CLEAR(tstate->exc_state.exc_value); |
| Py_CLEAR(tstate->exc_state.exc_traceback); |
| assert(tstate->exc_info->exc_previous == NULL); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a comment here?
This looks good to me on the principle. It needs adressing @serhiy-storchaka 's comments, and also fixing the merge conflicts.
@markshannon Mark, will you be able to work on this in the following couple of weeks? If not, I can help resolve code review comments.
Not right now, but in the next week or so, yes.
…erator/thread) object where it belongs.
…t exception 'state' forms a stack.
1st1 approved these changes Oct 22, 2017
@markshannon, do you have push rights or would you like someone else to merge this PR?
| struct _exc_stackitem *exc_previous; |
| } _PyErr_StackItem; |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent names _exc_stackitem and _PyErr_StackItem.
| @@ -87,6 +96,20 @@ _PyGen_Finalize(PyObject *self) |
|---|
| PyErr_Restore(error_type, error_value, error_traceback); |
| } |
| static inline void exc_state_clear(_PyErr_StackItem *exc_state) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move static inline void on a separate line.
| @@ -53,6 +53,17 @@ PyErr_Restore(PyObject *type, PyObject *value, PyObject *traceback) |
|---|
| Py_XDECREF(oldtraceback); |
| } |
| _PyErr_StackItem *_PyErr_GetTopmostException(PyThreadState *tstate) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move _PyErr_StackItem * on a separate line.
| @@ -52,6 +52,15 @@ PyErr_Restore(PyObject *type, PyObject *value, PyObject *traceback) |
|---|
| Py_XDECREF(oldtraceback); |
| } |
| PyExcState *_PyErr_GetExcInfo(PyThreadState *tstate) { |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it would be the same as PyErr_GetExcInfo(), we should just use PyErr_GetExcInfo().
Is the problem I described on the tracker resolved?
Thanks a lot, Mark, for looking into this.
tacaswell added a commit to tacaswell/cython that referenced this pull request
This was referenced
Oct 28, 2017
scoder pushed a commit to cython/cython that referenced this pull request
akruis pushed a commit to stackless-dev/stackless that referenced this pull request
bpo-25612 (python#1773) moves the exception state information from frame object to coroutine (generator/thread) object where it belongs. As a consequence Stackless moves the exception state information for non-current tasklets from thread-state to the tasklet. This changes the pickle format of frame, tasklet and generator objects. The commit adds three test cases.
