Call UpdateService for SRV & A/AAAA records as well as TXT #239

mattsaxon · 2020-04-06T20:02:12Z

Fixes #235

mattsaxon · 2020-04-06T20:07:16Z

There is a possibility that my fix here is a breaking change as UpdateService used to get called on service addition. I feel this is wrong and was a mistake.

coveralls · 2020-04-07T09:12:21Z

Coverage decreased (-1.2%) to 92.489% when pulling 4e6c84d on mattsaxon:master into f071f3d on jstasiak:master.

jstasiak · 2020-04-09T09:13:25Z

Two comments here so far (on top of that it's nice work you're doing):

there's some cleanup to do in order to make the code less repetitive, I have an idea how to do this and will let you know
I see that PyPy is a major obstacle now – I'll try to wrap my head around why it is so, maybe I'll be able to help

mattsaxon · 2020-04-09T09:21:00Z

Yep the pypy stuff seems to pass sometimes, fail others. I’m gonna install it on my personal machine as a next step so I can look at this more closely. I had initially suspected timing issues, but not so sure now. Any help you can give is appreciated.

jstasiak · 2020-04-09T11:02:59Z

If you rebase your branch onto the master branch to get the benefit of #240 we should see some relevant details in the CI logs.

Switch to pytest for test running (python-zeroconf#240)

This reverts commit 902548b.

This reverts commit d8f3508.

This reverts commit dc570c9.

This reverts commit c1d5625.

This reverts commit aa3e947.

This reverts commit cc78868.

mattsaxon · 2020-04-10T16:41:32Z

Right worked it out! Interestingly the pypy stuff did uncover a couple of issues in the code and the test suite

jstasiak · 2020-04-13T08:43:22Z

Sure, I'll have a look later today. I think it'll be fine once things are squashed.

Use tuple rather than List for handler specification

mattsaxon · 2020-04-13T09:15:48Z

So I've cleaned up all the various linting errors, a lot of the changes you've seen above are to tidy up the errors reporting in my UI (vscode). However the move to pytest has thrown up a lot warnings. Some I can understand, which are the deprecation warning. However there are only 3 of those and there are a further 17 other warnings reported, but I can't see the actual warnings even using verbose mode (pytest -v), any ideas?

jstasiak

Regarding warnings – I think I have an idea how pytest counts those. From https://travis-ci.org/github/jstasiak/python-zeroconf/jobs/674322037, we see three warnings because pytest shows all instances of a specific warning together, so first one:

=============================== warnings summary ===============================

zeroconf/test.py: 11 tests with warnings

  /home/travis/build/jstasiak/python-zeroconf/zeroconf/__init__.py:1605: DeprecationWarning: address is deprecated, use addresses instead

    warnings.warn("address is deprecated, use addresses instead", DeprecationWarning)

That's 11, then we have 4 more:

zeroconf/test.py::test_multiple_addresses

zeroconf/test.py::test_multiple_addresses

zeroconf/test.py::test_multiple_addresses

zeroconf/test.py::test_multiple_addresses

  /home/travis/build/jstasiak/python-zeroconf/zeroconf/__init__.py:1633: DeprecationWarning: ServiceInfo.address is deprecated, use addresses instead

    warnings.warn("ServiceInfo.address is deprecated, use addresses instead", DeprecationWarning)

That's 15 total, then two more:

zeroconf/test.py::test_multiple_addresses

zeroconf/test.py::test_multiple_addresses

  /home/travis/build/jstasiak/python-zeroconf/zeroconf/__init__.py:1642: DeprecationWarning: ServiceInfo.address is deprecated, use addresses instead

    warnings.warn("ServiceInfo.address is deprecated, use addresses instead", DeprecationWarning)

-- Docs: https://docs.pytest.org/en/latest/warnings.html

And this gives us 17. So I think problem solved here (as far as understanding where do those come from is concerned, anyway). :)

All in all very nice work.

.gitignore

zeroconf/__init__.py

jstasiak · 2020-04-14T00:04:29Z

zeroconf/__init__.py

-                    zeroconf=zeroconf, service_type=self.type, name=name, state_change=state_change
-                )
-            )
+


OK, two things here.

Regarding the logic, I had to write myself a little table to make sure I understand things correctly. I added some notes in two cases that I believe to be worth taking a second look at. The table:

Existing enqueued state change New state change to enqueue State change on the list afterwards My notes

Added Added

Updated Updated

Removed Removed

Added Added Added

Added Updated Added

Added Removed Added Shouldn't this be just Removed, since the service is gone? When we enqueue Removed then there's Updated on the list already we switch to Removed.

Updated Added Added

Updated Updated Updated

Updated Removed Removed

