gh-112182: Replace StopIteration with RuntimeError for future by ordinary-jamie · Pull Request #113220 · python/cpython

ordinary-jamie · 2023-12-16T23:23:36Z

When an StopIteration raises into asyncio.Future, this will cause a thread to hang. This commit address this by not raising an exception and silently transforming the StopIteration with a RuntimeError, which the caller can reconstruct from fut.exception().__cause__

Issue: StopIteration in a generator in a thread hanging on asyncio #112182

When an `StopIteration` raises into `asyncio.Future`, this will cause a thread to hang. This commit address this by not raising an exception and silently transforming the `StopIteration` with a `RuntimeError`, which the caller can reconstruct from `fut.exception().__cause__`

gvanrossum

Hey @ordinary-jamie, your Python's pretty good, your C could use a bit more care with reference counts and such. I think I caught everything. (BTW did you see the similar code in intrinsics.c in stopiteration_error()? It could serve as a useful comparison.)

@NewUserHa -- do you have any feedback on this PR? Do you have a version of your own that's further developed?

ordinary-jamie · 2023-12-24T02:09:22Z

Hey @ordinary-jamie, your Python's pretty good, your C could use a bit more care with reference counts and such. I think I caught everything.

Thanks for the detailed review @gvanrossum! Yes, sorry about my C, got a bit too eager to try and contribute before finishing my studies with CPython/C. I'll address your comments asap and let you know :)

(BTW did you see the similar code in intrinsics.c in stopiteration_error()? It could serve as a useful comparison.)

👀 thanks!

… asyncio-future-stopiteration

serhiy-storchaka

Agree with all of Guido's comments.

…-future-stopiteration

gvanrossum

I have a nit for the blurb, otherwise LGTM. But let's make sure @serhiy-storchaka also agrees (I might have missed something).

ordinary-jamie · 2024-01-03T23:42:47Z

Happy holidays @gvanrossum @serhiy-storchaka -- sorry for the slow response on this, just came back from leave.

Side question: should PyObject_CallOneArg be updated in the docs with the reference count annotation "Return value: New reference." (as is the case with PyObject_CallFunction et al.)

gvanrossum · 2024-01-04T00:15:57Z

Side question: should PyObject_CallOneArg be updated in the docs with the reference count annotation "Return value: New reference." (as is the case with PyObject_CallFunction et al.)

I have no idea. I don't even know where that is generated -- I don't see it in the .rst file.

ordinary-jamie · 2024-01-04T00:25:11Z

Side question: should PyObject_CallOneArg be updated in the docs with the reference count annotation "Return value: New reference." (as is the case with PyObject_CallFunction et al.)

I have no idea. I don't even know where that is generated -- I don't see it in the .rst file.

I believe its set here: Doc/data/refcounts.dat

edit: I can look into it and raise a separate issue ticket

…-future-stopiteration

ordinary-jamie · 2024-01-04T04:14:09Z

Sorry @gvanrossum -- missed out a fix in the Python stub

gvanrossum

LG -- waiting for @serhiy-storchaka

bedevere-app · 2024-01-04T19:25:30Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

kumaraditya303 · 2024-01-04T19:27:02Z

+        assert(PyExceptionInstance_Check(err));
+
+        PyException_SetCause(err, Py_NewRef(exc_val));
+        PyException_SetContext(err, Py_NewRef(exc_val));


You need set this to exc_val.__context__ not exc_val itself.

This was taken from the similar, existing code in intrinsics.c that also handles StopIteration for asyncio:

cpython/Python/intrinsics.c

Line 181 in 36aab34

PyException_SetContext(error, Py_NewRef(exc));

Would this need updating as well?

…-future-stopiteration

ordinary-jamie · 2024-01-04T20:06:57Z

Sorry about the mistakes @serhiy-storchaka and @kumaraditya303

I've addressed the comments, except for one RE: setting the context. The current code copies over the logic from PyObject *stopiteration_error in intrinsics.c so am unsure which way to go forward.

