From acefdd0cd17443dae59921ae9f1245385e5ad4d0 Mon Sep 17 00:00:00 2001 From: Daniel Stenberg Date: Wed, 4 Jul 2018 14:47:10 +0200 Subject: [PATCH] multi: always do the COMPLETED procedure/state It was previously erroneously skipped in some situations. libtest/libntlmconnect.c wrongly depended on wrong behavior (that it would get a zero timeout) when no handles are "running" in a multi handle. That behavior is no longer present with this fix. Now libcurl will always return a -1 timeout when all handles are completed. Closes #2733 --- lib/multi.c | 53 +++++++++++++++++++--------------- tests/libtest/libntlmconnect.c | 15 ++++++---- 2 files changed, 40 insertions(+), 28 deletions(-) diff --git a/lib/multi.c b/lib/multi.c index decdbc443..70a406078 100644 --- a/lib/multi.c +++ b/lib/multi.c @@ -107,6 +107,16 @@ static const char * const statename[]={ /* function pointer called once when switching TO a state */ typedef void (*init_multistate_func)(struct Curl_easy *data); +static void Curl_init_completed(struct Curl_easy *data) +{ + /* this is a completed transfer */ + + /* Important: reset the conn pointer so that we don't point to memory + that could be freed anytime */ + data->easy_conn = NULL; + Curl_expire_clear(data); /* stop all timers */ +} + /* always use this function to change state, to make debugging easier */ static void mstate(struct Curl_easy *data, CURLMstate state #ifdef DEBUGBUILD @@ -116,17 +126,25 @@ static void mstate(struct Curl_easy *data, CURLMstate state { CURLMstate oldstate = data->mstate; static const init_multistate_func finit[CURLM_STATE_LAST] = { - NULL, - NULL, + NULL, /* INIT */ + NULL, /* CONNECT_PEND */ Curl_init_CONNECT, /* CONNECT */ - NULL, - NULL, - NULL, - NULL, - NULL, - NULL, - Curl_connect_free /* DO */ - /* the rest is NULL too */ + NULL, /* WAITRESOLVE */ + NULL, /* WAITCONNECT */ + NULL, /* WAITPROXYCONNECT */ + NULL, /* SENDPROTOCONNECT */ + NULL, /* PROTOCONNECT */ + NULL, /* WAITDO */ + Curl_connect_free, /* DO */ + NULL, /* DOING */ + NULL, /* DO_MORE */ + NULL, /* DO_DONE */ + NULL, /* WAITPERFORM */ + NULL, /* PERFORM */ + NULL, /* TOOFAST */ + NULL, /* DONE */ + Curl_init_completed, /* COMPLETED */ + NULL /* MSGSENT */ }; #if defined(DEBUGBUILD) && defined(CURL_DISABLE_VERBOSE_STRINGS) @@ -2062,16 +2080,6 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi, break; case CURLM_STATE_COMPLETED: - /* this is a completed transfer, it is likely to still be connected */ - - /* This node should be delinked from the list now and we should post - an information message that we are complete. */ - - /* Important: reset the conn pointer so that we don't point to memory - that could be freed anytime */ - data->easy_conn = NULL; - - Curl_expire_clear(data); /* stop all timers */ break; case CURLM_STATE_MSGSENT: @@ -2123,6 +2131,7 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi, } multistate(data, CURLM_STATE_COMPLETED); + rc = CURLM_CALL_MULTI_PERFORM; } /* if there's still a connection to use, call the progress function */ else if(data->easy_conn && Curl_pgrsUpdate(data->easy_conn)) { @@ -2147,14 +2156,12 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi, msg->extmsg.data.result = result; rc = multi_addmsg(multi, msg); - + DEBUGASSERT(!data->easy_conn); multistate(data, CURLM_STATE_MSGSENT); } } while((rc == CURLM_CALL_MULTI_PERFORM) || multi_ischanged(multi, FALSE)); data->result = result; - - return rc; } diff --git a/tests/libtest/libntlmconnect.c b/tests/libtest/libntlmconnect.c index 59f94b68a..519c5296e 100644 --- a/tests/libtest/libntlmconnect.c +++ b/tests/libtest/libntlmconnect.c @@ -5,7 +5,7 @@ * | (__| |_| | _ <| |___ * \___|\___/|_| \_\_____| * - * Copyright (C) 2012 - 2017, Daniel Stenberg, , et al. + * Copyright (C) 2012 - 2018, Daniel Stenberg, , et al. * * This software is licensed as described in the file COPYING, which * you should have received as part of this distribution. The terms @@ -158,6 +158,9 @@ int test(char *url) multi_perform(multi, &running); + fprintf(stderr, "%s:%d running %ld state %d\n", + __FILE__, __LINE__, running, state); + abort_on_test_timeout(); if(!running && state == NoMoreHandles) @@ -179,14 +182,16 @@ int test(char *url) } state = num_handles < MAX_EASY_HANDLES ? ReadyForNewHandle : NoMoreHandles; + fprintf(stderr, "%s:%d new state %d\n", + __FILE__, __LINE__, state); } multi_timeout(multi, &timeout); /* At this point, timeout is guaranteed to be greater or equal than -1. */ - fprintf(stderr, "%s:%d num_handles %d timeout %ld\n", - __FILE__, __LINE__, num_handles, timeout); + fprintf(stderr, "%s:%d num_handles %d timeout %ld running %d\n", + __FILE__, __LINE__, num_handles, timeout, running); if(timeout != -1L) { int itimeout = (timeout > (long)INT_MAX) ? INT_MAX : (int)timeout; @@ -194,8 +199,8 @@ int test(char *url) interval.tv_usec = (itimeout%1000)*1000; } else { - interval.tv_sec = TEST_HANG_TIMEOUT/1000 + 1; - interval.tv_usec = 0; + interval.tv_sec = 0; + interval.tv_usec = 5000; /* if there's no timeout and we get here on the last handle, we may already have read the last part of the stream so waiting makes no