Skip to content

Fix an outdated test in asyncio - #24477

Merged
kumaraditya303 merged 2 commits into
python:mainfrom
fantix:fix-outdated-test
Dec 8, 2022
Merged

Fix an outdated test in asyncio#24477
kumaraditya303 merged 2 commits into
python:mainfrom
fantix:fix-outdated-test

Conversation

@fantix

@fantixfantix commented Feb 7, 2021

Copy link
Copy Markdown
Contributor

The test was added in f3e2e092 before _StopError was removed in 41f69f4, after which this test will not fail without the fix it covers. This PR fixes the test to resume its coverage.

  • skip issue
  • skip news

Refs: MagicStack/uvloop#337

The test was added in f3e2e092 before `_StopError` was removed in
41f69f4, after which this test will not fail without the fix it covers.
This PR fixes the test to resume its coverage.
@github-actions

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity.

@github-actionsgithub-actionsBot added the stale Stale PR or inactive for long period of time. label Mar 10, 2021

@MaxwellDupreMaxwellDupre left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ran 2287 tests in 83.131s
OK (skipped=18) All windows tests
Also hit base_events.
Looks good.

@netlify

netlifyBot commented Dec 8, 2022

Copy link
Copy Markdown

Deploy Preview for python-cpython-preview canceled.

NameLink
🔨 Latest commitc793590
🔍 Latest deploy loghttps://app.netlify.com/sites/python-cpython-preview/deploys/639196c70ff0c10009fefc96

self.loop.run_forever()
except KeyboardInterrupt:
pass
self.loop.call_later(0.01, func)

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.

A bit unfortunate that we had to add a 10 msec delay here -- in most cases that will waste 10 msec and occasionally it will not wait long enough and the test will flake-fail. Why won't call_soon work?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Background: this test covers a bug that base exceptions like KeyboardInterrupt will cause the loop to be closed twice, leading to an early exit in the next loop run.

This test was added before _StopError was dropped, at that time the loop will stop as soon as _StopError is raised, even if the _ready queue still has a handle (which is the next run - the func() here), therefore this was a good test. But now the loop will exhaust the _ready queue before stopping, so this test couldn't cover the bug anymore without this PR. That's also why we cannot simply add func() into the _ready queue by call_soon().

You're right about the flakyness of using call_later() tho. Now I think a better fix would be to use chained call_soon() to skip the first iteration where the buggy stop occured:

Suggested change
self.loop.call_later(0.01, func)
# If the issue persists, the loop will exit early after the first iteration,
# in that case our func() in the second iteration won't be called.
self.loop.call_soon(self.loop.call_soon, func)

I verified and this error fails if the fix in f3e2e09 is reverted.

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.

Clever solution, go ahead and make it a PR, assign to me or @kumaraditya303 for review.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip issueskip newstestsTests in the Lib/test dirtopic-asyncio

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@fantix@gvanrossum@MaxwellDupre@iritkatriel@the-knights-who-say-ni@ezio-melotti@bedevere-bot@kumaraditya303