gera2ld / async_dns

DNS library based on asyncio
MIT License
70 stars 17 forks source link

[feature] Add an option to suppress CancelledError #28

Open jonmartz opened 2 years ago

jonmartz commented 2 years ago

Hello,

In recursive_resolver.py there is a try block that we suspect could be preventing some cancelled programs from actually exiting:

image

If a asyncio.CancelledError is thrown on line 145, it would be supressed by the except clause. Isn't this an issue? Or is it intentional?

In case it's an issue, a possible solution would be to add the following two lines above the except in line 147:

except asyncio.CancelledError:
    raise

Thanks.

gera2ld commented 2 years ago

Hi, thanks for reaching out, I think it was intentional. If we cancel the request on CancelledError due to timeout, we will lose the chance to get the response for the ongoing request and we will need to restart it again later if we still need it.

For example, a query takes a timeout of 3.0s by default, and if it contains 2 requests, each taking 2s, then the query will time out on the second request, and your question is, whether the second request should be canceled.

If the request is canceled, and later the user retries, we still have to resend the request, and it's likely to fail again. But if the request is shielded and it continues to wait for a full timeout (3.0s) as an independent query, it will finally succeed and update the cache. So when the user retries, we can provide the cached response immediately.

BTW, did this behavior cause any problem for you?

jonmartz commented 2 years ago

Thanks, that makes sense.

Our problem is that a program that should terminate (due to an exception) instead hangs forever, apparently because one or more asyncio task cancellations failed due to try blocks that stop the raise of asyncio.CancelledError.

That being said, we are not sure if specifically async_dns is causing the problem.