Skip to content

Make add_listener and remove_listener threadsafe #794

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.

Sign up for GitHub

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

Merged
bdraco merged 1 commit into from
Jun 21, 2021
Merged

Make add_listener and remove_listener threadsafe #794

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions tests/services/test_browser.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,6 +315,7 @@ def mock_incoming_msg(service_state_change: r.ServiceStateChange) -> r.DNSIncomi
finally:
assert len(zeroconf.listeners) == 1
service_browser.cancel()
time.sleep(0.2)
assert len(zeroconf.listeners) == 0
zeroconf.remove_all_service_listeners()
zeroconf.close()
Expand Down Expand Up @@ -422,6 +423,7 @@ def _mock_get_expiration_time(self, percent):
finally:
assert len(zeroconf.listeners) == 1
service_browser.cancel()
time.sleep(0.2)
assert len(zeroconf.listeners) == 0
zeroconf.remove_all_service_listeners()
zeroconf.close()
Expand Down
34 changes: 30 additions & 4 deletions zeroconf/_core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -548,12 +548,38 @@ def add_listener(
) -> None:
"""Adds a listener for a given question. The listener will have
its update_record method called when information is available to
answer the question(s)."""
self.record_manager.add_listener(listener, question)
answer the question(s).

This function is threadsafe
"""
assert self.loop is not None
self.loop.call_soon_threadsafe(self.record_manager.async_add_listener, listener, question)

def remove_listener(self, listener: RecordUpdateListener) -> None:
"""Removes a listener."""
self.record_manager.remove_listener(listener)
"""Removes a listener.

This function is threadsafe
"""
assert self.loop is not None
self.loop.call_soon_threadsafe(self.record_manager.async_remove_listener, listener)

def async_add_listener(
self, listener: RecordUpdateListener, question: Optional[Union[DNSQuestion, List[DNSQuestion]]]
) -> None:
"""Adds a listener for a given question. The listener will have
its update_record method called when information is available to
answer the question(s).

This function is not threadsafe and must be called in the eventloop.
"""
self.record_manager.async_add_listener(listener, question)

def async_remove_listener(self, listener: RecordUpdateListener) -> None:
"""Removes a listener.

