Repository: mitmproxy/mitmproxy
Pull request: Make `DnsResolver.resolver` and `DnsResolver.name_servers` as cached functions instead of cached properties
#### Description
Fixes #7064 
`mitmproxy_rs.get_system_dns_servers()` might fail with `RuntimeError`s, which would crash mitmproxy while initializing `DnsResolver`'s attributes during startup. We fix this by making `DnsResolver.resolver` and `DnsResolver.name_servers` as cached functions instead of cached properties

#### Checklist

 - [x] I have updated tests where applicable.
 - [x] I have added an entry to the CHANGELOG.

Problem statement:
【10.4.1】 mitm start failed: failed to get dns servers
#### Problem Description
After upgrading to 10.4.1 from 10.3.1, the mitm program could not be loaded.

#### Steps to reproduce the behavior:
1.  **pipx** --upgrade mitmproxy
2.  start mitmproxy
3. 

#### System Information
Paste the output of "mitmproxy --version" here.
**Failed**
(base) root@machine:/home/projects/mitmproxy# mitmproxy --version
Traceback (most recent call last):
  File "mitmproxy.py", line 3, in <module>
  File "mitmproxy/tools/main.py", line 137, in mitmproxy
  File "mitmproxy/tools/main.py", line 131, in run
  File "asyncio/runners.py", line 194, in run
  File "asyncio/runners.py", line 118, in run
  File "asyncio/base_events.py", line 687, in run_until_complete
  File "mitmproxy/tools/main.py", line 67, in main
  File "mitmproxy/tools/console/master.py", line 50, in __init__
  File "mitmproxy/addonmanager.py", line 204, in add
  File "mitmproxy/addonmanager.py", line 194, in register
  File "mitmproxy/command.py", line 177, in collect_commands
  File "functools.py", line 995, in __get__
  File "mitmproxy/addons/dns_resolver.py", line 52, in name_servers
RuntimeError: failed to get dns servers: io error: other error
[1324413] Failed to execute script 'mitmproxy' due to unhandled exception!

Hints from issue discussion:
Which OS are you on?
`
(base) root@ttg-virtual-machine:~# uname -a
Linux ttg-virtual-machine 5.4.0-181-generic #201-Ubuntu SMP Thu Mar 28 15:39:01 UTC 2024 x86_64 x86_64 x86_64 GNU/Linux
`
When I downgraded the app version to 10.3.1, everything went back to normal.
> Which OS are you on?

Ubuntu 20.04.6 LTS
Thanks! @errorxyz, can we hotfix this to use functool's `@cache` instead of `@cached_property`?
I get the same exception on Windows 11 using versions 10.4.0 and 10.4.1.  10.3.1 is working fine.
Thank you folks! @camckee, we'd be highly interested in getting the specific error message on Windows.

Irrespective of that, we'll likely ship a patch release for this today. :)

Commit message: Fixup