serhiy-storchaka

I was surprised that the Python code set exception.__context__. Was there any reason for this or it was a typo? There are no tests for this.

Besides this minor question, LGTM.

ordinary-jamie · 2024-01-04T20:20:04Z

I was surprised that the Python code set exception.__context__. Was there any reason for this or it was a typo? There are no tests for this.

typo 😳 sorry about that!

gvanrossum · 2024-01-10T02:42:43Z

I was surprised that the Python code set exception.__context__. Was there any reason for this or it was a typo? There are no tests for this.

typo 😳 sorry about that!

@ordinary-jamie I lost track of the state of this PR. Did you fix that typo yet?

ordinary-jamie · 2024-01-10T03:30:35Z

I was surprised that the Python code set exception.__context__. Was there any reason for this or it was a typo? There are no tests for this.

typo 😳 sorry about that!

@ordinary-jamie I lost track of the state of this PR. Did you fix that typo yet?

Yes! The typo is already fixed (sorry for the comment spam)

gvanrossum · 2024-01-10T05:57:04Z

Thanks @ordinary-jamie ! We'll get the other thing sorted too. Happy to work with you more.

…ython#113220) When an `StopIteration` raises into `asyncio.Future`, this will cause a thread to hang. This commit address this by not raising an exception and silently transforming the `StopIteration` with a `RuntimeError`, which the caller can reconstruct from `fut.exception().__cause__`

bharel · 2024-08-15T01:17:02Z

What version was this fixed in?
For some reason I still get it on 3.12.5 but not on 13rc1.
Did we forget backporting?

serhiy-storchaka · 2024-08-15T10:19:30Z

It was not backported to 3.12.

Taking into account that the current behavior is wrong, and it is very unlikely that any code depends on it (how can you depend on hanging code?), I think that it is reasonable to backport this change (if there was no other changes that can prevent this).

miss-islington-app · 2024-08-15T10:19:49Z

Thanks @ordinary-jamie for the PR, and @gvanrossum for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

…ythonGH-113220) When an `StopIteration` raises into `asyncio.Future`, this will cause a thread to hang. This commit address this by not raising an exception and silently transforming the `StopIteration` with a `RuntimeError`, which the caller can reconstruct from `fut.exception().__cause__` (cherry picked from commit 4826d52) Co-authored-by: Jamie Phan <jamie@ordinarylab.dev>

bedevere-app · 2024-08-15T10:19:58Z

GH-123033 is a backport of this pull request to the 3.12 branch.

…H-113220) (GH-123033) When an `StopIteration` raises into `asyncio.Future`, this will cause a thread to hang. This commit address this by not raising an exception and silently transforming the `StopIteration` with a `RuntimeError`, which the caller can reconstruct from `fut.exception().__cause__` (cherry picked from commit 4826d52) Co-authored-by: Jamie Phan <jamie@ordinarylab.dev>

…ython#113220) When an `StopIteration` raises into `asyncio.Future`, this will cause a thread to hang. This commit address this by not raising an exception and silently transforming the `StopIteration` with a `RuntimeError`, which the caller can reconstruct from `fut.exception().__cause__`

ordinary-jamie added 2 commits December 17, 2023 10:14

Add news

73c896a

ordinary-jamie requested review from 1st1, asvetlov, gvanrossum, kumaraditya303 and willingc as code owners December 16, 2023 23:23

bedevere-app Bot added the awaiting review label Dec 16, 2023

bedevere-app Bot mentioned this pull request Dec 16, 2023

StopIteration in a generator in a thread hanging on asyncio #112182

Closed

serhiy-storchaka self-requested a review December 17, 2023 10:06

gvanrossum reviewed Dec 24, 2023

View reviewed changes

Merge branch 'main' of https://github.com/ordinary-jamie/cpython into…

72e6637

… asyncio-future-stopiteration

