async_dns icon indicating copy to clipboard operation
async_dns copied to clipboard

[feature] Add an option to suppress CancelledError

Open jonmartz opened this issue 3 years ago • 2 comments

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.

jonmartz avatar May 22 '22 12:05 jonmartz

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?

gera2ld avatar May 22 '22 14:05 gera2ld

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.

jonmartz avatar May 23 '22 14:05 jonmartz