NotificationsYou must be signed in to change notification settings
Fork32k
Star67.3k

GH-98388: Add tests for happy eyeballs and its internal workings#98389

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

Jump to bottom

Open

twisteroidambassador wants to merge11 commits intopython:main

base:main

Choose a base branch

fromtwisteroidambassador:happy_eyeballs_checkup

Open

GH-98388: Add tests for happy eyeballs and its internal workings#98389

twisteroidambassador wants to merge11 commits intopython:mainfromtwisteroidambassador:happy_eyeballs_checkup

+195 −0

Conversation

Copy link

Contributor

twisteroidambassador commentedOct 18, 2022•
edited by gvanrossum
Loading

GH-98388

Add tests for happy eyeballs and its internal workings.

d8b4492

One of the new tests may fail intermittently due topython#86296.

twisteroidambassador requested review from1st1,asvetlov andgvanrossum ascode owners

October 18, 2022 11:29

bedevere-bot added awaiting review testsTests in the Lib/test dir labels

Oct 18, 2022

gvanrossum changed the title~~Add tests for happy eyeballs and its internal workings.~~GH-98388: Add tests for happy eyeballs and its internal workings

Oct 18, 2022

gvanrossum added the skip news label

Oct 18, 2022

gvanrossum requested changes

Oct 18, 2022

View reviewed changes

Copy link

Member

gvanrossum left a comment

Choose a reason for hiding this comment

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

I've added the "skip news" label because this is a test-only PR.

Lib/test/test_asyncio/test_base_events.py OutdatedShow resolvedHide resolved

bedevere-bot removed the awaiting review label

Oct 18, 2022

Copy link

bedevere-bot commentedOct 18, 2022

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

bedevere-bot added the awaiting changes label

Oct 18, 2022

twisteroidambassador added4 commits

October 18, 2022 23:48

Correct typo in filename.

34a8f6f

Improve readability for _interleave_addrinfos tests.

c0e179e

Rename _interleave_addrinfos tests to match the method being tested.

8270e69

Made test_simultaneous_success_fail fail more consistently (I hope).

afb6abf

Copy link

ContributorAuthor

twisteroidambassador commentedOct 18, 2022

I have made the test that exposes#86296 fail more consistently on my computer. Let's see whether these CI tests fail.

I guess the next thing to consider is: What to do with this failing test? On 3.11 and above I can replacewait_for withtimeout, and I can confirm that fixes the issue, and the test passes. On older releases we don't havetimeout.

Copy link

Member

gvanrossum commentedOct 18, 2022

On my Intel Mac I don't see these tests fail yet.

But it seems they are failing in CI, so I believe you.

I'd say let's first prepare a fix that uses timeout and see if that passes everything.

For the 3.10 backport we have to do something completely different. I wonder if the key problem here isn't the one that caused so much trouble for the Semaphore class as well -- you can await a future, and catch CancelledError, but that doesn't mean the future was cancelled, it could mean the current task was cancelledafter the future already completed. Maybe there's still a path through the code that doesn't account for that, somehow?

Replacewait_for withwait.

513f7f9

This solution is compatible with all Python versions, and should pass all tests.

Copy link

ContributorAuthor

twisteroidambassador commentedOct 19, 2022

I got some inspiration from my past comments, and implemented a fix that's available on older Python versions as well.

The general approach is probably applicable to more situations: by waiting on the inner task withasyncio.wait, its exceptions and cancellation won't be raised automatically, and any exception caught in the outer task are definitely from the outer task itself. Oncewait returns, the inner task can be inspected.

twisteroidambassador mentioned this pull request

Oct 19, 2022

AsyncIO's wait_for can hide cancellation in a rare race condition#86296

Closed

kumaraditya303 requested changes

Feb 12, 2023

View reviewed changes

Copy link

Contributor

kumaraditya303 left a comment

Choose a reason for hiding this comment

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

