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

Add context to find_unused_port #1837

Merged
merged 2 commits into from
Apr 1, 2020

Conversation

hodbn
Copy link
Member

@hodbn hodbn commented Mar 31, 2020

The helper function find_unused_port might return a port that will immediately be reused by the system.
I wrapped the helper in a contextmanager so the port is allocated to tempsock while inside the context.

This PR affects #1830, I will update it/this PR once one of them is accepted.

@codecov
Copy link

codecov bot commented Mar 31, 2020

Codecov Report

Merging #1837 into master will not change coverage by %.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##            master     #1837   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           22        22           
  Lines         2007      2007           
=========================================
  Hits          2007      2007           

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 c84d5ad...86d6cf4. Read the comment docs.

@pquentin
Copy link
Member

pquentin commented Apr 1, 2020

The function still says: The temporary socket is then closed and deleted, and the ephemeral port is returned.

Obviously the tests passed fine, but how does this work? Why is it not an issue to not close the socket before starting the test?

@hodbn
Copy link
Member Author

hodbn commented Apr 1, 2020

The yield is just after the bind_port call and before the close(), so the block inside the with is executed while the socket is still registered and the port is reserved by the OS. I've fixed the docs :)

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.

Great, thanks!

@pquentin pquentin merged commit a9508d8 into urllib3:master Apr 1, 2020
pquentin added a commit to pquentin/urllib3 that referenced this pull request Apr 7, 2020
pquentin added a commit to pquentin/urllib3 that referenced this pull request Apr 7, 2020
sethmlarson pushed a commit to sethmlarson/urllib3 that referenced this pull request Apr 11, 2020
Dobatymo pushed a commit to Dobatymo/urllib3 that referenced this pull request Mar 16, 2022
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

2 participants