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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure thread does not die due to uncaught exception #1163

Closed

Conversation

farrokhi
Copy link

@farrokhi farrokhi commented Nov 8, 2024

The caller should be able to catch and handle this exception, which is caused when a quic connection fails.

The caller should be able to catch and handle this exception, which is
caused when a quic connection fails.
@rthalley
Copy link
Owner

rthalley commented Nov 8, 2024

Can you explain the theory of this change? The description says the caller should be able to catch the exception, but dnspython's QUIC abstraction doesn't expose any failure details (it just results in all the streams getting a short read and failing) and this PR doesn't add details. I think this change just delays the recognition of the error and then results in the same handling.

When the worker thread exits, it's supposed to cause any waiting streams to fail; did this not happen for you?

@farrokhi
Copy link
Author

farrokhi commented Nov 9, 2024

Here is a test case:

#!/usr/bin/env python3

import sys

import httpx
import dns.message
import dns.query
import dns.rdatatype


def main():
    where = "https://127.0.0.1/dns-query"
    qname = "example.com."
    with httpx.Client() as client:
        q = dns.message.make_query(qname, dns.rdatatype.A)
        try:
            r = dns.query.https(q, where, session=client, http_version=dns.query.HTTPVersion.H3)
        except (ConnectionRefusedError, httpx.ConnectError, dns.quic._common.UnexpectedEOF):
            print(f"The server did not respond to DNS-over-HTTPS/3")
            sys.exit(1)

        for answer in r.answer:
            print(answer)


if __name__ == "__main__":
    main()

The exception that is raised inside the thread cannot be caught and causes this output:

Exception in thread Thread-1 (_worker):
Traceback (most recent call last):
  File "/opt/homebrew/Cellar/python@3.12/3.12.7_1/Frameworks/Python.framework/Versions/3.12/lib/python3.12/threading.py", line 1075, in _bootstrap_inner
    self.run()
  File "/opt/homebrew/Cellar/python@3.12/3.12.7_1/Frameworks/Python.framework/Versions/3.12/lib/python3.12/threading.py", line 1012, in run
    self._target(*self._args, **self._kwargs)
  File "/Users/farrokhi/dev/dnsdiag/.venv/lib/python3.12/site-packages/dns/quic/_sync.py", line 142, in _worker
    key.data()
  File "/Users/farrokhi/dev/dnsdiag/.venv/lib/python3.12/site-packages/dns/quic/_sync.py", line 119, in _read
    datagram = self._socket.recv(QUIC_MAX_DATAGRAM)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
ConnectionRefusedError: [Errno 61] Connection refused
The server did not respond to DNS-over-HTTPS/3

@rthalley
Copy link
Owner

I finally had time to look at this properly and fixed it in a more comprehensive way by ensuring that no exceptions escape the worker thread. I might come back to this in the future with a way to return a more useful exception to the caller, but in the meantime this will end the annoying traceback info.

Thanks for the example!

/Bob

@rthalley rthalley closed this Nov 17, 2024
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.

2 participants