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

curl_multi_remove_handle() can block until DNS resolution finishes even when using c-ares #3371

Closed
b-spencer opened this issue Dec 12, 2018 · 3 comments
Labels
name lookup DNS and related tech

Comments

@b-spencer
Copy link
Contributor

This is related to #2975, but it's not exactly the same issue.

When configured to use c-ares as the DNS resolver, and a handle is still waiting for DNS resolution (such as when the DNS server is unreachable), the curl_multi_remove_handle() call can block until that DNS request times out.

This behaviour appears to have been introduced as part of ac9a179.

I understand that it might not be possible to cancel an in-progress DNS request with all resolver backends without blocking. It does seem like it should be possible to cancel an in-progress c-ares DNS request without blocking.

Changing the multi_done() call of Curl_resolver_wait_resolv() to Curl_resolver_cancel() seems to work, but I'm not familiar enough with the details of resolver management inside cURL (especially for other resolver backends) to know if this is correct or "safe" in all cases. Based on the discussion in #2975, it sounds like the threaded resolver must join with the thread.

Perhaps Curl_resolver_cancel() should take a flag that says whether it must not return until all resources are freed. The c-ares resolver could then simply ignore the flag since has nothing special to do, then clean up and return quickly, while the threaded resolver could block until it has joined with the thread.

(BTW, it looks like there is at least one case where Curl_resolver_cancel() is called when it "shouldn't" wait, in conn_free(), but I'm not sure how the threaded resolver's threads get cleaned up in that case.)

@bagder bagder added the name lookup DNS and related tech label Dec 13, 2018
@bagder
Copy link
Member

bagder commented Dec 14, 2018

Agreed. The c-ares backend shouldn't wait for completion for resolves it doesn't care about anymore and it doesn't have to do it.

We could even just make it more explicit and provide a Curl_resolver_killall function in the resolver backends that would do the right thing, as you describe.

Would you like to work on a fix for this?

@b-spencer
Copy link
Contributor Author

I will take a look, yes, but I might not be able to publish a patch until January. There are only the two resolvers to worry about, right? asyn-ares and asyn-thread?

@bagder
Copy link
Member

bagder commented Dec 14, 2018

Right. The third one is the synchronous backend but that can probably be implemented just as an empty macro or something since it will never actually end up in that state when used.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
name lookup DNS and related tech
Development

No branches or pull requests

2 participants