Skip to content
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

Consume connections better in socket-level tests #1958

Merged
merged 4 commits into from Sep 18, 2020

Conversation

hodbn
Copy link
Member

@hodbn hodbn commented Sep 10, 2020

Add a utility that allows better timing of test responses, instead of socket consuming.

In Windows, after the second operation on a remotely closed socket a WSAECONNABORTED is raised.
In our socket-level tests, we implement a dummy HTTP server that only consumes data from the client socket, therefore it can't know when to stop consuming and might close the socket too early. This might cause unnecessary send/recvs on the already closed socket, which in turn will raise WSAECONNABORTED when it could be avoided.

I've added the 2 tests that are currently skipped on Windows (since #1951).
This change should prevent WSAECONNABORTED when testing the aforementioned tests on Windows.

@codecov
Copy link

codecov bot commented Sep 10, 2020

Codecov Report

Merging #1958 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##            master     #1958   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           24        24           
  Lines         2187      2187           
=========================================
  Hits          2187      2187           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0f11d38...edad878. Read the comment docs.

@hodbn hodbn changed the title Mark connections for better socket-level tests Consume connections better in socket-level tests Sep 10, 2020
@pquentin
Copy link
Member

@hodbn I merged master into your branch, since it was a prerequisite before merging this anyway

dummyserver/testcase.py Outdated Show resolved Hide resolved
@pquentin
Copy link
Member

This looks like black magic to me, but is great if it works! Would like to have another pair of eyes before I merge it

@hodbn
Copy link
Member Author

hodbn commented Sep 15, 2020

This looks like black magic to me, but is great if it works! Would like to have another pair of eyes before I merge it

I've added another sentence of docs, hopefully the class purpose is more clear now.

Copy link
Member

@pquentin pquentin left a comment

Choose a reason for hiding this comment

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

Let's try that! Thanks.

@pquentin pquentin merged commit d560e21 into urllib3:master Sep 18, 2020
@PleasantMachine9
Copy link
Contributor

I'm guessing this is the change that makes the CI flaky?
https://github.com/urllib3/urllib3/pull/1970/checks?check_run_id=1136185374

At least the MacOS tests failed 2/2 times, and first it was the 3.8, then the 3.6 test, and different ones, with the only common thing being that they were both socket-related (Connection Refused).

@hodbn
Copy link
Member Author

hodbn commented Sep 20, 2020

I'm guessing this is the change that makes the CI flaky?
https://github.com/urllib3/urllib3/pull/1970/checks?check_run_id=1136185374

I don't think these failures are related to this change.
This change only affects test_preserve_chunked_on_redirect and test_preserve_chunked_on_broken_connection for now.

Dobatymo pushed a commit to Dobatymo/urllib3 that referenced this pull request Mar 16, 2022
Co-authored-by: Quentin Pradet <quentin.pradet@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants