Add dynamic port binding#10697
Conversation
psf-chronographer
bot
added
the
bot:chronographer:provided
|
It was unclear if & where any tests should be added. When I attempted to build the docs locally, the reference to the new port attribute wasn't picked up properly, and I couldn't work out why. |
CodSpeed Performance ReportMerging #10697 will not alter performanceComparing Summary
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #10697 +/- ##
=======================================
Coverage 98.71% 98.71%
=======================================
Files 125 125
Lines 37445 37459 +14
Branches 2075 2076 +1
=======================================
+ Hits 36963 36977 +14
Misses 335 335
Partials 147 147
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Because it's not been added to the reference, so there's nothing to link to.
test_web_runner.py would seem like the obvious place. A test validating the port property works is probably what we want. |
|
Thanks, I had thought the reference docs were generated from the class docstrings. |
|
I have no clue why unrelated tests started failing / hanging after I added a test. If anyone understands this, please let me know and I'll fix it. The added test completes correctly locally, runinng all tests never completed for me locally, even when running in the master branch. Presumably because there's some part of the development setup I'm unaware of. |
Co-authored-by: J. Nick Koston <nick+github@koston.org>
webknjaz
left a comment
webknjaz
left a comment
There was a problem hiding this comment.
Some tests use a "free port" lookup helper. Now that this is implemented, it should be possible to update them too.
| if self._port == 0: | ||
| # Port 0 means bind to any port, so we need to set the attribute | ||
| # to the port the server was actually bound to. | ||
| self._port = self._server.sockets[0].getsockname()[1] |
There was a problem hiding this comment.
I wouldn't reassign this but track the bound port separately. This is so starting and stopping the TCP site multiple times would be able to get a new port allocated each time.
|
|
||
| await site.start() | ||
| assert site.port != 0 | ||
| assert site.name.startswith("http://0.0.0.0:") |
There was a problem hiding this comment.
Since we know the port, this could just compare the entire string..
|
|
||
| @property | ||
| def port(self) -> int: | ||
| """Return the port number the server is bound to, useful for the dynamically allocated port (0).""" |
There was a problem hiding this comment.
Please, avoid having the docstring title line exceed 72 chars. You can have a longer paragraph below.
Also, referencing a magic number is not useful to readers unfamiliar with it. Use the ephemeral terminology so people could at least google it. Although, I would probably not even mention it since the underlying implementation details aren't really important to the end-users who'd just use this unconditionally.
bdraco
left a comment
bdraco
left a comment
There was a problem hiding this comment.
Marking this request changes are there are pending review comments.
Dreamsorcerer
added
pr-unfinished
| return str(URL.build(scheme=scheme, host=host, port=self._port)) | ||
|
|
||
| @property | ||
| def port(self) -> int: |
There was a problem hiding this comment.
Let's also get examples/fake_server.py updated to use this new property.
|
@Dreamsorcerer @webknjaz @twhittock-disguise I'd like to pick this up and finish it (I need Is it ok if I continue from here? I'll address webknjaz's comments about not mutating |
github-actions
bot
removed
the
pr-unfinished
|
Yes I'm not working on it, please do! |
ok, nice. Thanks @twhittock-disguise |
|
@rodrigobnogueira if you're confident enough with Git, another (sometimes temporary) way of collaboration is creating a branch in your fork off of the OP's PR branch and the opening a PR in their fork against that branch. Then you can see diffs of what's yours vs the prior art but later could end up updating that original branch, once ready. This is more or less universal since PR authors don't always give us permission to update their branches and you many need another PR. But starting in your own fork is a good idea most of the time. |
@webknjaz Thank you for the detailed suggestion! I wasn't familiar with this approach. I'll do it. 👏 |
Finishes PR aio-libs#10697, replacing unused_port utility in examples and correctly tracking _bound_port against the resolved socket without mutating _port.
|
@rodrigobnogueira 😎 as you add multiple remotes ( |
Finishes PR aio-libs#10697, replacing unused_port utility in examples and correctly tracking _bound_port against the resolved socket without mutating _port.
Finishes PR aio-libs#10697, replacing unused_port utility in examples and correctly tracking _bound_port against the resolved socket without mutating _port.
Finishes PR aio-libs#10697, replacing unused_port utility in examples and correctly tracking _bound_port against the resolved socket without mutating _port.
5 participants
What do these changes do?
As per #10665 the TCPSite has a public
portproperty which is updated with the bound port number if it was dynamically allocatedAre there changes in behavior for the user?
Users are now able to rely on the port property and name property on TCPSite having the correct port number if they were relying on port
0dynamic allocation.Is it a substantial burden for the maintainers to support this?
Port 0 is a standard feature in all supported OSs, and leverages the OS capabilities, rather than any specific logic in aiohttp, so the burden should be small.
Related issue number
Fixes #10665
Checklist
CONTRIBUTORS.txtCHANGES/foldername it
<issue_or_pr_num>.<type>.rst(e.g.588.bugfix.rst)if you don't have an issue number, change it to the pull request
number after creating the PR
.bugfix: A bug fix for something the maintainers deemed animproper undesired behavior that got corrected to match
pre-agreed expectations.
.feature: A new behavior, public APIs. That sort of stuff..deprecation: A declaration of future API removals and breakingchanges in behavior.
.breaking: When something public is removed in a breaking way.Could be deprecated in an earlier release.
.doc: Notable updates to the documentation structure or buildprocess.
.packaging: Notes for downstreams about unobvious side effectsand tooling. Changes in the test invocation considerations and
runtime assumptions.
.contrib: Stuff that affects the contributor experience. e.g.Running tests, building the docs, setting up the development
environment.
.misc: Changes that are hard to assign to any of the abovecategories.
Make sure to use full sentences with correct case and punctuation,
for example:
Use the past tense or the present tense a non-imperative mood,
referring to what's changed compared to the last released version
of this project.