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

Jump to bottom

curl_multi_timeout() returns very large timeout while DNS resolving #2996

Closed
sagebind opened this issue Sep 15, 2018 · 5 comments
Closed

curl_multi_timeout() returns very large timeout while DNS resolving #2996

sagebind opened this issue Sep 15, 2018 · 5 comments

Comments

@sagebind
Copy link
Contributor

I did this

My code experiencing this issue is written in Rust using the wrapper crate, but we concluded that this is likely an upstream bug.

I am using the multi interface to drive easy handles in a loop. The basic logic is something like this:

while (work_to_do) {
    curl_multi_perform(multi_handle, &still_running);
    curl_multi_timeout(multi_handle, &timeout);
    curl_multi_wait(multi_handle, &extra_fds, 1, timeout, &numfds);
}

When attaching an easy handle with a URL that needs resolving (bare IP address has no problems), curl_multi_timeout() is returning the value of CURLOPT_CONNECTTIMEOUT_MS (which by default is 300 seconds).

By itself, this is not a problem, but the fact that I have 1 extra file descriptor I want to poll (this is used in my lib to receive "wakeups" from another thread and has very low activity), curl_multi_wait() decides to poll just my extra fd for up to 300 seconds. This basically causes the entire application to hang for 300 seconds.

I raise this issue because a previous version of libcurl did not exhibit this behavior. See more details on this in alexcrichton/curl-rust#227.

I expected the following

I expected curl_multi_timeout() to return some small timeout to indicate that curl_multi_wait() should not be called for very long to avoid indefinite blocking on nothing.

curl/libcurl version

libcurl/7.61.1-DEV OpenSSL/1.0.2g zlib/1.2.11 nghttp2/1.33.90

operating system

Ubuntu 17.10
@bagder
Copy link
Member

bagder commented Sep 15, 2018

This is using the threaded resolver right? And no specific timeout set?

It is a bit weird to call curl_multi_timeout() to ask for the timeout to pass to curl_multi_wait() and it is probably based on a misunderstanding. The timeout to this latter function is supposed to be the maximum time your app can tolerate to wait, and you shouldn't need to ask libcurl about that...

However, I think the real bug (even if you remove the curl_multi_timeout() call might be that this logic:

https://github.com/curl/curl/blob/master/lib/multi.c#L1016-L1018

is done before:

curl/lib/multi.c

Line 1023 in 0d717a3

bitmap = multi_getsock(data, sockbunch, MAX_SOCKSPEREASYHANDLE);

... as the latter function will end up calling https://github.com/curl/curl/blob/master/lib/asyn-thread.c#L564-L585 which sets up a suitable timeout for while the thread is resolving a host.

I'll push a PR in a sec to address that particular flaw.

bagder added a commit that referenced this issue Sep 15, 2018
@bagder
curl_multi_wait: call getsock before figuring out timeout
96fa8ba 
.... since it may update the expiry timer.

Fixes #2996
@bagder bagder mentioned this issue Sep 15, 2018
Closed
@sagebind
Copy link
Contributor Author

Yes, libcurl is built with the threaded resolver, and I am letting libcurl use its default connect timeout (currently 300 seconds).

It is a bit weird to call curl_multi_timeout() to ask for the timeout to pass to curl_multi_wait() [...]

Ah yes, now that I look closer that is a bit silly of me, as curl_multi_wait() will effectively call curl_multi_timeout() anyway and truncate the timeout I specify to at most that value. So yeah, harmless, but rather silly to do.

So the real bug is still that timers are not being set correctly. I had a hunch, but I wasn't familiar enough with the code to deduce that.

Until my lib can use the patch for this, I'll keep doing it the silly way though, since my current workaround is to assume that if curl_multi_timeout() returns the connect timeout, then this bug is occurring and I instead pass a small timeout value to curl_multi_wait().

@bagder
Copy link
Member

bagder commented Sep 16, 2018

Until my lib can use the patch for this

It'd be great if you could verify that the patch actually fixes your case as intended!

@sagebind
Copy link
Contributor Author

sagebind commented Sep 16, 2018 •
edited

I will pull in the fix locally sometime today and see if it fixes the problem.

(I'll have to wait until the Rust bindings updates it's curl version before I can publish an actual release with the fix.)

Update: Just tested the patch, the change does indeed fix the behavior for my use case.

@bagder
Copy link
Member

bagder commented Sep 18, 2018

Thanks for confirming @sagebind!

@bagder bagder closed this as completed in ec5d099 Sep 18, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Dec 17, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@bagder @sagebind