mirror of
https://github.com/moparisthebest/curl
synced 2025-01-13 06:58:01 -05:00
curl_multi_remove_handle() don't block terminating c-ares requests
Added Curl_resolver_kill() for all three resolver modes, which only blocks when necessary, along with test 1592 to confirm curl_multi_remove_handle() doesn't block unless it must. Closes #3428 Fixes #3371
This commit is contained in:
parent
ebe658c1e5
commit
84a30d0a41
@ -5,7 +5,7 @@
|
||||
* | (__| |_| | _ <| |___
|
||||
* \___|\___/|_| \_\_____|
|
||||
*
|
||||
* Copyright (C) 1998 - 2018, Daniel Stenberg, <daniel@haxx.se>, et al.
|
||||
* Copyright (C) 1998 - 2019, Daniel Stenberg, <daniel@haxx.se>, et al.
|
||||
*
|
||||
* This software is licensed as described in the file COPYING, which
|
||||
* you should have received as part of this distribution. The terms
|
||||
@ -198,6 +198,17 @@ void Curl_resolver_cancel(struct connectdata *conn)
|
||||
destroy_async_data(&conn->async);
|
||||
}
|
||||
|
||||
/*
|
||||
* We're equivalent to Curl_resolver_cancel() for the c-ares resolver. We
|
||||
* never block.
|
||||
*/
|
||||
void Curl_resolver_kill(struct connectdata *conn)
|
||||
{
|
||||
/* We don't need to check the resolver state because we can be called safely
|
||||
at any time and we always do the same thing. */
|
||||
Curl_resolver_cancel(conn);
|
||||
}
|
||||
|
||||
/*
|
||||
* destroy_async_data() cleans up async resolver data.
|
||||
*/
|
||||
@ -361,13 +372,13 @@ CURLcode Curl_resolver_is_resolved(struct connectdata *conn,
|
||||
/*
|
||||
* Curl_resolver_wait_resolv()
|
||||
*
|
||||
* waits for a resolve to finish. This function should be avoided since using
|
||||
* Waits for a resolve to finish. This function should be avoided since using
|
||||
* this risk getting the multi interface to "hang".
|
||||
*
|
||||
* If 'entry' is non-NULL, make it point to the resolved dns entry
|
||||
*
|
||||
* Returns CURLE_COULDNT_RESOLVE_HOST if the host was not resolved, and
|
||||
* CURLE_OPERATION_TIMEDOUT if a time-out occurred.
|
||||
* Returns CURLE_COULDNT_RESOLVE_HOST if the host was not resolved,
|
||||
* CURLE_OPERATION_TIMEDOUT if a time-out occurred, or other errors.
|
||||
*/
|
||||
CURLcode Curl_resolver_wait_resolv(struct connectdata *conn,
|
||||
struct Curl_dns_entry **entry)
|
||||
|
@ -5,7 +5,7 @@
|
||||
* | (__| |_| | _ <| |___
|
||||
* \___|\___/|_| \_\_____|
|
||||
*
|
||||
* Copyright (C) 1998 - 2018, Daniel Stenberg, <daniel@haxx.se>, et al.
|
||||
* Copyright (C) 1998 - 2019, Daniel Stenberg, <daniel@haxx.se>, et al.
|
||||
*
|
||||
* This software is licensed as described in the file COPYING, which
|
||||
* you should have received as part of this distribution. The terms
|
||||
@ -461,14 +461,34 @@ static CURLcode resolver_error(struct connectdata *conn)
|
||||
return result;
|
||||
}
|
||||
|
||||
/*
|
||||
* Until we gain a way to signal the resolver threads to stop early, we must
|
||||
* simply wait for them and ignore their results.
|
||||
*/
|
||||
void Curl_resolver_kill(struct connectdata *conn)
|
||||
{
|
||||
struct thread_data *td = (struct thread_data*) conn->async.os_specific;
|
||||
|
||||
/* If we're still resolving, we must wait for the threads to fully clean up,
|
||||
unfortunately. Otherwise, we can simply cancel to clean up any resolver
|
||||
data. */
|
||||
if(td && td->thread_hnd != curl_thread_t_null)
|
||||
(void)Curl_resolver_wait_resolv(conn, NULL);
|
||||
else
|
||||
Curl_resolver_cancel(conn);
|
||||
}
|
||||
|
||||
/*
|
||||
* Curl_resolver_wait_resolv()
|
||||
*
|
||||
* waits for a resolve to finish. This function should be avoided since using
|
||||
* Waits for a resolve to finish. This function should be avoided since using
|
||||
* this risk getting the multi interface to "hang".
|
||||
*
|
||||
* If 'entry' is non-NULL, make it point to the resolved dns entry
|
||||
*
|
||||
* Returns CURLE_COULDNT_RESOLVE_HOST if the host was not resolved,
|
||||
* CURLE_OPERATION_TIMEDOUT if a time-out occurred, or other errors.
|
||||
*
|
||||
* This is the version for resolves-in-a-thread.
|
||||
*/
|
||||
CURLcode Curl_resolver_wait_resolv(struct connectdata *conn,
|
||||
@ -478,6 +498,7 @@ CURLcode Curl_resolver_wait_resolv(struct connectdata *conn,
|
||||
CURLcode result = CURLE_OK;
|
||||
|
||||
DEBUGASSERT(conn && td);
|
||||
DEBUGASSERT(td->thread_hnd != curl_thread_t_null);
|
||||
|
||||
/* wait for the thread to resolve the name */
|
||||
if(Curl_thread_join(&td->thread_hnd)) {
|
||||
|
27
lib/asyn.h
27
lib/asyn.h
@ -7,7 +7,7 @@
|
||||
* | (__| |_| | _ <| |___
|
||||
* \___|\___/|_| \_\_____|
|
||||
*
|
||||
* Copyright (C) 1998 - 2011, Daniel Stenberg, <daniel@haxx.se>, et al.
|
||||
* Copyright (C) 1998 - 2019, Daniel Stenberg, <daniel@haxx.se>, et al.
|
||||
*
|
||||
* This software is licensed as described in the file COPYING, which
|
||||
* you should have received as part of this distribution. The terms
|
||||
@ -87,10 +87,25 @@ CURLcode Curl_resolver_duphandle(struct Curl_easy *easy, void **to,
|
||||
*
|
||||
* It is called from inside other functions to cancel currently performing
|
||||
* resolver request. Should also free any temporary resources allocated to
|
||||
* perform a request.
|
||||
* perform a request. This never waits for resolver threads to complete.
|
||||
*
|
||||
* It is safe to call this when conn is in any state.
|
||||
*/
|
||||
void Curl_resolver_cancel(struct connectdata *conn);
|
||||
|
||||
/*
|
||||
* Curl_resolver_kill().
|
||||
*
|
||||
* This acts like Curl_resolver_cancel() except it will block until any threads
|
||||
* associated with the resolver are complete. This never blocks for resolvers
|
||||
* that do not use threads. This is intended to be the "last chance" function
|
||||
* that cleans up an in-progress resolver completely (before its owner is about
|
||||
* to die).
|
||||
*
|
||||
* It is safe to call this when conn is in any state.
|
||||
*/
|
||||
void Curl_resolver_kill(struct connectdata *conn);
|
||||
|
||||
/* Curl_resolver_getsock()
|
||||
*
|
||||
* This function is called from the multi_getsock() function. 'sock' is a
|
||||
@ -117,14 +132,13 @@ CURLcode Curl_resolver_is_resolved(struct connectdata *conn,
|
||||
/*
|
||||
* Curl_resolver_wait_resolv()
|
||||
*
|
||||
* waits for a resolve to finish. This function should be avoided since using
|
||||
* Waits for a resolve to finish. This function should be avoided since using
|
||||
* this risk getting the multi interface to "hang".
|
||||
*
|
||||
* If 'entry' is non-NULL, make it point to the resolved dns entry
|
||||
*
|
||||
* Returns CURLE_COULDNT_RESOLVE_HOST if the host was not resolved, and
|
||||
* CURLE_OPERATION_TIMEDOUT if a time-out occurred.
|
||||
|
||||
* Returns CURLE_COULDNT_RESOLVE_HOST if the host was not resolved,
|
||||
* CURLE_OPERATION_TIMEDOUT if a time-out occurred, or other errors.
|
||||
*/
|
||||
CURLcode Curl_resolver_wait_resolv(struct connectdata *conn,
|
||||
struct Curl_dns_entry **dnsentry);
|
||||
@ -148,6 +162,7 @@ Curl_addrinfo *Curl_resolver_getaddrinfo(struct connectdata *conn,
|
||||
#ifndef CURLRES_ASYNCH
|
||||
/* convert these functions if an asynch resolver isn't used */
|
||||
#define Curl_resolver_cancel(x) Curl_nop_stmt
|
||||
#define Curl_resolver_kill(x) Curl_nop_stmt
|
||||
#define Curl_resolver_is_resolved(x,y) CURLE_COULDNT_RESOLVE_HOST
|
||||
#define Curl_resolver_wait_resolv(x,y) CURLE_COULDNT_RESOLVE_HOST
|
||||
#define Curl_resolver_getsock(x,y,z) 0
|
||||
|
@ -537,10 +537,8 @@ static CURLcode multi_done(struct connectdata **connp,
|
||||
/* Stop if multi_done() has already been called */
|
||||
return CURLE_OK;
|
||||
|
||||
if(data->mstate == CURLM_STATE_WAITRESOLVE) {
|
||||
/* still waiting for the resolve to complete */
|
||||
(void)Curl_resolver_wait_resolv(conn, NULL);
|
||||
}
|
||||
/* Stop the resolver and free its own resources (but not dns_entry yet). */
|
||||
Curl_resolver_kill(conn);
|
||||
|
||||
Curl_getoff_all_pipelines(data, conn);
|
||||
|
||||
@ -587,7 +585,6 @@ static CURLcode multi_done(struct connectdata **connp,
|
||||
}
|
||||
|
||||
data->state.done = TRUE; /* called just now! */
|
||||
Curl_resolver_cancel(conn);
|
||||
|
||||
if(conn->dns_entry) {
|
||||
Curl_resolv_unlock(data, conn->dns_entry); /* done with this */
|
||||
|
@ -5,7 +5,7 @@
|
||||
# | (__| |_| | _ <| |___
|
||||
# \___|\___/|_| \_\_____|
|
||||
#
|
||||
# Copyright (C) 1998 - 2018, Daniel Stenberg, <daniel@haxx.se>, et al.
|
||||
# Copyright (C) 1998 - 2019, Daniel Stenberg, <daniel@haxx.se>, et al.
|
||||
#
|
||||
# This software is licensed as described in the file COPYING, which
|
||||
# you should have received as part of this distribution. The terms
|
||||
@ -179,8 +179,8 @@ test1550 test1551 test1552 test1553 test1554 test1555 test1556 test1557 \
|
||||
\
|
||||
test1560 test1561 \
|
||||
\
|
||||
test1590 \
|
||||
test1591 \
|
||||
test1590 test1591 test1592 \
|
||||
\
|
||||
test1600 test1601 test1602 test1603 test1604 test1605 test1606 test1607 \
|
||||
test1608 test1609 test1620 \
|
||||
\
|
||||
|
37
tests/data/test1592
Normal file
37
tests/data/test1592
Normal file
@ -0,0 +1,37 @@
|
||||
<testcase>
|
||||
<info>
|
||||
<keywords>
|
||||
HTTP
|
||||
multi
|
||||
resolve
|
||||
speedcheck
|
||||
</keywords>
|
||||
</info>
|
||||
|
||||
# Client-side
|
||||
<client>
|
||||
<server>
|
||||
none
|
||||
</server>
|
||||
<tool>
|
||||
lib1592
|
||||
</tool>
|
||||
<name>
|
||||
HTTP request, remove handle while resolving, don't block
|
||||
</name>
|
||||
|
||||
<command>
|
||||
http://a-site-never-accessed.example.org/1592
|
||||
</command>
|
||||
</client>
|
||||
|
||||
# Verify data after the test has been "shot"
|
||||
<verify>
|
||||
<valgrind>
|
||||
disable
|
||||
</valgrind>
|
||||
<errorcode>
|
||||
0
|
||||
</errorcode>
|
||||
</verify>
|
||||
</testcase>
|
@ -31,7 +31,7 @@ noinst_PROGRAMS = chkhostname libauthretry libntlmconnect \
|
||||
lib1540 \
|
||||
lib1550 lib1551 lib1552 lib1553 lib1554 lib1555 lib1556 lib1557 \
|
||||
lib1560 \
|
||||
lib1591 \
|
||||
lib1591 lib1592 \
|
||||
lib1900 \
|
||||
lib2033
|
||||
|
||||
@ -523,6 +523,10 @@ lib1591_SOURCES = lib1591.c $(SUPPORTFILES) $(TESTUTIL) $(WARNLESS)
|
||||
lib1591_LDADD = $(TESTUTIL_LIBS)
|
||||
lib1591_CPPFLAGS = $(AM_CPPFLAGS) -DLIB1591
|
||||
|
||||
lib1592_SOURCES = lib1592.c $(SUPPORTFILES) $(TESTUTIL) $(WARNLESS)
|
||||
lib1592_LDADD = $(TESTUTIL_LIBS)
|
||||
lib1592_CPPFLAGS = $(AM_CPPFLAGS) -DLIB1592
|
||||
|
||||
lib1900_SOURCES = lib1900.c $(SUPPORTFILES) $(TESTUTIL) $(WARNLESS)
|
||||
lib1900_LDADD = $(TESTUTIL_LIBS)
|
||||
lib1900_CPPFLAGS = $(AM_CPPFLAGS)
|
||||
|
119
tests/libtest/lib1592.c
Normal file
119
tests/libtest/lib1592.c
Normal file
@ -0,0 +1,119 @@
|
||||
/***************************************************************************
|
||||
* _ _ ____ _
|
||||
* Project ___| | | | _ \| |
|
||||
* / __| | | | |_) | |
|
||||
* | (__| |_| | _ <| |___
|
||||
* \___|\___/|_| \_\_____|
|
||||
*
|
||||
* Copyright (C) 1998 - 2019, Daniel Stenberg, <daniel@haxx.se>, et al.
|
||||
*
|
||||
* This software is licensed as described in the file COPYING, which
|
||||
* you should have received as part of this distribution. The terms
|
||||
* are also available at https://curl.haxx.se/docs/copyright.html.
|
||||
*
|
||||
* You may opt to use, copy, modify, merge, publish, distribute and/or sell
|
||||
* copies of the Software, and permit persons to whom the Software is
|
||||
* furnished to do so, under the terms of the COPYING file.
|
||||
*
|
||||
* This software is distributed on an "AS IS" basis, WITHOUT WARRANTY OF ANY
|
||||
* KIND, either express or implied.
|
||||
*
|
||||
***************************************************************************/
|
||||
/*
|
||||
* See https://github.com/curl/curl/issues/3371
|
||||
*
|
||||
* This test case checks whether curl_multi_remove_handle() cancels
|
||||
* asynchronous DNS resolvers without blocking where possible. Obviously, it
|
||||
* only tests whichever resolver cURL is actually built with.
|
||||
*/
|
||||
|
||||
/* We're willing to wait a very generous two seconds for the removal. This is
|
||||
as low as we can go while still easily supporting SIGALRM timing for the
|
||||
non-threaded blocking resolver. It doesn't matter that much because when
|
||||
the test passes, we never wait this long. */
|
||||
#define TEST_HANG_TIMEOUT 2 * 1000
|
||||
|
||||
#include "test.h"
|
||||
#include "testutil.h"
|
||||
|
||||
#include <sys/stat.h>
|
||||
|
||||
int test(char *URL)
|
||||
{
|
||||
int stillRunning;
|
||||
CURLM *multiHandle = NULL;
|
||||
CURL *curl = NULL;
|
||||
CURLMcode res = CURLM_OK;
|
||||
int timeout;
|
||||
|
||||
global_init(CURL_GLOBAL_ALL);
|
||||
|
||||
multi_init(multiHandle);
|
||||
|
||||
easy_init(curl);
|
||||
|
||||
easy_setopt(curl, CURLOPT_VERBOSE, 1L);
|
||||
easy_setopt(curl, CURLOPT_URL, URL);
|
||||
|
||||
/* Set a DNS server that hopefully will not respond when using c-ares. */
|
||||
if(curl_easy_setopt(curl, CURLOPT_DNS_SERVERS, "0.0.0.0") == CURLE_OK)
|
||||
/* Since we could set the DNS server, presume we are working with a
|
||||
resolver that can be cancelled (i.e. c-ares). Thus,
|
||||
curl_multi_remove_handle() should not block even when the resolver
|
||||
request is outstanding. So, set a request timeout _longer_ than the
|
||||
test hang timeout so we will fail if the handle removal call incorrectly
|
||||
blocks. */
|
||||
timeout = TEST_HANG_TIMEOUT * 2;
|
||||
else {
|
||||
/* If we can't set the DNS server, presume that we are configured to use a
|
||||
resolver that can't be cancelled (i.e. the threaded resolver or the
|
||||
non-threaded blocking resolver). So, we just test that the
|
||||
curl_multi_remove_handle() call does finish well within our test
|
||||
timeout.
|
||||
|
||||
But, it is very unlikely that the resolver request will take any time at
|
||||
all because we haven't been able to configure the resolver to use an
|
||||
non-responsive DNS server. At least we exercise the flow.
|
||||
*/
|
||||
fprintf(stderr,
|
||||
"CURLOPT_DNS_SERVERS not supported; "
|
||||
"assuming curl_multi_remove_handle() will block\n");
|
||||
timeout = TEST_HANG_TIMEOUT / 2;
|
||||
}
|
||||
|
||||
/* Setting a timeout on the request should ensure that even if we have to
|
||||
wait for the resolver during curl_multi_remove_handle(), it won't take
|
||||
longer than this, because the resolver request inherits its timeout from
|
||||
this. */
|
||||
easy_setopt(curl, CURLOPT_TIMEOUT_MS, timeout);
|
||||
|
||||
multi_add_handle(multiHandle, curl);
|
||||
|
||||
/* This should move the handle from INIT => CONNECT => WAITRESOLVE. */
|
||||
fprintf(stderr, "curl_multi_perform()...\n");
|
||||
multi_perform(multiHandle, &stillRunning);
|
||||
fprintf(stderr, "curl_multi_perform() succeeded\n");
|
||||
|
||||
/* Start measuring how long it takes to remove the handle. */
|
||||
fprintf(stderr, "curl_multi_remove_handle()...\n");
|
||||
start_test_timing();
|
||||
res = curl_multi_remove_handle(multiHandle, curl);
|
||||
if(res) {
|
||||
fprintf(stderr, "curl_multi_remove_handle() failed, "
|
||||
"with code %d\n", (int)res);
|
||||
goto test_cleanup;
|
||||
}
|
||||
fprintf(stderr, "curl_multi_remove_handle() succeeded\n");
|
||||
|
||||
/* Fail the test if it took too long to remove. This happens after the fact,
|
||||
and says "it seems that it would have run forever", which isn't true, but
|
||||
it's close enough, and simple to do. */
|
||||
abort_on_test_timeout();
|
||||
|
||||
test_cleanup:
|
||||
curl_easy_cleanup(curl);
|
||||
curl_multi_cleanup(multiHandle);
|
||||
curl_global_cleanup();
|
||||
|
||||
return (int)res;
|
||||
}
|
Loading…
Reference in New Issue
Block a user