Once#98518 lands, please remove the implementation changes and only add tests.

Copy link

Contributor

willingc commentedApr 26, 2023

Hi@twisteroidambassador,

Once#98518 lands, please remove the implementation changes and only add tests.

As mentioned by@kumaraditya303: Now that#98518 has landed, would you like to modify this PR to only include the tests? Thanks.

Delete staggered.py

079eec8

bedevere-bot mentioned this pull request

Apr 26, 2023

Tests for happy eyeballs + working around bug in wait_for#98388

Open

Merge branch 'main' into happy_eyeballs_checkup

ec144fb

Copy link

Member

arhadthedev commentedApr 26, 2023

The OP has no activity since January.

Copy link

Member

arhadthedev commentedApr 26, 2023

Well, working with diff using the mobile version of GitHub is extremely hard. I'll restore the deleted file in an hour after getting to a proper computer with a proper mouse.

arhadthedev added2 commits

April 26, 2023 19:50

Restore staggered.py frommain branch

5ec7d65

Address patchcheck report

4e9f06a

arhadthedev requested a review fromwillingc as acode owner

April 26, 2023 18:54

arhadthedev requested review fromkumaraditya303 andgvanrossum

April 26, 2023 19:27

Merge branch 'main' into happy_eyeballs_checkup

62387d6

gvanrossum added the needs backport to 3.12only security fixes label

May 23, 2023

gvanrossum reviewed

May 24, 2023

View reviewed changes

Copy link

Member

gvanrossum left a comment

Choose a reason for hiding this comment

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

This basically LGTM, I only have very minor nits.

@arhadthedev, would you take over this PR, since@twisteroidambassador seems to have disappeared? I think it would be good to land this and backport it to 3.12. At the very least you could review my suggestions, if you agree with everything I can merge.

@willingc Did you want to add anything?

Lib/test/test_asyncio/test_base_events.py

		FOUR_C = (socket.AF_INET, 0, 0, '', ('192.0.2.3', 7))
		FOUR_D = (socket.AF_INET, 0, 0, '', ('192.0.2.4', 8))

		addrinfos = [SIX_A, SIX_B, SIX_C, SIX_D, FOUR_A, FOUR_B, FOUR_C, FOUR_D]

Copy link

Member

gvanrossumMay 23, 2023

Choose a reason for hiding this comment

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

Would it be a more thorough test if we mixed up the order a bit, e.g. like this?

Suggested change

	addrinfos= [SIX_A,SIX_B,SIX_C,SIX_D,FOUR_A,FOUR_B,FOUR_C,FOUR_D]
	addrinfos= [SIX_A,SIX_B,SIX_C,FOUR_A,FOUR_B,FOUR_C,FOUR_D,SIX_D]

Copy link

Contributor

willingcMay 25, 2023

Choose a reason for hiding this comment

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

LGTM@gvanrossum.

Lib/test/test_asyncio/test_staggered.py

Comment on lines +100 to +103

		# There's a potential race condition here:
		# https://github.com/python/cpython/issues/86296
		# As with any race condition, it can be difficult to reproduce.
		# This test may not fail every time.

Copy link

Member

gvanrossumMay 24, 2023

Choose a reason for hiding this comment

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

Please add a note to this comment that the race condition has been fixed in 3.12, e.g.

Suggested change

	# There's a potential race condition here:
	# https://github.com/python/cpython/issues/86296
	# As with any race condition, it can be difficult to reproduce.
	# This test may not fail every time.
	# There's a potential race condition here (fixed in Python 3.12):
	# https://github.com/python/cpython/issues/86296
	# As with any race condition, it can be difficult to reproduce.
	# This test may not fail every time.

Copy link

ContributorAuthor

twisteroidambassador commentedJun 21, 2023

It would be great if@arhadthedev can take over this PR, since I don't think I'll have time to work on this in the foreseeable future. Thanks in advance to everyone!