Removed Added Added

Removed Updated Removed Now I'm not sure when would this happen, why wouldn't it make sense to result in Updated here?

Removed Removed Removed

Regarding the implementation details, I think _handlers_to_call could work better as an OrderedDict[str, ServiceStateChange] instead of List[Tuple[str,ServiceStateChange]]. The code below could then look more or less like (keeping the logic intact, I let myself reverse the logic and remove the early return(s), as it it wouldn't really make sense in this case):

if ( (state_change is ServiceStateChange.Updated and name not in self._handlers_to_call) or ( state_change is ServiceStateChange.Removed and self._handlers_to_call.get(name) is ServiceStateChange.Updated ) or state_change is ServiceStateChange.Added ): self._handlers_to_call[name] = state_change

This removes all that count(), remove() and append() dance which is nice I think. Then the popping code would use OrderedDict.popitem() instead of list.pop().

Thinking about it, the code could just be an unconditional self._handlers_to_call[name] = state_change. :)

I’ll have a bit more of a think about this one the coming week.

Your last suggestion won't work because then an update trumps an add. I've used a slightly amended version of your first suggestion and the code you had above didn't update removed if for example the list was empty.

Regarding your 2 queries about the truth table, yes there are some combinations that don't make sense and in a well written responded shouldn't happen. However I've taken the view that I need to make a decision on the precedence of the messages and my view is that Added always wins, so for example if we receive an add and remove message in the same batch, then add is the most likely meaning. I can hypothesise a reason this might occur if you have multiple res ponders and one goes offline. In this case Added + Removed still means there is a service there to respond. (note this is a contrived example as the messages would come in a different batch)

Fair enough – if it causes issues I expect it'll be reported and we'll sort it out then, having a concrete case to deal with.

jstasiak

Sorry, closed by accident. LGTM, I'll merge this now.

Fix #235 Contains: * Add lock around handlers list * Reverse DNSCache order to ensure newest records take precedence When there are multiple records in the cache, the behaviour was inconsistent. Whilst the DNSCache.get() method returned the newest, any function which iterated over the entire cache suffered from a last write winds issue. This change makes this behaviour consistent and allows the removal of an (incorrect) wait from one of the unit tests.

jstasiak · 2020-04-25T16:47:28Z

Just merged manually (to get rid of merge errors, squash and produce nice commit message) as 552a030. Nice job! I'll release this later today.

jstasiak · 2020-04-26T01:02:34Z

Shipped in 0.26.0.

Those are results of a bad conflict resolution I did when merging [1]. [1] 552a030 ("Call UpdateService on SRV & A/AAAA updates as well as TXT (#239)")

Closes #255 Background: #239 adds the lock _handlers_lock: python-zeroconf/zeroconf/__init__.py self._handlers_lock = threading.Lock() # ensure we process a full message in one go Which is used in the engine thread: def handle_response(self, msg: DNSIncoming) -> None: """Deal with incoming response packets. All answers are held in the cache, and listeners are notified.""" with self._handlers_lock: And also by the service browser when issuing the state change callbacks: if len(self._handlers_to_call) > 0 and not self.zc.done: with self.zc._handlers_lock: handler = self._handlers_to_call.popitem(False) self._service_state_changed.fire( zeroconf=self.zc, service_type=self.type, name=handler[0], state_change=handler[1] ) Both pychromecast and Home Assistant calls Zeroconf.get_service_info from the service callbacks which means the lock may be held for several seconds which will starve the engine thread.

See python-zeroconf/python-zeroconf#239

mattsaxon added 2 commits April 5, 2020 20:03

Interim Commit

43e1b24

Call UpdateService on SRV & A/AAAA updates as well as TXT

d314896

Fix python-zeroconf#235

Tidy up tests and fix mypy issue

f40649a

mattsaxon added 3 commits April 7, 2020 19:16

Black reformat

6a3deb7

adds waits to deal with possible pypy issues

4a1f7e5

Nasty fixes

2b258fd

mattsaxon added 18 commits April 10, 2020 12:20

Merge pull request #1 from jstasiak/master

064056b

