Skip to content

gh-128479: fix asyncio staggered race leaking tasks, and logging unhandled exception.append exception #128475

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 16 commits into from
Jan 23, 2025
Merged

gh-128479: fix asyncio staggered race leaking tasks, and logging unhandled exception.append exception #128475

merged 16 commits into from
Jan 23, 2025

Conversation

graingert
Copy link
Contributor

@graingert graingert commented Jan 4, 2025 •
edited by bedevere-app bot
Loading

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

graingert
graingert commented Jan 4, 2025
View reviewed changes
Lib/asyncio/staggered.py
for task in running_tasks:
task.cancel(*ex.args)
on_completed_fut = None
if __debug__ and unhandled_exceptions:
Copy link
Contributor Author

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

@graingert graingert changed the title fix asyncio staggered leaking tasks, and logging unhandled exception.append exception gh-128479: fix asyncio staggered leaking tasks, and logging unhandled exception.append exception Jan 4, 2025
@bedevere-app bedevere-app bot mentioned this pull request Jan 4, 2025
Closed
@graingert graingert added needs backport to 3.12 only security fixes needs backport to 3.13 bugs and security fixes labels Jan 4, 2025
@graingert graingert marked this pull request as ready for review January 4, 2025 11:13
@graingert graingert changed the title gh-128479: fix asyncio staggered leaking tasks, and logging unhandled exception.append exception gh-128479: fix asyncio staggered race leaking tasks, and logging unhandled exception.append exception Jan 4, 2025
ZeroIntensity
ZeroIntensity reviewed Jan 4, 2025
View reviewed changes
Copy link
Member

@ZeroIntensity ZeroIntensity 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 @ZeroIntensity
Update Misc/NEWS.d/next/Library/2025-01-04-11-10-04.gh-issue-128479.j…
4a4c027 
…vOrF-.rst

Co-authored-by: Peter Bierma <zintensitydev@gmail.com>
@graingert
Copy link
Contributor Author

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
Contributor Author

graingert commented Jan 4, 2025 •
edited
Loading

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

ZeroIntensity
ZeroIntensity reviewed Jan 4, 2025
View reviewed changes
graingert
graingert commented Jan 4, 2025
View reviewed changes
ZeroIntensity
ZeroIntensity reviewed Jan 6, 2025
View reviewed changes
Copy link
Member

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

graingert
graingert commented Jan 6, 2025
View reviewed changes
graingert
graingert commented Jan 6, 2025
View reviewed changes
@ambv ambv merged commit ec91e1c into python:main Jan 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
@graingert @ZeroIntensity @miss-islington
pythongh-128479: fix asyncio staggered race leaking tasks, and loggin…
2234a53 
…g unhandled exception.append exception (pythonGH-128475)

(cherry picked from commit ec91e1c)

Co-authored-by: Thomas Grainger <tagrain@gmail.com>
Co-authored-by: Peter Bierma <zintensitydev@gmail.com>
@bedevere-app
Copy link

bedevere-app bot commented Jan 23, 2025

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

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Jan 23, 2025
@graingert graingert 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
@graingert @ZeroIntensity @miss-islington
pythongh-128479: fix asyncio staggered race leaking tasks, and loggin…
d5053a3 
…g unhandled exception.append exception (pythonGH-128475)

(cherry picked from commit ec91e1c)

Co-authored-by: Thomas Grainger <tagrain@gmail.com>
Co-authored-by: Peter Bierma <zintensitydev@gmail.com>
@bedevere-app
Copy link

bedevere-app bot commented Jan 23, 2025

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

@bedevere-app bedevere-app bot 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
@miss-islington @graingert @ZeroIntensity
[3.13] gh-128479: fix asyncio staggered race leaking tasks, and loggi…
75ec6ca 
…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 <tagrain@gmail.com>
Co-authored-by: Peter Bierma <zintensitydev@gmail.com>
ambv pushed a commit that referenced this pull request Jan 23, 2025
@miss-islington @graingert @ZeroIntensity
[3.12] gh-128479: fix asyncio staggered race leaking tasks, and loggi…
e94939c 
…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 <tagrain@gmail.com>
Co-authored-by: Peter Bierma <zintensitydev@gmail.com>
bdraco added a commit to aio-libs/aiohappyeyeballs that referenced this pull request Mar 3, 2025
@bdraco
fix: update staggered race implementation to use latest cpython version
d248314 
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
@bdraco bdraco mentioned this pull request Mar 3, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ZeroIntensity ZeroIntensity ZeroIntensity approved these changes

@1st1 1st1 Awaiting requested review from 1st1 1st1 is a code owner

@asvetlov asvetlov Awaiting requested review from asvetlov asvetlov is a code owner

@kumaraditya303 kumaraditya303 Awaiting requested review from kumaraditya303 kumaraditya303 is a code owner

@willingc willingc Awaiting requested review from willingc willingc is a code owner

Assignees

@ambv ambv

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@graingert @ZeroIntensity @ambv