This function is not threadsafe and must be called in the eventloop.
"""
self.record_manager.async_remove_listener(listener)

def handle_response(self, msg: DNSIncoming) -> None:
"""Deal with incoming response packets. All answers
Expand Down
18 changes: 12 additions & 6 deletions zeroconf/_handlers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -387,20 +387,23 @@ def _async_mark_unique_cached_records_older_than_1s_to_expire(
# Expire in 1s
entry.set_created_ttl(now, 1)

def add_listener(
def async_add_listener(
self, listener: RecordUpdateListener, question: Optional[Union[DNSQuestion, List[DNSQuestion]]]
) -> None:
"""Adds a listener for a given question. The listener will have
its update_record method called when information is available to
answer the question(s)."""
answer the question(s).

This function is not threadsafe and must be called in the eventloop.
"""
self.listeners.append(listener)

if question is None:
return

questions = [question] if isinstance(question, DNSQuestion) else question
assert self.zc.loop is not None
self.zc.loop.call_soon_threadsafe(self._async_update_matching_records, listener, questions)
self._async_update_matching_records(listener, questions)

def _async_update_matching_records(
self, listener: RecordUpdateListener, questions: List[DNSQuestion]
Expand All @@ -422,10 +425,13 @@ def _async_update_matching_records(
listener.async_update_records_complete()
self.zc.async_notify_all()

def remove_listener(self, listener: RecordUpdateListener) -> None:
"""Removes a listener."""
def async_remove_listener(self, listener: RecordUpdateListener) -> None:
"""Removes a listener.

This function is not threadsafe and must be called in the eventloop.
"""
try:
self.listeners.remove(listener)
self.zc.notify_all()
self.zc.async_notify_all()
except ValueError as e:
log.exception('Failed to remove listener: %r', e)
24 changes: 10 additions & 14 deletions zeroconf/_services/browser.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,14 +242,16 @@ def __init__(
for h in handlers:
self.service_state_changed.register_handler(h)

def _setup(self) -> None:
def _async_start(self) -> None:
"""Generate the next time and setup listeners.

Must be called by uses of this base class after they
have finished setting their properties.
"""
self._generate_first_next_time()
self.zc.add_listener(self, [DNSQuestion(type_, _TYPE_PTR, _CLASS_IN) for type_ in self.types])
self.zc.async_add_listener(self, [DNSQuestion(type_, _TYPE_PTR, _CLASS_IN) for type_ in self.types])
# Only start queries after the listener is installed
self._browser_task = cast(asyncio.Task, asyncio.ensure_future(self.async_browser_task()))

def _generate_first_next_time(self) -> None:
"""Generate the initial next query times.
Expand Down Expand Up @@ -374,10 +376,10 @@ def _fire_service_state_changed_event(self, event: Tuple[Tuple[str, str], Servic
state_change=state_change,
)

def cancel(self) -> None:
def _async_cancel(self) -> None:
"""Cancel the browser."""
self.done = True
self.zc.remove_listener(self)
self.zc.async_remove_listener(self)

def generate_ready_queries(self) -> List[DNSOutgoing]:
"""Generate the service browser query for any type that is due."""
Expand Down Expand Up @@ -454,20 +456,15 @@ def __init__(
self.queue = get_best_available_queue()
self.daemon = True
self.start()
self._setup()
# Start queries after the listener is installed in _setup
zc.loop.call_soon_threadsafe(self._async_start_browser)
zc.loop.call_soon_threadsafe(self._async_start)
self.name = "zeroconf-ServiceBrowser-%s-%s" % (
'-'.join([type_[:-7] for type_ in self.types]),
getattr(self, 'native_id', self.ident),
)

def _async_start_browser(self) -> None:
"""Start the browser from the event loop."""
self._browser_task = cast(asyncio.Task, asyncio.ensure_future(self.async_browser_task()))

def _async_cancel_browser_soon(self) -> None:
def _async_cancel_soon(self) -> None:
"""Cancel the browser from the event loop."""
self._async_cancel()
if self._browser_task:
asyncio.ensure_future(self._async_cancel_browser())

Expand All @@ -476,8 +473,7 @@ def cancel(self) -> None:
assert self.zc.loop is not None
assert self.queue is not None
self.queue.put(None)
self.zc.loop.call_soon_threadsafe(self._async_cancel_browser_soon)
super().cancel()
self.zc.loop.call_soon_threadsafe(self._async_cancel_soon)
self.join()

def run(self) -> None:
Expand Down
4 changes: 2 additions & 2 deletions zeroconf/_services/info.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -421,7 +421,7 @@ async def async_request(
last = now + timeout
await zc.async_wait_for_start()
try:
zc.add_listener(self, None)
zc.async_add_listener(self, None)
while not self._is_complete:
if last <= now:
return False
Expand All @@ -436,7 +436,7 @@ async def async_request(
await zc.async_wait(min(next_, last) - now)
now = current_time_millis()
finally:
zc.remove_listener(self)
zc.async_remove_listener(self)

return True

Expand Down
8 changes: 3 additions & 5 deletions zeroconf/aio.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
import asyncio
import contextlib
from types import TracebackType # noqa # used in type hints
from typing import Awaitable, Callable, Dict, List, Optional, Tuple, Type, Union, cast
from typing import Awaitable, Callable, Dict, List, Optional, Tuple, Type, Union

from ._core import Zeroconf
from ._dns import DNSQuestionType
Expand Down Expand Up @@ -86,15 +86,13 @@ def __init__(
question_type: Optional[DNSQuestionType] = None,
) -> None:
super().__init__(zeroconf, type_, handlers, listener, addr, port, delay, question_type)
self._setup()
# Start queries after the listener is installed in _setup
self._browser_task = cast(asyncio.Task, asyncio.ensure_future(self.async_browser_task()))
self._async_start()

async def async_cancel(self) -> None:
"""Cancel the browser."""
self._async_cancel()
with contextlib.suppress(asyncio.CancelledError):
await self._async_cancel_browser()
super().cancel()


class AsyncZeroconfServiceTypes(ZeroconfServiceTypes):
Expand Down