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

[FR] Support listening to an ephemeral port 0 #617

Closed
webknjaz opened this issue Oct 1, 2021 · 13 comments · Fixed by #638
Closed

[FR] Support listening to an ephemeral port 0 #617

webknjaz opened this issue Oct 1, 2021 · 13 comments · Fixed by #638
Assignees

Comments

@webknjaz
Copy link
Contributor

webknjaz commented Oct 1, 2021

Is your feature request related to a problem? Please describe.

If I pass the port 0 to proxy.py when running it programmatically via the context manager Proxy, there's no way of accessing the actual port the kernel allocates.

Describe the solution you'd like

I want to run proxy.py with an ephemeral port so that each time it'd get a guaranteed unoccupied port from the OS kernel. This is useful in the context of testing where I don't want to have a hardcoded port to avoid conflicts / race conditions with other processes.

I've skimmed though the code and it seems easy to implement. If you green-light this feature, I may even send you a PR myself.

Describe alternatives you've considered

Giving up or monkey-patching stuff.

Additional context

I'm trying to use proxy.py in a test suite for a web framework.

@abhinavsingh
Copy link
Owner

IIRC, TestCase.PROXY_PORT will give you the port chosen by test case, if you choose to use proxy.py provided TestCase class. See https://github.com/abhinavsingh/proxy.py/blob/develop/proxy/testing/test_case.py and https://github.com/abhinavsingh/proxy.py#unit-testing-with-proxypy

In case you are directly using unittest.TestCase as described here https://github.com/abhinavsingh/proxy.py#with-unittesttestcase, then you can also use get_available_port to manually find a free random port. TestCase provided by proxy.py itself uses this method, see https://github.com/abhinavsingh/proxy.py/blob/develop/proxy/common/utils.py#L223

If none of that works out, I agree, we can even add this feature within core where passing --port=0 results in assignment of a random port. Happy to see a PR for it.

@webknjaz
Copy link
Contributor Author

webknjaz commented Oct 4, 2021

IIRC, TestCase.PROXY_PORT will give you the port chosen by test case

There are two problems with this. When it tries to find a free port, it may still end up being occupied because of a race condition before detecting it, closing the socket, and reopening it in the test. The chance is quite small but this is not an elegant solution.
Another problem is that it relies on unittest and we use pytest. Combining unittest.TestCase with pytest is a recipe for a bad DX so normally I try to get rid of unittest.

then you can also use get_available_port to manually find a free random port.

We also have a helper in our tests, that does exactly the same. But per my point above, I'd rather use a more elegant and robust solution.

If none of that works out, I agree, we can even add this feature within core where passing --port=0 results in assignment of a random port. Happy to see a PR for it.

I have a local patch that does that in like changing 2 lines (although, it probably makes sense to expose this info up the stack which would require adding a few more). Let me see if I can find time to come up with a PR.

P.S. For now, I've just patched the acceptor pool in tests: https://github.com/aio-libs/aiohttp/pull/6002/files#diff-be15bec475f6c781d5d3e4413c19d0e24ccaf616b8a2495c6824fc3a16d139f5R44.

@abhinavsingh
Copy link
Owner

@webknjaz Totally agreed, get_free_port approach can still lead to unexpected issues. Regarding pytest, I think proxy.py must also provide fixtures for it, so that pytest users can integrate with ease. If you have a PR handy, feel free to send across. We'll try to put this feature in develop branch asap.

@abhinavsingh
Copy link
Owner

@webknjaz #638 will allow access to the ephemeral port. See

proxy.py/proxy/proxy.py

Lines 452 to 455 in c1e38f9

with Proxy(input_args=input_args, **opts) as proxy:
assert proxy.pool is not None
logger.info('Listening on %s:%d' %
(proxy.pool.flags.hostname, proxy.pool.flags.port))
for usage example

@webknjaz
Copy link
Contributor Author

Sweet, I'll be able to replace https://github.com/aio-libs/aiohttp/blob/4fcfb8a/tests/test_proxy_functional.py#L60-L69 now :)

@webknjaz
Copy link
Contributor Author

@abhinavsingh oh, wait, I can't upgrade proxy.py just yet — it has a conflicting dependency. You add a very specific pin for typing-extensions which conflicts with our deps tree (namely typing-extensions==3.7.4.3).
Why is it even a runtime dep? And why is it pinned? It's an antipattern to have strict pins in the lib dependencies, the lockfiles are normally used to describe envs, not lib deps.

@abhinavsingh
Copy link
Owner

I think pinning automagically comes from dependabot, as it forces an update to newest version as and when one is available.

If we remove ==3.10.0.2 from https://github.com/abhinavsingh/proxy.py/blob/develop/requirements.txt , does it then work fine at your end? Can you probably try by clone, removing pin, install via local path and confirm. I can then push this through via a PR. Just want to confirm if this resolves the issue.

@abhinavsingh
Copy link
Owner

Btw, no dependencies were changed in my last PR. So I am thinking why did it run into a conflict now. IIUC, typing-extensions dependency goes back to several releases, since type checking was introduced within proxy.py.

Lemme know. This dep is just to satisfy type checker and as such plays no functional role. So happy to relax the requirements wherever possible.

@webknjaz
Copy link
Contributor Author

@abhinavsingh I can tell you right away that dropping the pin will make it possible for us to actually upgrade.

As a side note, with my PyPA hat on, I really recommend using https://github.com/jazzband/pip-tools1 to keep your CI/testing/dev envs reproducible but only put lower boundaries on the direct deps you add via install_requires. More info may be found at https://packaging.python.org/discussions/install-requires-vs-requirements/#install-requires-vs-requirements-files and https://hynek.me/articles/python-app-deps-2018/, for example.

Footnotes

  1. Dependabot supports pip-tools natively, for pairs of .in and .txt files with the same filename. Usage example in the project that powers PyPI itself: https://github.com/pypa/warehouse/tree/main/requirements.

@webknjaz
Copy link
Contributor Author

Btw, no dependencies were changed in my last PR. So I am thinking why did it run into a conflict now.

Oh, it didn't happen right now, I noticed it a month ago but forgot to file an issue. This is why we've pinned proxy.py==2.3.1 back then. Honestly, I don't see any use of this dependency in the code base, it looks like it shouldn't even be a runtime dependency but maybe just a test dep.

@abhinavsingh
Copy link
Owner

Makes sense. Let me take a look into it tomorrow sometime. I am almost in bed now (5:30am here). I thought I had it covered, alas :). Yes, there is no dependency on this library, but IIRC, we did see an exception during pip package installation if this dependency is missed. I'll recheck and update the package tomorrow. Have a good day.

@webknjaz
Copy link
Contributor Author

webknjaz commented Oct 30, 2021

@abhinavsingh #641 should fix this. This dependency is indeed used in one place in runtime but I've checked that any version of it available on PyPI has Protocol that is the only thing that's used meaning that the version bounds can be relaxed and even dropped in the metadata.

@abhinavsingh
Copy link
Owner

Thank you for digging into it :). Looks good, merged

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 a pull request may close this issue.

2 participants