Patch to review:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index c33b602f45..b08e326bf4 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -16,6 +16,7 @@
   ([#7045](https://github.com/mitmproxy/mitmproxy/pull/7045), @mhils)
 - Container images are now published to both Docker Hub and GitHub Container Registry.
   ([#7061](https://github.com/mitmproxy/mitmproxy/pull/7061), @mhils)
+- Make `DnsResolver.resolver` and `DnsResolver.name_servers` as cached functions instead of cached properties
 
 ## 25 July 2024: mitmproxy 10.4.0
 
diff --git a/mitmproxy/addons/dns_resolver.py b/mitmproxy/addons/dns_resolver.py
index b362640c52..257f546ba8 100644
--- a/mitmproxy/addons/dns_resolver.py
+++ b/mitmproxy/addons/dns_resolver.py
@@ -2,7 +2,7 @@
 import socket
 from collections.abc import Iterable
 from collections.abc import Sequence
-from functools import cached_property
+from functools import cache
 
 import mitmproxy_rs
 
@@ -37,19 +37,24 @@ def load(self, loader):
 
     def configure(self, updated):
         if "dns_use_hosts_file" in updated or "dns_name_servers" in updated:
-            self.__dict__.pop("resolver", None)
-            self.__dict__.pop("name_servers", None)
+            self.resolver.cache_clear()
+            self.name_servers.cache_clear()
 
-    @cached_property
+    @cache
     def resolver(self) -> mitmproxy_rs.DnsResolver:
         return mitmproxy_rs.DnsResolver(
-            name_servers=self.name_servers,
+            name_servers=self.name_servers(),
             use_hosts_file=ctx.options.dns_use_hosts_file,
         )
 
-    @cached_property
+    @cache
     def name_servers(self) -> list[str]:
-        return ctx.options.dns_name_servers or mitmproxy_rs.get_system_dns_servers()
+        try:
+            return ctx.options.dns_name_servers or mitmproxy_rs.get_system_dns_servers()
+        except RuntimeError as e:  # pragma: no cover
+            raise RuntimeError(
+                f"Failed to get system dns servers: {e}\nMust set dns_name_servers option to run DNS mode."
+            )
 
     async def dns_request(self, flow: dns.DNSFlow) -> None:
         assert flow.request
@@ -81,7 +86,7 @@ async def dns_request(self, flow: dns.DNSFlow) -> None:
                 # TODO: We need to handle overly long responses here.
                 flow.response = await self.resolve_message(flow.request)
             elif not flow.server_conn.address:
-                flow.server_conn.address = (self.name_servers[0], 53)
+                flow.server_conn.address = (self.name_servers()[0], 53)
 
     async def resolve_message(self, message: dns.Message) -> dns.Message:
         try:
@@ -100,9 +105,9 @@ async def resolve_question(
 
         try:
             if question.type == dns.types.A:
-                addrinfos = await self.resolver.lookup_ipv4(question.name)
+                addrinfos = await self.resolver().lookup_ipv4(question.name)
             elif question.type == dns.types.AAAA:
-                addrinfos = await self.resolver.lookup_ipv6(question.name)
+                addrinfos = await self.resolver().lookup_ipv6(question.name)
         except socket.gaierror as e:
             # We aren't exactly following the RFC here
             # https://datatracker.ietf.org/doc/html/rfc2308#section-2
diff --git a/test/mitmproxy/addons/test_dns_resolver.py b/test/mitmproxy/addons/test_dns_resolver.py
index 97cc9d7276..b99155134b 100644
--- a/test/mitmproxy/addons/test_dns_resolver.py
+++ b/test/mitmproxy/addons/test_dns_resolver.py
@@ -27,17 +27,17 @@ async def test_ignores_reverse_mode():
 async def test_resolver():
     dr = dns_resolver.DnsResolver()
     with taddons.context(dr) as tctx:
-        assert dr.name_servers == mitmproxy_rs.get_system_dns_servers()
+        assert dr.name_servers() == mitmproxy_rs.get_system_dns_servers()
 
         tctx.options.dns_name_servers = ["1.1.1.1"]
-        assert dr.name_servers == ["1.1.1.1"]
+        assert dr.name_servers() == ["1.1.1.1"]
 
-        res_old = dr.resolver
+        res_old = dr.resolver()
         tctx.options.dns_use_hosts_file = False
-        assert dr.resolver != res_old
+        assert dr.resolver() != res_old
 
         tctx.options.dns_name_servers = ["8.8.8.8"]
-        assert dr.name_servers == ["8.8.8.8"]
+        assert dr.name_servers() == ["8.8.8.8"]
 
 
 async def lookup_ipv4(name: str):
@@ -68,14 +68,14 @@ async def process_questions(questions):
         flow = tflow.tdnsflow(req=req)
         flow.server_conn.address = None
         await resolver.dns_request(flow)
-        assert flow.server_conn.address[0] == resolver.name_servers[0]
+        assert flow.server_conn.address[0] == resolver.name_servers()[0]
 
         req.query = False
         req.op_code = dns.op_codes.QUERY
         flow = tflow.tdnsflow(req=req)
         flow.server_conn.address = None
         await resolver.dns_request(flow)
-        assert flow.server_conn.address[0] == resolver.name_servers[0]
+        assert flow.server_conn.address[0] == resolver.name_servers()[0]
 
         flow = await process_questions(
             [
@@ -83,7 +83,7 @@ async def process_questions(questions):
                 dns.Question("dns.google", dns.types.NS, dns.classes.IN),
             ]
         )
-        assert flow.server_conn.address[0] == resolver.name_servers[0]
+        assert flow.server_conn.address[0] == resolver.name_servers()[0]
 
         flow = await process_questions(
             [
@@ -119,4 +119,4 @@ async def process_questions(questions):
                 dns.Question("dns.google", dns.types.A, dns.classes.IN),
             ]
         )
-        assert flow.server_conn.address[0] == resolver.name_servers[0]
+        assert flow.server_conn.address[0] == resolver.name_servers()[0]


Code review comments:
diff_hunk: @@ -16,6 +16,7 @@
   ([#7045](https://github.com/mitmproxy/mitmproxy/pull/7045), @mhils)
 - Container images are now published to both Docker Hub and GitHub Container Registry.
   ([#7061](https://github.com/mitmproxy/mitmproxy/pull/7061), @mhils)
+- Make `DnsResolver.resolver` and `DnsResolver.name_servers` as cached functions instead of cached properties
path: CHANGELOG.md
text: @user1:
Wrong spot and not an ideal message. Focus on the user's perspective. I'd suggest something like "fix a crash on startup when mitmproxy is unable to determine the OS' DNS servers.".

@author:
Makes sense👍