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

Timout in async and sync DoH query #978

Open
fleurysun opened this issue Aug 7, 2023 · 3 comments
Open

Timout in async and sync DoH query #978

fleurysun opened this issue Aug 7, 2023 · 3 comments
Labels
Bug Enhancement Request Future A longer term issue, possibly addressed in the future.

Comments

@fleurysun
Copy link

Sometimes timeout in dns.asyncquery.https and dns.query.https doesn't work as expected, when querying server 75.119.137.220, which in fact isn't a DoH server, it will never timeout, it will hang at calling the_client.get or the_client.post method.

To Reproduce:

import asyncio
import dns.message
import dns.asyncquery
import dns.rdatatype

async def check_doh(ip, query_name, timeout):
    query = dns.message.make_query(query_name, dns.rdatatype.A)
    response = await dns.asyncquery.https(query, ip, verify=False, timeout=timeout)
    print(response)

if __name__ == "__main__":
    asyncio.run(check_doh("75.119.137.220", "dnspython.org", 6))

System info:

  • dnspython version [2.5.0]
  • Python version [3.10.12]
  • OS version [CentOS 7.8.2003]
@rthalley
Copy link
Owner

rthalley commented Aug 7, 2023

This one seems to be a limitation in the way timeouts work in httpx/httpcore. I'm seeing the correct timeout value get passed into httpcore, but the API only has the concept of individual operation timeouts. I see it trying to get the body of the response, and constantly reading 16KiB chunks of data, and this seems to go on for some time, certainly longer than the 6 second lifetime timeout we would like.

I am not sure how to fix this for the sync API, but for async I can fix it by limiting how long we wait for the future. E.g. if I do this:

async def check_doh(ip, query_name, timeout):
    query = dns.message.make_query(query_name, dns.rdatatype.A)
    response = await asyncio.wait_for(dns.asyncquery.https(query, ip, verify=False), timeout)
    print(response)

it returns after 6 seconds. I can move that kind of logic into dns.asyncquery.https().

rthalley added a commit that referenced this issue Aug 8, 2023
according to the timeout [#978].

Unfortunately we do not currently have a good way to
make this guarantee for sync https() calls.
rthalley added a commit that referenced this issue Aug 8, 2023
according to the timeout [#978].

Unfortunately we do not currently have a good way to
make this guarantee for sync https() calls.

(cherry picked from commit a22644d)
@fleurysun
Copy link
Author

fleurysun commented Aug 8, 2023

Thank you!
Yep, the httpx API timeouts aren't fully supported, as mentioned in encode/httpx#2658

@rthalley
Copy link
Owner

I'm leaving this ticket open so we remember to fix this for sync if httpx gives us a way to do so in the future. I made fixes for asyncio and trio, and they were included in the 2.4.2 release.

@rthalley rthalley added the Future A longer term issue, possibly addressed in the future. label Nov 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Enhancement Request Future A longer term issue, possibly addressed in the future.
Projects
None yet
Development

No branches or pull requests

2 participants