Skip to content

bpo-35998: Fix test_asyncio.test_start_tls_server_1() #16815

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 2 commits into from
Oct 16, 2019
Merged

bpo-35998: Fix test_asyncio.test_start_tls_server_1() #16815

merged 2 commits into from
Oct 16, 2019

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Oct 15, 2019 •
edited by bedevere-bot
Loading

main() is now responsible to send the ANSWER, rather than
ServerProto. main() now waits until it got the HELLO before sending
the ANSWER over the new transport.

Previously, there was a race condition between main() replacing the
protocol and the protocol sending the ANSWER once it gets the HELLO.

TLSv1.3 was disabled for the test: reenable it.

https://bugs.python.org/issue35998

@vstinner
bpo-35998: Fix test_asyncio.test_start_tls_server_1()
f181ba7 
main() is now responsible to send the ANSWER, rather than
ServerProto. main() now waits until it got the HELLO before sending
the ANSWER over the new transport.

Previously, there was a race condition between main() replacing the
protocol and the protocol sending the ANSWER once it gets the HELLO.

TLSv1.3 was disabled for the test: reenable it.
@vstinner vstinner requested a review from tiran as a code owner October 15, 2019 23:37
@bedevere-bot bedevere-bot added tests Tests in the Lib/test dir awaiting core review labels Oct 15, 2019
@vstinner
Copy link
Member Author

I tested manually this fix on my laptop using ./python -u -m test test_asyncio -m test.test_asyncio.test_sslproto.SelectorStartTLSTests.test_start_tls_server_1 -F -j100 command. Without my fix, the command fails in less than 100 attempts in less than 30 seconds. With the fix, the test runs successfuly more than 1000 times for longer than 3 minutes:

...
0:03:53 load avg: 99.51 [1315] test_asyncio passed
0:03:53 load avg: 99.51 [1316] test_asyncio passed
0:03:53 load avg: 99.51 [1317] test_asyncio passed
0:03:53 load avg: 99.51 [1318] test_asyncio passed

I did a similar check on Gentoo's buildbot worker using using ./python -u -m test test_asyncio -m test.test_asyncio.test_sslproto.SelectorStartTLSTests.test_start_tls_server_1 -F -j2 command. Without the fix, it fails in less than 3 attempts in a few seconds. With the fix, the test runs successfully more than 250 tests for longer than 5 minutes:

...
0:05:13 load avg: 11.74 [278] test_asyncio passed
0:05:13 load avg: 11.74 [279] test_asyncio passed
0:05:13 load avg: 11.74 [280] test_asyncio passed

pablogsal
pablogsal approved these changes Oct 15, 2019
View reviewed changes
Copy link
Member

@pablogsal pablogsal left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this tricky bug, @vstinner.

Great work!

@vstinner vstinner merged commit fab4ef2 into python:master Oct 16, 2019
@vstinner vstinner deleted the fix_test_start_tls_server_1 branch October 16, 2019 00:36
@vstinner
Copy link
Member Author

This change needs to be backported to 3.7 and 3.8, but I prefer to wait to see if buildbots like this new change, before doing the backport.

@miss-islington
Copy link
Contributor

Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 16, 2019
@vstinner @miss-islington
bpo-35998: Fix test_asyncio.test_start_tls_server_1() (pythonGH-16815)
34b782c 
main() is now responsible to send the ANSWER, rather than
ServerProto. main() now waits until it got the HELLO before sending
the ANSWER over the new transport.

Previously, there was a race condition between main() replacing the
protocol and the protocol sending the ANSWER once it gets the HELLO.

TLSv1.3 was disabled for the test: reenable it.
(cherry picked from commit fab4ef2)

Co-authored-by: Victor Stinner <vstinner@python.org>
@bedevere-bot
Copy link

GH-16817 is a backport of this pull request to the 3.8 branch.

miss-islington added a commit that referenced this pull request Oct 16, 2019
@miss-islington @vstinner
bpo-35998: Fix test_asyncio.test_start_tls_server_1() (GH-16815)
afbcd9f 
main() is now responsible to send the ANSWER, rather than
ServerProto. main() now waits until it got the HELLO before sending
the ANSWER over the new transport.

Previously, there was a race condition between main() replacing the
protocol and the protocol sending the ANSWER once it gets the HELLO.

TLSv1.3 was disabled for the test: reenable it.
(cherry picked from commit fab4ef2)

Co-authored-by: Victor Stinner <vstinner@python.org>
vstinner added a commit that referenced this pull request Oct 16, 2019
@vstinner
bpo-35998: Fix test_asyncio.test_start_tls_server_1() (GH-16815) (GH-…
1bceb0e 
…16818)

main() is now responsible to send the ANSWER, rather than
ServerProto. main() now waits until it got the HELLO before sending
the ANSWER over the new transport.

Previously, there was a race condition between main() replacing the
protocol and the protocol sending the ANSWER once it gets the HELLO.

TLSv1.3 was disabled for the test: reenable it.

(cherry picked from commit fab4ef2)
jacobneiltaylor pushed a commit to jacobneiltaylor/cpython that referenced this pull request Dec 5, 2019
@vstinner
bpo-35998: Fix test_asyncio.test_start_tls_server_1() (pythonGH-16815)
c2a7bf8 
main() is now responsible to send the ANSWER, rather than
ServerProto. main() now waits until it got the HELLO before sending
the ANSWER over the new transport.

Previously, there was a race condition between main() replacing the
protocol and the protocol sending the ANSWER once it gets the HELLO.

TLSv1.3 was disabled for the test: reenable it.
shihai1991 pushed a commit to shihai1991/cpython that referenced this pull request Jan 31, 2020
@vstinner @shihai1991
bpo-35998: Fix test_asyncio.test_start_tls_server_1() (pythonGH-16815)
8269df1 
main() is now responsible to send the ANSWER, rather than
ServerProto. main() now waits until it got the HELLO before sending
the ANSWER over the new transport.

Previously, there was a race condition between main() replacing the
protocol and the protocol sending the ANSWER once it gets the HELLO.

TLSv1.3 was disabled for the test: reenable it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pablogsal pablogsal pablogsal approved these changes

@tiran tiran Awaiting requested review from tiran

Assignees
No one assigned
Labels
tests Tests in the Lib/test dir
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@vstinner @miss-islington @bedevere-bot @pablogsal @the-knights-who-say-ni