Switch to pytest for test running (python-zeroconf#240)

Update test.py

cc78868

Update test.py

aa3e947

update tests

c1d5625

Update test.py

dc570c9

Add lock around handlers list

d8f3508

more locks and waits

902548b

Revert "more locks and waits"

f3d6bf2

This reverts commit 902548b.

Revert "Add lock around handlers list"

53cd650

This reverts commit d8f3508.

Revert "Update test.py"

700a183

This reverts commit dc570c9.

Revert "update tests"

d2cfb86

This reverts commit c1d5625.

Revert "Update test.py"

23aa41f

This reverts commit aa3e947.

Revert "Update test.py"

2c16b31

This reverts commit cc78868.

return to a reasonable test place!

98bdbfd

Update test.py

a3ab378

Update test.py

a42ee2c

Add lock

5d69f91

Tidy up code and add some comments

c2d4b0c

mattsaxon added 5 commits April 12, 2020 19:06

linting

592c0e1

linting

982f75b

linting

4b48dd9

linting

56955ed

linting

abc4773

Strengthen type checking of handlers_to_call

73ae509

Use tuple rather than List for handler specification

jstasiak reviewed Apr 14, 2020

View reviewed changes

mattsaxon and others added 7 commits April 19, 2020 19:11

code review comments (1)

f142584

code review comments (2)

aad875d

code review comments (3)

7430ca0

linting

9fe89c6

linting

55a0026

Delete launch.json

4b2c476

Delete settings.json

4e6c84d

jstasiak self-requested a review April 25, 2020 16:21

jstasiak closed this Apr 25, 2020

jstasiak reopened this Apr 25, 2020

jstasiak approved these changes Apr 25, 2020

View reviewed changes

jstasiak closed this Apr 25, 2020

jstasiak added a commit that referenced this pull request May 6, 2020

Remove unwanted pylint directives

0dd6fe4

Those are results of a bad conflict resolution I did when merging [1]. [1] 552a030 ("Call UpdateService on SRV & A/AAAA updates as well as TXT (#239)")

This was referenced May 22, 2020

Chromecast and Home-mini not detected after HA restart home-assistant/core#35922

Closed

Zeroconf.get_service_info seems broken in 0.26.0 #255

Closed

Don't call callbacks whend holding _handlers_lock #258

Merged

ggilestro added a commit to gilestrolab/ethoscope that referenced this pull request Nov 23, 2020

Adds an update_service routine for zeroconf future compatibility

8e13e47

See python-zeroconf/python-zeroconf#239

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Call UpdateService for SRV & A/AAAA records as well as TXT #239

Call UpdateService for SRV & A/AAAA records as well as TXT #239

mattsaxon commented Apr 6, 2020 •

edited

mattsaxon commented Apr 6, 2020

coveralls commented Apr 7, 2020 •

edited

jstasiak commented Apr 9, 2020

mattsaxon commented Apr 9, 2020

jstasiak commented Apr 9, 2020

mattsaxon commented Apr 10, 2020

jstasiak commented Apr 13, 2020

mattsaxon commented Apr 13, 2020

jstasiak left a comment

jstasiak Apr 14, 2020

jstasiak Apr 15, 2020

mattsaxon Apr 19, 2020

mattsaxon Apr 25, 2020

jstasiak Apr 25, 2020

jstasiak left a comment

jstasiak commented Apr 25, 2020

jstasiak commented Apr 26, 2020

Existing enqueued state change	New state change to enqueue	State change on the list afterwards	My notes
	Added	Added
	Updated	Updated
	Removed	Removed
Added	Added	Added
Added	Updated	Added
Added	Removed	Added	Shouldn't this be just Removed, since the service is gone? When we enqueue Removed then there's Updated on the list already we switch to Removed.
Updated	Added	Added
Updated	Updated	Updated
Updated	Removed	Removed
Removed	Added	Added
Removed	Updated	Removed	Now I'm not sure when would this happen, why wouldn't it make sense to result in Updated here?
Removed	Removed	Removed

Call UpdateService for SRV & A/AAAA records as well as TXT #239

Call UpdateService for SRV & A/AAAA records as well as TXT #239

Conversation

mattsaxon commented Apr 6, 2020 • edited

mattsaxon commented Apr 6, 2020

coveralls commented Apr 7, 2020 • edited

jstasiak commented Apr 9, 2020

mattsaxon commented Apr 9, 2020

jstasiak commented Apr 9, 2020

mattsaxon commented Apr 10, 2020

jstasiak commented Apr 13, 2020

mattsaxon commented Apr 13, 2020

jstasiak left a comment

Choose a reason for hiding this comment

jstasiak Apr 14, 2020

Choose a reason for hiding this comment

jstasiak Apr 15, 2020

Choose a reason for hiding this comment

mattsaxon Apr 19, 2020

Choose a reason for hiding this comment

mattsaxon Apr 25, 2020

Choose a reason for hiding this comment

jstasiak Apr 25, 2020

Choose a reason for hiding this comment

jstasiak left a comment

Choose a reason for hiding this comment

jstasiak commented Apr 25, 2020

jstasiak commented Apr 26, 2020

mattsaxon commented Apr 6, 2020 •

edited

coveralls commented Apr 7, 2020 •

edited