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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move exc state to generator. Fixes bpo-25612 #1773

Merged
merged 10 commits into from Oct 22, 2017

Conversation

Projects
None yet
10 participants
@markshannon
Contributor

markshannon commented May 23, 2017

Moves the exception state from the frame to the coroutine. (Coroutine in the CS sense, which includes generators, coroutines and thread). This cleans up the code, removing about 60 lines, and fixes https://bugs.python.org/issue25612.

https://bugs.python.org/issue25612

@the-knights-who-say-ni

This comment has been minimized.

Show comment
Hide comment
@the-knights-who-say-ni

the-knights-who-say-ni May 23, 2017

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!

the-knights-who-say-ni commented May 23, 2017

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!

@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot May 23, 2017

@markshannon, thanks for your PR! By analyzing the history of the files in this pull request, we identified @benjaminp, @serhiy-storchaka and @Yhg1s to be potential reviewers.

mention-bot commented May 23, 2017

@markshannon, thanks for your PR! By analyzing the history of the files in this pull request, we identified @benjaminp, @serhiy-storchaka and @Yhg1s to be potential reviewers.

@1st1 1st1 self-requested a review May 23, 2017

@markshannon

This comment has been minimized.

Show comment
Hide comment
@markshannon

markshannon May 23, 2017

Contributor

I've just linked by bpo user to my github user. Which should satisfy @the-knights-who-say-ni, hopefully.

Contributor

markshannon commented May 23, 2017

I've just linked by bpo user to my github user. Which should satisfy @the-knights-who-say-ni, hopefully.

@njsmith

This comment has been minimized.

Show comment
Hide comment
@njsmith

njsmith May 23, 2017

Contributor

You might also want to check how this affects this strange behavior that @arigo found: https://bugs.python.org/issue28884#msg282532

Contributor

njsmith commented May 23, 2017

You might also want to check how this affects this strange behavior that @arigo found: https://bugs.python.org/issue28884#msg282532

@1st1

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).

Show outdated Hide outdated Objects/genobject.c Outdated
Show outdated Hide outdated Include/pystate.h Outdated
Show outdated Hide outdated Include/pystate.h Outdated
Show outdated Hide outdated Objects/genobject.c Outdated
@markshannon

This comment has been minimized.

Show comment
Hide comment
@markshannon

markshannon May 23, 2017

Contributor

@njsmith It does indeed fix https://bugs.python.org/issue28884#msg282532. I've added it as a test case.

Contributor

markshannon commented May 23, 2017

@njsmith It does indeed fix https://bugs.python.org/issue28884#msg282532. I've added it as a test case.

Show outdated Hide outdated Include/pystate.h Outdated
Show outdated Hide outdated Python/errors.c Outdated
@1st1

This comment has been minimized.

Show comment
Hide comment
@1st1

1st1 May 25, 2017

Member

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.

Member

1st1 commented May 25, 2017

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.

Show outdated Hide outdated Objects/genobject.c Outdated
Show outdated Hide outdated Include/pystate.h Outdated
Show outdated Hide outdated Include/pystate.h Outdated

@Mariatta Mariatta added needs rebase and removed needs rebase labels Oct 9, 2017

@1st1 1st1 requested a review from pitrou Oct 10, 2017

@1st1

This comment has been minimized.

Show comment
Hide comment
@1st1

1st1 Oct 10, 2017

Member

@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.

Member

1st1 commented Oct 10, 2017

@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.

@1st1 1st1 requested a review from serhiy-storchaka Oct 10, 2017

Show outdated Hide outdated Include/pyerrors.h Outdated
Show outdated Hide outdated Include/pystate.h Outdated
@@ -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

This comment has been minimized.

@serhiy-storchaka

serhiy-storchaka Oct 11, 2017

Member

Py_LOCAL_INLINE(int)?

@serhiy-storchaka

serhiy-storchaka Oct 11, 2017

Member

Py_LOCAL_INLINE(int)?

This comment has been minimized.

@markshannon

markshannon Oct 22, 2017

Contributor

Now that we support C99, I think we ought to use standard C where possible.

@markshannon

markshannon Oct 22, 2017

Contributor

