Skip to content

Commit c0f4f48

Browse files
bdracobdraco
authored
Implement duplicate question supression (#770)
https://datatracker.ietf.org/doc/html/rfc6762#section-7.3
1 parent b5d54e4 commit c0f4f48

11 files changed

Lines changed: 285 additions & 14 deletions

File tree

‎tests/__init__.py‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,3 +65,4 @@ def has_working_ipv6():
6565

6666
def _clear_cache(zc):
6767
zc.cache.cache.clear()
68+
zc.question_history._history.clear()

‎tests/services/test_browser.py‎

Lines changed: 45 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
from zeroconf._services import ServiceStateChange
2121
from zeroconf._services.browser import ServiceBrowser
2222
from zeroconf._services.info import ServiceInfo
23+
from zeroconf.aio import AsyncZeroconf
2324

2425
from .. import has_working_ipv6, _inject_response
2526

@@ -426,7 +427,8 @@ def _mock_get_expiration_time(self, percent):
426427
zeroconf.close()
427428

428429

429-
def test_backoff():
430+
@unittest.mock.patch("zeroconf._core.QuestionHistory.suppresses", return_value=False)
431+
def test_backoff(suppresses_mock):
430432
got_query = Event()
431433

432434
type_ = "_http._tcp.local."
@@ -902,3 +904,45 @@ def test_group_ptr_queries_with_known_answers():
902904
# If we generate multiple packets there must
903905
# only be one question
904906
assert len(packets) == 1 or len(out.questions) == 1
907+
908+
909+
# This test uses asyncio because it needs to access the cache directly
910+
# which is not threadsafe
911+
@pytest.mark.asyncio
912+
async def test_generate_service_query_suppress_duplicate_questions():
913+
"""Generate a service query for sending with zeroconf.send."""
914+
aiozc = AsyncZeroconf(interfaces=['127.0.0.1'])
915+
zc = aiozc.zeroconf
916+
now = current_time_millis()
917+
name = "_hap._tcp.local."
918+
question = r.DNSQuestion(name, const._TYPE_PTR, const._CLASS_IN)
919+
answer = r.DNSPointer(
920+
name,
921+
const._TYPE_PTR,
922+
const._CLASS_IN,
923+
10000,
924+
f'known-to-other.{name}',
925+
)
926+
other_known_answers = set([answer])
927+
zc.question_history.add_question_at_time(question, now, other_known_answers)
928+
assert zc.question_history.suppresses(question, now, other_known_answers)
929+
930+
# The known answer list is different, do not suppress
931+
outs = _services_browser.generate_service_query(zc, now, [name], multicast=True)
932+
assert outs
933+
934+
zc.cache.async_add_records([answer])
935+
# The known answer list contains all the asked questions in the history
936+
# we should suppress
937+
938+
outs = _services_browser.generate_service_query(zc, now, [name], multicast=True)
939+
assert not outs
940+
941+
# We do not suppress once the question history expires
942+
outs = _services_browser.generate_service_query(zc, now + 1000, [name], multicast=True)
943+
assert outs
944+
945+
# We do not suppress QU queries ever
946+
outs = _services_browser.generate_service_query(zc, now, [name], multicast=False)
947+
assert outs
948+
await aiozc.async_close()

‎tests/test_core.py‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,10 +50,26 @@ async def test_reaper():
5050
record_with_10s_ttl = r.DNSAddress('a', const._TYPE_SOA, const._CLASS_IN, 10, b'a')
5151
record_with_1s_ttl = r.DNSAddress('a', const._TYPE_SOA, const._CLASS_IN, 1, b'b')
5252
zeroconf.cache.async_add_records([record_with_10s_ttl, record_with_1s_ttl])
53+
question = r.DNSQuestion("_hap._tcp._local.", const._TYPE_PTR, const._CLASS_IN)
54+
now = r.current_time_millis()
55+
other_known_answers = set(
56+
[
57+
r.DNSPointer(
58+
"_hap._tcp.local.",
59+
const._TYPE_PTR,
60+
const._CLASS_IN,
61+
10000,
62+
'known-to-other._hap._tcp.local.',
63+
)
64+
]
65+
)
66+
zeroconf.question_history.add_question_at_time(question, now, other_known_answers)
67+
assert zeroconf.question_history.suppresses(question, now, other_known_answers)
5368
entries_with_cache = list(itertools.chain(*[cache.entries_with_name(name) for name in cache.names()]))
5469
await asyncio.sleep(1.2)
5570
entries = list(itertools.chain(*[cache.entries_with_name(name) for name in cache.names()]))
5671
await aiozc.async_close()
72+
assert not zeroconf.question_history.suppresses(question, now, other_known_answers)
5773
assert entries != original_entries
5874
assert entries_with_cache != original_entries
5975
assert record_with_10s_ttl in entries

‎tests/test_handlers.py‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -960,3 +960,53 @@ def async_update_records(self, zc: 'Zeroconf', now: float, records: List[r.DNSRe
960960

961961
assert set(updated) == set([ptr_record, a_record])
962962
await aiozc.async_close()
963+
964+
965+
def test_questions_query_handler_populates_the_question_history_from_qm_questions():
966+
zc = Zeroconf(interfaces=['127.0.0.1'])
967+
now = current_time_millis()
968+
_clear_cache(zc)
969+
970+
generated = r.DNSOutgoing(const._FLAGS_QR_QUERY)
971+
question = r.DNSQuestion("_hap._tcp._local.", const._TYPE_PTR, const._CLASS_IN)
972+
question.unicast = False
973+
known_answer = r.DNSPointer(
974+
"_hap._tcp.local.", const._TYPE_PTR, const._CLASS_IN, 10000, 'known-to-other._hap._tcp.local.'
975+
)
976+
generated.add_question(question)
977+
generated.add_answer_at_time(known_answer, 0)
978+
now = r.current_time_millis()
979+
packets = generated.packets()
980+
unicast_out, multicast_out = zc.query_handler.async_response(
981+
[r.DNSIncoming(packet) for packet in packets], "1.2.3.4", const._MDNS_PORT
982+
)
983+
assert unicast_out is None
984+
assert multicast_out is None
985+
assert zc.question_history.suppresses(question, now, set([known_answer]))
986+
987+
zc.close()
988+
989+
990+
def test_questions_query_handler_does_not_put_qu_questions_in_history():
991+
zc = Zeroconf(interfaces=['127.0.0.1'])
992+
now = current_time_millis()
993+
_clear_cache(zc)
994+
995+
generated = r.DNSOutgoing(const._FLAGS_QR_QUERY)
996+
question = r.DNSQuestion("_hap._tcp._local.", const._TYPE_PTR, const._CLASS_IN)
997+
question.unicast = True
998+
known_answer = r.DNSPointer(
999+
"_hap._tcp.local.", const._TYPE_PTR, const._CLASS_IN, 10000, 'known-to-other._hap._tcp.local.'
1000+
)
1001+
generated.add_question(question)
1002+
generated.add_answer_at_time(known_answer, 0)
1003+
now = r.current_time_millis()
1004+
packets = generated.packets()
1005+
unicast_out, multicast_out = zc.query_handler.async_response(
1006+
[r.DNSIncoming(packet) for packet in packets], "1.2.3.4", const._MDNS_PORT
1007+
)
1008+
assert unicast_out is None
1009+
assert multicast_out is None
1010+
assert not zc.question_history.suppresses(question, now, set([known_answer]))
1011+
1012+
zc.close()

‎tests/test_history.py‎

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,75 @@
1+
#!/usr/bin/env python
2+
# -*- coding: utf-8 -*-
3+
4+
5+
"""Unit tests for _history.py."""
6+
7+
from zeroconf._history import QuestionHistory
8+
import zeroconf as r
9+
import zeroconf.const as const
10+
11+
12+
def test_question_suppression():
13+
history = QuestionHistory()
14+
15+
question = r.DNSQuestion("_hap._tcp._local.", const._TYPE_PTR, const._CLASS_IN)
16+
now = r.current_time_millis()
17+
other_known_answers = set(
18+
[
19+
r.DNSPointer(
20+
"_hap._tcp.local.", const._TYPE_PTR, const._CLASS_IN, 10000, 'known-to-other._hap._tcp.local.'
21+
)
22+
]
23+
)
24+
our_known_answers = set(
25+
[
26+
r.DNSPointer(
27+
"_hap._tcp.local.", const._TYPE_PTR, const._CLASS_IN, 10000, 'known-to-us._hap._tcp.local.'
28+
)
29+
]
30+
)
31+
32+
history.add_question_at_time(question, now, other_known_answers)
33+
34+
# Verify the question is suppressed if the known answers are the same
35+
assert history.suppresses(question, now, other_known_answers)
36+
37+
# Verify the question is suppressed if we know the answer to all the known answers
38+
assert history.suppresses(question, now, other_known_answers | our_known_answers)
39+
40+
# Verify the question is not suppressed if our known answers do no include the ones in the last question
41+
assert not history.suppresses(question, now, set())
42+
43+
# Verify the question is not suppressed if our known answers do no include the ones in the last question
44+
assert not history.suppresses(question, now, our_known_answers)
45+
46+
# Verify the question is no longer suppressed after 1s
47+
assert not history.suppresses(question, now + 1000, other_known_answers)
48+
49+
50+
def test_question_expire():
51+
history = QuestionHistory()
52+
53+
question = r.DNSQuestion("_hap._tcp._local.", const._TYPE_PTR, const._CLASS_IN)
54+
now = r.current_time_millis()
55+
other_known_answers = set(
56+
[
57+
r.DNSPointer(
58+
"_hap._tcp.local.", const._TYPE_PTR, const._CLASS_IN, 10000, 'known-to-other._hap._tcp.local.'
59+
)
60+
]
61+
)
62+
history.add_question_at_time(question, now, other_known_answers)
63+
64+
# Verify the question is suppressed if the known answers are the same
65+
assert history.suppresses(question, now, other_known_answers)
66+
67+
history.async_expire(now)
68+
69+
# Verify the question is suppressed if the known answers are the same since the cache hasn't expired
70+
assert history.suppresses(question, now, other_known_answers)
71+
72+
history.async_expire(now + 1000)
73+
74+
# Verify the question not longer suppressed since the cache has expired
75+
assert not history.suppresses(question, now, other_known_answers)

‎zeroconf/_core.py‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
from ._dns import DNSQuestion
3636
from ._exceptions import NonUniqueNameException
3737
from ._handlers import QueryHandler, RecordManager
38+
from ._history import QuestionHistory
3839
from ._logger import QuietLogger, log
3940
from ._protocol import DNSIncoming, DNSOutgoing
4041
from ._services import RecordUpdateListener, ServiceListener
@@ -134,6 +135,7 @@ async def _async_cache_cleanup(self) -> None:
134135
"""Periodic cache cleanup."""
135136
while not self.zc.done:
136137
now = current_time_millis()
138+
self.zc.question_history.async_expire(now)
137139
self.zc.record_manager.async_updates(now, self.zc.cache.async_expire(now))
138140
self.zc.record_manager.async_updates_complete()
139141
await asyncio.sleep(millis_to_seconds(_CACHE_CLEANUP_INTERVAL))
@@ -288,7 +290,8 @@ def __init__(
288290
self.browsers: Dict[ServiceListener, ServiceBrowser] = {}
289291
self.registry = ServiceRegistry()
290292
self.cache = DNSCache()
291-
self.query_handler = QueryHandler(self.registry, self.cache)
293+
self.question_history = QuestionHistory()
294+
self.query_handler = QueryHandler(self.registry, self.cache, self.question_history)
292295
self.record_manager = RecordManager(self)
293296

294297
self.notify_event: Optional[asyncio.Event] = None

‎zeroconf/_dns.py‎

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -414,18 +414,19 @@ def __init__(self, records: Iterable[DNSRecord]) -> None:
414414
self._records = records
415415
self._lookup: Optional[Dict[DNSRecord, DNSRecord]] = None
416416

417-
def suppresses(self, record: DNSRecord) -> bool:
418-
"""Returns true if any answer in the rrset can suffice for the
419-
information held in this record."""
417+
@property
418+
def lookup(self) -> Dict[DNSRecord, DNSRecord]:
420419
if self._lookup is None:
421420
# Build the hash table so we can lookup the record independent of the ttl
422421
self._lookup = {record: record for record in self._records}
423-
other = self._lookup.get(record)
422+
return self._lookup
423+
424+
def suppresses(self, record: DNSRecord) -> bool:
425+
"""Returns true if any answer in the rrset can suffice for the
426+
information held in this record."""
427+
other = self.lookup.get(record)
424428
return bool(other and other.ttl > (record.ttl / 2))
425429

426430
def __contains__(self, record: DNSRecord) -> bool:
427431
"""Returns true if the rrset contains the record."""
428-
if self._lookup is None:
429-
# Build the hash table so we can lookup the record independent of the ttl
430-
self._lookup = {record: record for record in self._records}
431-
return record in self._lookup
432+
return record in self.lookup

‎zeroconf/_handlers.py‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -21,10 +21,11 @@
2121
"""
2222

2323
import itertools
24-
from typing import Dict, List, Optional, Set, TYPE_CHECKING, Tuple, Union, cast
24+
from typing import Dict, Iterable, List, Optional, Set, TYPE_CHECKING, Tuple, Union, cast
2525

2626
from ._cache import DNSCache, _UniqueRecordsType
2727
from ._dns import DNSAddress, DNSPointer, DNSQuestion, DNSRRSet, DNSRecord
28+
from ._history import QuestionHistory
2829
from ._logger import log
2930
from ._protocol import DNSIncoming, DNSOutgoing
3031
from ._services import RecordUpdateListener
@@ -156,10 +157,11 @@ def _has_mcast_record_in_last_second(self, record: DNSRecord) -> bool:
156157
class QueryHandler:
157158
"""Query the ServiceRegistry."""
158159

159-
def __init__(self, registry: ServiceRegistry, cache: DNSCache) -> None:
160+
def __init__(self, registry: ServiceRegistry, cache: DNSCache, question_history: QuestionHistory) -> None:
160161
"""Init the query handler."""
161162
self.registry = registry
162163
self.cache = cache
164+
self.question_history = question_history
163165

164166
def _add_service_type_enumeration_query_answers(
165167
self, answer_set: _AnswerWithAdditionalsType, known_answers: DNSRRSet, now: float
@@ -253,6 +255,8 @@ def async_response( # pylint: disable=unused-argument
253255

254256
for msg in msgs:
255257
for question in msg.questions:
258+
if not question.unicast:
259+
self.question_history.add_question_at_time(question, msg.now, set(known_answers.lookup))
256260
answer_set: _AnswerWithAdditionalsType = {}
257261
self._answer_question(question, answer_set, known_answers, msg.now)
258262
if not ucast_source and question.unicast:
@@ -364,7 +368,7 @@ def async_updates_from_response(self, msg: DNSIncoming) -> None:
364368
self.async_updates_complete()
365369

366370
def _async_mark_unique_cached_records_older_than_1s_to_expire(
367-
self, unique_types: Set[Tuple[str, int, int]], answers: List[DNSRecord], now: float
371+
self, unique_types: Set[Tuple[str, int, int]], answers: Iterable[DNSRecord], now: float
368372
) -> None:
369373
# rfc6762#section-10.2 para 2
370374
# Since unique is set, all old records with that name, rrtype,

‎zeroconf/_history.py‎

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,70 @@
1+
""" Multicast DNS Service Discovery for Python, v0.14-wmcbrine
2+
Copyright 2003 Paul Scott-Murphy, 2014 William McBrine
3+
4+
This module provides a framework for the use of DNS Service Discovery
5+
using IP multicast.
6+
7+
This library is free software; you can redistribute it and/or
8+
modify it under the terms of the GNU Lesser General Public
9+
License as published by the Free Software Foundation; either
10+
version 2.1 of the License, or (at your option) any later version.
11+
12+
This library is distributed in the hope that it will be useful,
13+
but WITHOUT ANY WARRANTY; without even the implied warranty of
14+
MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
15+
Lesser General Public License for more details.
16+
17+
You should have received a copy of the GNU Lesser General Public
18+
License along with this library; if not, write to the Free Software
19+
Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301
20+
USA
21+
"""
22+
23+
from typing import Dict, Set, Tuple
24+
25+
from ._dns import DNSQuestion, DNSRecord
26+
from .const import _DUPLICATE_QUESTION_INTERVAL
27+
28+
# The QuestionHistory is used to implement Duplicate Question Suppression
29+
# https://datatracker.ietf.org/doc/html/rfc6762#section-7.3
30+
31+
32+
class QuestionHistory:
33+
def __init__(self) -> None:
34+
self._history: Dict[DNSQuestion, Tuple[float, Set[DNSRecord]]] = {}
35+
36+
def add_question_at_time(self, question: DNSQuestion, now: float, known_answers: Set[DNSRecord]) -> None:
37+
"""Remember a question with known answers."""
38+
self._history[question] = (now, known_answers)
39+
40+
def suppresses(self, question: DNSQuestion, now: float, known_answers: Set[DNSRecord]) -> bool:
41+
"""Check to see if a question should be suppressed.
42+
43+
https://datatracker.ietf.org/doc/html/rfc6762#section-7.3
44+
When multiple queriers on the network are querying
45+
for the same resource records, there is no need for them to all be
46+
repeatedly asking the same question.
47+
"""
48+
previous_question = self._history.get(question)
49+
# There was not previous question in the history
50+
if not previous_question:
51+
return False
52+
than, previous_known_answers = previous_question
53+
# The last question was older than 999ms
54+
if now - than > _DUPLICATE_QUESTION_INTERVAL:
55+
return False
56+
# The last question has more known answers than
57+
# we knew so we have to ask
58+
if previous_known_answers - known_answers:
59+
return False
60+
return True
61+
62+
def async_expire(self, now: float) -> None:
63+
"""Expire the history of old questions."""
64+
removes = [
65+
question
66+
for question, now_known_answers in self._history.items()
67+
if now - now_known_answers[0] > _DUPLICATE_QUESTION_INTERVAL
68+
]
69+
for question in removes:
70+
del self._history[question]

0 commit comments

Comments
 (0)