serhiy-storchaka reviewed Dec 28, 2023

View reviewed changes

Comment thread Modules/_asynciomodule.c

ordinary-jamie added 2 commits January 3, 2024 14:35

Merge branch 'main' of https://github.com/python/cpython into asyncio…

eaa1a43

…-future-stopiteration

Address review comments

9da2320

gvanrossum approved these changes Jan 3, 2024

View reviewed changes

Comment thread Misc/NEWS.d/next/Library/2023-12-17-10-22-55.gh-issue-112182.jLWGlr.rst Outdated

bedevere-app Bot added awaiting merge and removed awaiting review labels Jan 3, 2024

Add :exc: markup to blurb

e74d8f7

ordinary-jamie added 2 commits January 4, 2024 15:12

Merge branch 'main' of https://github.com/python/cpython into asyncio…

341d811

…-future-stopiteration

Remove traceback chaining

9853c2a

gvanrossum approved these changes Jan 4, 2024

View reviewed changes

kumaraditya303 requested changes Jan 4, 2024

View reviewed changes

Comment thread Lib/asyncio/futures.py Outdated

bedevere-app Bot added awaiting changes and removed awaiting merge labels Jan 4, 2024

kumaraditya303 reviewed Jan 4, 2024

View reviewed changes

serhiy-storchaka reviewed Jan 4, 2024

View reviewed changes

Comment thread Lib/test/test_asyncio/test_futures.py Outdated

Comment thread Lib/test/test_asyncio/test_futures.py Outdated

Comment thread Modules/_asynciomodule.c Outdated

Comment thread Lib/asyncio/futures.py Outdated

ordinary-jamie added 2 commits January 5, 2024 06:51

Merge branch 'main' of https://github.com/python/cpython into asyncio…

677a99a

…-future-stopiteration

fix typos and update test

ce2e645

serhiy-storchaka approved these changes Jan 4, 2024

View reviewed changes

bedevere-app Bot added awaiting merge and removed awaiting changes labels Jan 4, 2024

ordinary-jamie requested a review from kumaraditya303 January 4, 2024 20:20

gvanrossum merged commit 4826d52 into python:main Jan 10, 2024

bedevere-app Bot removed the awaiting merge label Jan 10, 2024

serhiy-storchaka added the needs backport to 3.12 only security fixes label Aug 15, 2024

bedevere-app Bot removed the needs backport to 3.12 only security fixes label Aug 15, 2024

Uh oh!

Uh oh!

Conversation

ordinary-jamie commented Dec 16, 2023 • edited by bedevere-app Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gvanrossum left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ordinary-jamie commented Dec 24, 2023

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gvanrossum left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ordinary-jamie commented Jan 3, 2024

Uh oh!

gvanrossum commented Jan 4, 2024

Uh oh!

ordinary-jamie commented Jan 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ordinary-jamie commented Jan 4, 2024

Uh oh!

gvanrossum left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bedevere-app Bot commented Jan 4, 2024

Uh oh!

kumaraditya303 Jan 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ordinary-jamie Jan 4, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ordinary-jamie commented Jan 4, 2024

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

ordinary-jamie commented Jan 4, 2024

Uh oh!

gvanrossum commented Jan 10, 2024

Uh oh!

ordinary-jamie commented Jan 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gvanrossum commented Jan 10, 2024

Uh oh!

bharel commented Aug 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

serhiy-storchaka commented Aug 15, 2024

Uh oh!

miss-islington-app Bot commented Aug 15, 2024

Uh oh!

bedevere-app Bot commented Aug 15, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

ordinary-jamie commented Dec 16, 2023 •

edited by bedevere-app Bot

Loading

ordinary-jamie commented Jan 4, 2024 •

edited

Loading

kumaraditya303 Jan 4, 2024 •

edited

Loading

ordinary-jamie commented Jan 10, 2024 •

edited

Loading

bharel commented Aug 15, 2024 •

edited

Loading