Skip to content

Conversation

@graingert
Copy link
Contributor

@graingertgraingert commented Jan 4, 2025

Issue currently pending, but this fixes the following exception when running under anyio's pytest plugin, or logging unhandled asyncio exceptions:

https://github.com/Chia-Network/chia-blockchain/actions/runs/12586550910/job/35084132471#step:16:1813

 ______ test_long_reorg_nodes[ConsensusMode.HARD_FORK_2_0-2-500-100-True] _______ [gw1] darwin -- Python 3.12.8 /Users/runner/work/chia-blockchain/chia-blockchain/.venv/bin/python /Library/Frameworks/Python.framework/Versions/3.12/lib/python3.12/asyncio/staggered.py:104: in run_one_coro result = await coro_fn() .venv/lib/python3.12/site-packages/aiohappyeyeballs/impl.py:166: in _connect_sock await loop.sock_connect(sock, address) /Library/Frameworks/Python.framework/Versions/3.12/lib/python3.12/asyncio/selector_events.py:651: in sock_connect return await fut E asyncio.exceptions.CancelledError During handling of the above exception, another exception occurred: .venv/lib/python3.12/site-packages/anyio/pytest_plugin.py:160: in pytest_pyfunc_call runner.run_test(pyfuncitem.obj, testargs) .venv/lib/python3.12/site-packages/anyio/_backends/_asyncio.py:2316: in run_test self._raise_async_exceptions() .venv/lib/python3.12/site-packages/anyio/_backends/_asyncio.py:2220: in _raise_async_exceptions raise exceptions[0] /Library/Frameworks/Python.framework/Versions/3.12/lib/python3.12/asyncio/staggered.py:108: in run_one_coro exceptions[this_index] = e E NameError: cannot access free variable 'exceptions' where it is not associated with a value in enclosing scope 

for task in running_tasks:
task.cancel(*ex.args)
on_completed_fut = None
if __debug__ and unhandled_exceptions:
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

probably this should just always raise it, not sure why this was suppressed in the original code

@graingertgraingert changed the title fix asyncio staggered leaking tasks, and logging unhandled exception.append exceptiongh-128479: fix asyncio staggered leaking tasks, and logging unhandled exception.append exceptionJan 4, 2025
@graingertgraingert added needs backport to 3.12 only security fixes needs backport to 3.13 bugs and security fixes labels Jan 4, 2025
@graingertgraingert marked this pull request as ready for review January 4, 2025 11:13
@graingertgraingert changed the title gh-128479: fix asyncio staggered leaking tasks, and logging unhandled exception.append exceptiongh-128479: fix asyncio staggered race leaking tasks, and logging unhandled exception.append exceptionJan 4, 2025
Copy link
Member

@ZeroIntensityZeroIntensity left a comment

Choose a reason for hiding this comment

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

I haven't stayed up-to-date with the staggered_race problems. Did we adopt some of aiohttp's tests?

@graingert
Copy link
ContributorAuthor

I haven't stayed up-to-date with the staggered_race problems. Did we adopt some of aiohttp's tests?

Did you mean aiohappyeyeballs? No those have yet to be added

@ZeroIntensity
Copy link
Member

Ah yeah, aiohappyeyeballs. Can we run their (old) test suite against this PR? I don't want another release blocker fiasco.

@graingert
Copy link
ContributorAuthor

graingert commented Jan 4, 2025

@ZeroIntensity I got it running against the newest suite: graingert/aiohappyeyeballs@8587b00#diff-d38ea0c7de1f2c4e6030c2e14c786b426be1f20310d14997973c0c1f7621d0e4L33

but it failed on a test that multiple winners can complete. This is not acceptable for the cpython implementation, because it would result in connection objects being left unclosed and result in ResourceWarnings.

I'm not sure which test suite is the old test suite you refer to, do you have a commit hash for that?

@ZeroIntensity
Copy link
Member

I'm not sure which test suite is the old test suite you refer to, do you have a commit hash for that?

I don't have a commit hash, but I mean "old" as in when aiohappyeyeballs was still using our staggered_race instead of their own implementation. But, if you just replaced their staggered_race with ours then I'm comfortable.

Copy link
Member

@ZeroIntensityZeroIntensity left a comment

Choose a reason for hiding this comment

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

Sorry for dropping the ball here. Looks mostly good, with a few edge cases/nitpicks to be dealt with.

@ambvambv merged commit ec91e1c into python:mainJan 23, 2025
38 checks passed
@miss-islington-app
Copy link

Thanks @graingert for the PR, and @ambv for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12, 3.13.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jan 23, 2025
…g unhandled exception.append exception (pythonGH-128475) (cherry picked from commit ec91e1c) Co-authored-by: Thomas Grainger <[email protected]> Co-authored-by: Peter Bierma <[email protected]>
@bedevere-app
Copy link

GH-129227 is a backport of this pull request to the 3.13 branch.

@bedevere-appbedevere-appbot removed the needs backport to 3.13 bugs and security fixes label Jan 23, 2025
@graingertgraingert deleted the fix-exeptions-append-after-cancel-async-stagger branch January 23, 2025 15:54
@miss-islington-app
Copy link

Sorry @graingert and @ambv, I had trouble completing the backport.
Please retry by removing and re-adding the "needs backport to 3.13" label.
Please backport backport using cherry_picker on the command line.

cherry_picker ec91e1c2762412f1408b0dfb5d281873b852affe 3.13 

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jan 23, 2025
…g unhandled exception.append exception (pythonGH-128475) (cherry picked from commit ec91e1c) Co-authored-by: Thomas Grainger <[email protected]> Co-authored-by: Peter Bierma <[email protected]>
@bedevere-app
Copy link

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

@bedevere-appbedevere-appbot removed the needs backport to 3.12 only security fixes label Jan 23, 2025
@ambv
Copy link
Contributor

ambv commented Jan 23, 2025

The misleading comment by miss-islington is actually an improvement over the previous behavior. There were HTTP timeouts in PR creation so Stamina retried them a few times, not getting a response in time. The final retry returned HTTP 422 since one of the previous retries actually managed to create a PR. And this HTTP 422 is the basis of miss-islington's comment about not being able to complete the backport.

This is all due to an ongoing GitHub incident with Actions:
Screenshot 2025-01-23 at 17 01 52

ambv pushed a commit that referenced this pull request Jan 23, 2025
…ng unhandled exception.append exception (GH-128475) (#129227) gh-128479: fix asyncio staggered race leaking tasks, and logging unhandled exception.append exception (GH-128475) (cherry picked from commit ec91e1c) Co-authored-by: Thomas Grainger <[email protected]> Co-authored-by: Peter Bierma <[email protected]>
ambv pushed a commit that referenced this pull request Jan 23, 2025
…ng unhandled exception.append exception (GH-128475) (#129228) gh-128479: fix asyncio staggered race leaking tasks, and logging unhandled exception.append exception (GH-128475) (cherry picked from commit ec91e1c) Co-authored-by: Thomas Grainger <[email protected]> Co-authored-by: Peter Bierma <[email protected]>
bdraco added a commit to aio-libs/aiohappyeyeballs that referenced this pull request Mar 3, 2025
related aiohttp issue aio-libs/aiohttp#10506 In #101 we replaced the staggered race implementation since the cpython version had races that were not fixed at the time. cpython has since updated the implementation to fix additional races. Our current implementation still has problems with cancellation and cpython has fixed that in python/cpython#128475 and python/cpython#124847 This PR ports the latest cpython implementation
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@graingert@ZeroIntensity@ambv