Now that we support C99, I think we ought to use standard C where possible.

Show outdated Hide outdated Objects/genobject.c Outdated
Show outdated Hide outdated Objects/genobject.c Outdated
Show outdated Hide outdated Python/errors.c Outdated
Show outdated Hide outdated Python/errors.c Outdated
Show outdated Hide outdated Python/pystate.c Outdated
Show outdated Hide outdated Python/pystate.c Outdated
Show outdated Hide outdated Python/errors.c Outdated
Show outdated Hide outdated Python/pystate.c Outdated
@pitrou

This comment has been minimized.

Show comment
Hide comment
@pitrou

pitrou Oct 11, 2017

Member

This looks good to me on the principle. It needs adressing @serhiy-storchaka 's comments, and also fixing the merge conflicts.

Member

pitrou commented Oct 11, 2017

This looks good to me on the principle. It needs adressing @serhiy-storchaka 's comments, and also fixing the merge conflicts.

@1st1

This comment has been minimized.

Show comment
Hide comment
@1st1

1st1 Oct 11, 2017

Member

@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.

Member

1st1 commented Oct 11, 2017

@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.

@markshannon

This comment has been minimized.

Show comment
Hide comment
@markshannon

markshannon Oct 12, 2017

Contributor

Not right now, but in the next week or so, yes.

Contributor

markshannon commented Oct 12, 2017

Not right now, but in the next week or so, yes.

@1st1

1st1 approved these changes Oct 22, 2017

@pitrou

pitrou approved these changes Oct 22, 2017

@pitrou

This comment has been minimized.

Show comment
Hide comment
@pitrou

pitrou Oct 22, 2017

Member

@markshannon, do you have push rights or would you like someone else to merge this PR?

Member

pitrou commented Oct 22, 2017

@markshannon, do you have push rights or would you like someone else to merge this PR?

Show outdated Hide outdated Include/pystate.h Outdated
Show outdated Hide outdated Objects/genobject.c Outdated
Show outdated Hide outdated Python/errors.c Outdated
Show outdated Hide outdated Python/errors.c Outdated
@serhiy-storchaka

This comment has been minimized.

Show comment
Hide comment
@serhiy-storchaka

serhiy-storchaka Oct 22, 2017

Member

Is the problem I described on the tracker resolved?

Member

serhiy-storchaka commented Oct 22, 2017

Is the problem I described on the tracker resolved?

@markshannon

This comment has been minimized.

Show comment
Hide comment
@markshannon

markshannon Oct 22, 2017

Contributor

@serhiy-storchaka I've just fixed it.
@pitrou I don't have push rights.

Contributor

markshannon commented Oct 22, 2017

@serhiy-storchaka I've just fixed it.
@pitrou I don't have push rights.

@pitrou pitrou merged commit ae3087c into python:master Oct 22, 2017

3 of 4 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
bedevere/issue-number Issue number 25612 found
Details
bedevere/news News entry found in Misc/NEWS.d
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@markshannon markshannon deleted the markshannon:move-exc-state-to-generator branch Oct 23, 2017

@1st1

This comment has been minimized.

Show comment
Hide comment
@1st1

1st1 Oct 23, 2017

Member

Thanks a lot, Mark, for looking into this.

Member

1st1 commented Oct 23, 2017

Thanks a lot, Mark, for looking into this.

tacaswell added a commit to tacaswell/cython that referenced this pull request Oct 28, 2017

FIX: account for change in how exception information is stored
CPython change how the exception information is stored internally in
3.7a3.

This simply adds fences based on python version around how to access
the objects.

Xref:

python/cpython@ae3087c
https://bugs.python.org/issue25612
python/cpython#1773

scoder added a commit to cython/cython that referenced this pull request Oct 28, 2017

FIX: account for change in how exception information is stored
CPython change how the exception information is stored internally in
3.7a3.

This simply adds fences based on python version around how to access
the objects.

Xref:

python/cpython@ae3087c
https://bugs.python.org/issue25612
python/cpython#1773

akruis pushed a commit to stackless-dev/stackless that referenced this pull request Oct 13, 2018

Anselm Kruis
Stackless issue #149: Move exc state from thread to tasklet
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment