◐ Shell
reader mode source ↗
Skip to content

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

Merged
pitrou merged 10 commits into
python:masterfrom
markshannon:move-exc-state-to-generator
Oct 22, 2017
Merged

Move exc state to generator. Fixes bpo-25612#1773
pitrou merged 10 commits into
python:masterfrom
markshannon:move-exc-state-to-generator

Conversation

@markshannon

@markshannon markshannon commented May 23, 2017 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

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

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

Copy link
Copy Markdown

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

mention-bot commented May 23, 2017

Copy link
Copy Markdown

@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 22:12
@markshannon

markshannon commented May 23, 2017

Copy link
Copy Markdown
Member Author

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

@njsmith

njsmith commented May 23, 2017

Copy link
Copy Markdown
Contributor

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

1st1
1st1 reviewed May 23, 2017
View reviewed changes

@1st1 1st1 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

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

@markshannon markshannon force-pushed the move-exc-state-to-generator branch from e0b55d4 to e19057d Compare May 23, 2017 23:34
@markshannon

markshannon commented May 23, 2017

Copy link
Copy Markdown
Member Author

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

@1st1

1st1 commented May 25, 2017

Copy link
Copy Markdown
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.

@markshannon markshannon force-pushed the move-exc-state-to-generator branch 2 times, most recently from 0cc764b to 800150c Compare June 11, 2017 11:04
@1st1 1st1 requested a review from pitrou October 10, 2017 23:16
@1st1

1st1 commented Oct 10, 2017

Copy link
Copy Markdown
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.

@1st1 1st1 requested a review from serhiy-storchaka October 10, 2017 23:18
@pitrou

pitrou commented Oct 11, 2017

Copy link
Copy Markdown
Member

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

@1st1

1st1 commented Oct 11, 2017

Copy link
Copy Markdown
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.

@markshannon

markshannon commented Oct 12, 2017

Copy link
Copy Markdown
Member Author

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

19 hidden items Load more…
@markshannon markshannon force-pushed the move-exc-state-to-generator branch from 800150c to 415121e Compare October 22, 2017 12:04
@markshannon markshannon force-pushed the move-exc-state-to-generator branch from 7929841 to e5ffc67 Compare October 22, 2017 12:19
@pitrou

pitrou commented Oct 22, 2017

Copy link
Copy Markdown
Member

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

@serhiy-storchaka

serhiy-storchaka commented Oct 22, 2017

Copy link
Copy Markdown
Member

Is the problem I described on the tracker resolved?

@markshannon

markshannon commented Oct 22, 2017

Copy link
Copy Markdown
Member Author

@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
@markshannon markshannon deleted the move-exc-state-to-generator branch October 23, 2017 07:57
@1st1

1st1 commented Oct 23, 2017

Copy link
Copy Markdown
Member

Thanks a lot, Mark, for looking into this.

tacaswell added a commit to tacaswell/cython that referenced this pull request Oct 28, 2017
@tacaswell
FIX: account for change in how exception information is stored …
fdf4451 
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
This was referenced Oct 28, 2017
Closed
Merged
scoder pushed a commit to cython/cython that referenced this pull request Oct 28, 2017
@tacaswell @scoder
FIX: account for change in how exception information is stored …
2d33924 
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
@snaury snaury mentioned this pull request Dec 3, 2017
Closed
akruis added a commit to stackless-dev/stackless that referenced this pull request Oct 13, 2018
@akruis
Stackless issue #149: Move exc state from thread to tasklet …
245a802 
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.
@ghost ghost mentioned this pull request Mar 29, 2019
Closed
@indrora indrora mentioned this pull request Jun 15, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka left review comments

@larryhastings larryhastings larryhastings left review comments

@1st1 1st1 1st1 approved these changes

@pitrou pitrou pitrou approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@markshannon @the-knights-who-say-ni @mention-bot @njsmith @1st1 @pitrou @serhiy-storchaka @larryhastings @Mariatta @bedevere-bot