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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure all listeners are cleaned up on ServiceBrowser cancelation #290

Merged
merged 2 commits into from Aug 17, 2020

Conversation

bdraco
Copy link
Member

@bdraco bdraco commented Aug 14, 2020

Sorry I missed this in the original submission #249. 馃檴

When creating listeners for a ServiceBrowser with multiple types
they would not all be removed on cancelation. This led
to a build up of stale listeners when ServiceBrowsers were
frequently added and removed.

@coveralls
Copy link

coveralls commented Aug 14, 2020

Coverage Status

Coverage increased (+0.2%) to 92.896% when pulling 1158673 on bdraco:fix_listener_leak into 19e33a6 on jstasiak:master.

@bdraco bdraco force-pushed the fix_listener_leak branch 4 times, most recently from e3772ec to 8569101 Compare August 15, 2020 00:00
@bdraco bdraco marked this pull request as ready for review August 15, 2020 00:24
@bdraco
Copy link
Member Author

bdraco commented Aug 15, 2020

The coverage change is due to the listener actually being removed. I don't think it was actually covered with any assertions before.

I added coverage for the areas that were previously not actually covered with assertions.

@bdraco
Copy link
Member Author

bdraco commented Aug 15, 2020

May be one of the causes of home-assistant/core#38862

@bdraco bdraco force-pushed the fix_listener_leak branch 4 times, most recently from dbc568a to 7251dc7 Compare August 15, 2020 14:05
When creating listeners for a ServiceBrowser with multiple types
they would not all be removed on cancelation. This led
to a build up of stale listeners when ServiceBrowsers were
frequently added and removed.
@bdraco
Copy link
Member Author

bdraco commented Aug 17, 2020

@jstasiak If you have a moment, could we get a release bump with this fix. I'm trying to close out home-assistant/core#38862 and home-assistant/core#38933 and this is the one of the last ones to be resolved.

zeroconf/test.py Outdated
@@ -860,6 +861,20 @@ def test_cache_empty_does_not_leak_memory_by_leaving_empty_list(self):
cache.remove(record2)
assert 'a' not in cache.cache

def test_cache_empty_multiple_calls_does_not_throw(self):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this patch actually fix something that's verified by this test? Let's keep this out of this pull request if that's not the case.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll pull it out. It should probably go in a new PR after this merges

@jstasiak
Copy link
Collaborator

Sure, I'll make a release after merging.

@jstasiak jstasiak merged commit c9f3c91 into python-zeroconf:master Aug 17, 2020
@jstasiak
Copy link
Collaborator

Just released as 0.28.1. :)

@bdraco
Copy link
Member Author

bdraco commented Aug 17, 2020

TYVM

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

3 participants