chore: Rebase avahi_name_conflict commits onto edge
#10706
Conversation
SyntaxColoring
added
chore
update server
robot-svcs
Codecov Report
@@ Coverage Diff @@
## edge #10706 +/- ##
========================================
Coverage 73.81% 73.81%
========================================
Files 2139 2144 +5
Lines 57526 57629 +103
Branches 5807 5807
========================================
+ Hits 42460 42541 +81
- Misses 13846 13868 +22
Partials 1220 1220
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
The truncation stuff I think can have some slightly surprising outcomes. A better way to do this might be to limit the length of the names you can set low enough to give enough space for, say, 4 or 5 or 6 digits of service name deconfliction, and wrap the sequence numbers at that limit. Then there's no dynamic truncation stuff required. It also solves the problem with losing the original basename so that you might end up with just "Num0" as your name after a while.
| (If we care to fix this, we should look into preserving grapheme clusters: | ||
| https://unicode.org/reports/tr29/) | ||
| """ | ||
| encoded = s.encode("utf-8")[:maximum_utf8_octets] |
There was a problem hiding this comment.
I think this won't be as much of an issue because our name acceptability regex may prevent characters like this, depending on how the engine is implemented. it really is pretty gross though, and I think there are better ways to do it - see top level review
There was a problem hiding this comment.
The only name acceptability regex that I know of today is client-side. Is that the one that you mean?
If so, yes, it'll prevent non-ASCII. But we intend to loosen that eventually. I figured we may as well try to handle things nicely now in case a client that's looser with names ever talks to this server version.
Agreed with your top-level review about this being gross and there being better ways.
|
|
||
| # "3" comes from AVAHI_ENTRY_GROUP_COLLISION being index 3 in this enum: | ||
| # https://github.com/lathiat/avahi/blob/v0.8/avahi-common/defs.h#L234 | ||
| avahi_entry_group_collision = dbus.Int32(3) |
There was a problem hiding this comment.
[nit] love the comment with the source, but maybe also make this a class constant so you can see it from the code alone?
There was a problem hiding this comment.
Yeah, @X-sam suggested this in #10559 (comment) too. Two people independently suggesting this probably means it's a good idea, so I will.
There was a problem hiding this comment.
Oops, it turns out that this is hard because dev machines don't have the dbus module.
This syntax:
class _SyncClient:
_AVAHI_ENTRY_GROUP_COLLISION = dbus.Int32(3)
...Will evaluate dbus.Int32(3) at class definition time, and run into a NameError because dbus isn't defined.
I don't see an easy way of resolving this without doing something like guarding the whole class definition, which, anecdotally, I've seen mypy have trouble with.
Leaving this alone for now.
| # It seems like there should be a better way of doing this, | ||
| # but this is what the docs recommend. | ||
| # https://docs.aiohttp.org/en/stable/web_advanced.html#application-runners | ||
| await asyncio.sleep(3600) |
There was a problem hiding this comment.
can we make this like await asyncio.sleep(10)? i get why you would want to do this, but either
- it doesn't matter how long you sleep as long as it's "a while" (and ten seconds counts)
- something weird happens when this task wakes and it'll be easier to notice if it's every ten seconds instead of every hour
even better honestly would be establishing some dummy quitflag, setting it, and then await on it dropping. will never wake accidentally and provides a pretty clear path to adding graceful quitting.
There was a problem hiding this comment.
Good thoughts.
Going with await asyncio.sleep(10) for now because it's a more minimal change and it keeps us mirroring the aiohttp docs. But the dummy quitflag is a clever solution, I think I agree that it's better, and I'll make a note to do that in another PR.
There was a problem hiding this comment.
if you're gonna make a note for that - maybe note to go ahead and implement actual quitting by handling sigterm (default for systemd process killing, could change to basically anything else if you wanted to in the unit file) and raising the flag,
Yeah, that definitely would have been better. I think I'd like to stick with the logic as written for now, because:
|
There was a problem hiding this comment.
Based on discussion here, sounds good to me!
Will say though, I'm not an expert, but if splitting grapheme clusters is a concern I don't know how much I want to leave that to avahi
Addresses review comment: #10706 (comment)
Overview
This PR is to rebase two commits atop
edge:name_managementinto submodules #10523.sleep(3600)tosleep(10). See discussion below in this PR.Fixes bug #10126.
Review requests
edge(as opposed to an alpha or beta branch or something).Risk analysis
This should be safe at this point because:
update-serverin the meantime has been code linting changes, style(python): remove line length limit from flake8 config #10695.