Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

graingert
Copy link
Contributor

@graingertgraingert commentedJan 4, 2025•
edited by bedevere-appbot
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 futE   asyncio.exceptions.CancelledErrorDuring 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] = eE   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 titlefix 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.12only security fixes needs backport to 3.13bugs and security fixes labelsJan 4, 2025
@graingertgraingert marked this pull request as ready for reviewJanuary 4, 2025 11:13
@graingertgraingert changed the titlegh-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 thestaggered_race problems. Did we adopt some of aiohttp's tests?

…vOrF-.rstCo-authored-by: Peter Bierma <zintensitydev@gmail.com>
@graingert
Copy link
ContributorAuthor

I haven't stayed up-to-date with thestaggered_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 commentedJan 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 ourstaggered_race instead of their own implementation. But, if you just replaced theirstaggered_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 commitec91e1c intopython: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 requestJan 23, 2025
…g unhandled exception.append exception (pythonGH-128475)(cherry picked from commitec91e1c)Co-authored-by: Thomas Grainger <tagrain@gmail.com>Co-authored-by: Peter Bierma <zintensitydev@gmail.com>
@bedevere-app
Copy link

GH-129227 is a backport of this pull request to the3.13 branch.

@bedevere-appbedevere-appbot removed the needs backport to 3.13bugs and security fixes labelJan 23, 2025
@graingertgraingert deleted the fix-exeptions-append-after-cancel-async-stagger branchJanuary 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 usingcherry_picker on the command line.

cherry_picker ec91e1c2762412f1408b0dfb5d281873b852affe 3.13

miss-islington pushed a commit to miss-islington/cpython that referenced this pull requestJan 23, 2025
…g unhandled exception.append exception (pythonGH-128475)(cherry picked from commitec91e1c)Co-authored-by: Thomas Grainger <tagrain@gmail.com>Co-authored-by: Peter Bierma <zintensitydev@gmail.com>
@bedevere-app
Copy link

GH-129228 is a backport of this pull request to the3.12 branch.

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

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 requestJan 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 commitec91e1c)Co-authored-by: Thomas Grainger <tagrain@gmail.com>Co-authored-by: Peter Bierma <zintensitydev@gmail.com>
ambv pushed a commit that referenced this pull requestJan 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 commitec91e1c)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 requestMar 3, 2025
related aiohttp issueaio-libs/aiohttp#10506In#101 we replaced the staggered race implementation since the cpython version had racesthat were not fixed at the time. cpython has since updated the implementation to fixadditional races. Our current implementation still has problems with cancellationand cpython has fixed that inpython/cpython#128475andpython/cpython#124847This PR ports the latest cpython implementation
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@ZeroIntensityZeroIntensityZeroIntensity approved these changes

@1st11st1Awaiting requested review from 1st11st1 is a code owner

@asvetlovasvetlovAwaiting requested review from asvetlovasvetlov is a code owner

@kumaraditya303kumaraditya303Awaiting requested review from kumaraditya303kumaraditya303 is a code owner

@willingcwillingcAwaiting requested review from willingcwillingc is a code owner

Assignees

@ambvambv

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

3 participants
@graingert@ZeroIntensity@ambv
[8]ページ先頭
©2009-2025 Movatter.jp