-
-
Notifications
You must be signed in to change notification settings - Fork 30k
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
bpo-36123:race condition in test_socket #12053
Conversation
@@ -0,0 +1,2 @@ | |||
timeout is now set once the socket is created in :_testWithTimeoutTriggeredSend(self) rather than on the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In your NEWS entry, it's unclear why you need this change. I prefer to explicit say that you fix a race condition, and mention the test name from the start. Moreover, "_testWithTimeoutTriggeredSend" is the private method, whereas testWithTimeoutTriggeredSend() is the public test name.
Fix test_socket.testWithTimeoutTriggeredSend() race condition: timeout is now set once the socket is created in rather than on the connection.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. This have sense, but I don't know how to test it.
I'm able to reproduce the race condition using:
I'm not sure that the PR is enough. I suggest to also use a longer timeout on accept(). @nanjekyejoannah: can you please also include the following change in your PR?
MAIN_TIMEOUT is 60 seconds, whereas TIMEOUT is "only" 2 seconds. The FreeBSD buildbot is really slow, so a longer timeout can help. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Thanks @nanjekyejoannah for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7. |
…ion (pythonGH-12053) Use longer timeout for accept() in the server and block on accept in the client. The client now only sets the timeout once the socket is connected. (cherry picked from commit 53b9e1a) Co-authored-by: Joannah Nanjekye <33177550+nanjekyejoannah@users.noreply.github.com>
GH-12056 is a backport of this pull request to the 3.7 branch. |
I have set timeout once the socket is created rather than setting timeout on the
connection.
cc @vstinner
https://bugs.python.org/issue36123