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

twisteroidambassador · 2022-10-18T11:29:42Z

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

gvanrossum

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

Lib/test/test_asyncio/test_base_events.py

bedevere-bot · 2022-10-18T15:38:50Z

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 phrase I 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.

twisteroidambassador · 2022-10-18T17:41:48Z

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 replace wait_for with timeout, and I can confirm that fixes the issue, and the test passes. On older releases we don't have timeout.

gvanrossum · 2022-10-18T19:53:35Z

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 cancelled after the future already completed. Maybe there's still a path through the code that doesn't account for that, somehow?

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

twisteroidambassador · 2022-10-19T04:11:58Z

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 with asyncio.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. Once wait returns, the inner task can be inspected.

kumaraditya303

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

willingc · 2023-04-26T12:59:59Z

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.

arhadthedev · 2023-04-26T13:31:08Z

The OP has no activity since January.

arhadthedev · 2023-04-26T13:37:23Z

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.

gvanrossum

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?

gvanrossum · 2023-05-23T23:43:27Z

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]


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]

LGTM @gvanrossum.

gvanrossum · 2023-05-24T00:11:26Z

Lib/test/test_asyncio/test_staggered.py

+        # 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.


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.

twisteroidambassador · 2023-06-21T06:51:49Z

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!

Add tests for happy eyeballs and its internal workings.

d8b4492

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

twisteroidambassador requested review from 1st1, asvetlov and gvanrossum as code owners October 18, 2022 11:29

bedevere-bot added awaiting review tests Tests 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

Lib/test/test_asyncio/test_base_events.py Outdated Show resolved Hide resolved

bedevere-bot removed the awaiting review label Oct 18, 2022

bedevere-bot added the awaiting changes label Oct 18, 2022

twisteroidambassador added 4 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

Replace wait_for with wait.

513f7f9

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

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

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

arhadthedev added 2 commits April 26, 2023 19:50

Restore staggered.py from main branch

5ec7d65

Address patchcheck report

4e9f06a

arhadthedev requested a review from willingc as a code owner April 26, 2023 18:54

arhadthedev requested review from kumaraditya303 and gvanrossum April 26, 2023 19:27

Merge branch 'main' into happy_eyeballs_checkup

62387d6

gvanrossum added the needs backport to 3.12 bug and security fixes label May 23, 2023

gvanrossum reviewed May 24, 2023

View reviewed changes

kumaraditya303 removed their request for review July 8, 2023 08:55

serhiy-storchaka added the needs backport to 3.13 bugs and security fixes label May 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

twisteroidambassador commented Oct 18, 2022 •

edited by gvanrossum

Loading

gvanrossum left a comment

bedevere-bot commented Oct 18, 2022

twisteroidambassador commented Oct 18, 2022

gvanrossum commented Oct 18, 2022

twisteroidambassador commented Oct 19, 2022

kumaraditya303 left a comment

willingc commented Apr 26, 2023

arhadthedev commented Apr 26, 2023

arhadthedev commented Apr 26, 2023

gvanrossum left a comment

gvanrossum May 23, 2023

willingc May 25, 2023

gvanrossum May 24, 2023

twisteroidambassador commented Jun 21, 2023

	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]

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

Are you sure you want to change the base?

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

Conversation

twisteroidambassador commented Oct 18, 2022 • edited by gvanrossum Loading

gvanrossum left a comment

Choose a reason for hiding this comment

bedevere-bot commented Oct 18, 2022

twisteroidambassador commented Oct 18, 2022

gvanrossum commented Oct 18, 2022

twisteroidambassador commented Oct 19, 2022

kumaraditya303 left a comment

Choose a reason for hiding this comment

willingc commented Apr 26, 2023

arhadthedev commented Apr 26, 2023

arhadthedev commented Apr 26, 2023

gvanrossum left a comment

Choose a reason for hiding this comment

gvanrossum May 23, 2023

Choose a reason for hiding this comment

willingc May 25, 2023

Choose a reason for hiding this comment

gvanrossum May 24, 2023

Choose a reason for hiding this comment

twisteroidambassador commented Jun 21, 2023

twisteroidambassador commented Oct 18, 2022 •

edited by gvanrossum

Loading