Re: [PATCH 4/3] http: check curl_multi_remove_handle error code

[Jeff King]

On Wed, Sep 21, 2016 at 10:29:59PM +, Eric Wong wrote:

> Jeff King  wrote:
> > On Wed, Sep 21, 2016 at 09:46:23PM +, Eric Wong wrote:
> > 
> > > ---8<---
> > > Subject: [PATCH] http: check curl_multi_remove_handle error code
> > > 
> > > This should help detect bugs in future changes.  While we're at
> > > it, fix a (probably innocuous) bug in our http_cleanup function
> > > for users of older curl.
> > > 
> > > curl_multi_remove_handle was not idempotent until curl 7.33.0
> > > with commit 84f3b3dd448399f9548468676e1bd1475dba8de5
> > > ("curl_multi_remove_handle: allow multiple removes"),
> > > so we track the "curlm" membership of the curl easy handle
> > > ourselves with a new "in_multi" flag.
> > 
> > Does curl provide a meaningful error here? I'm just wondering if we
> > could simply let curl handle this, and just ignore the error that comes
> > from older versions. We're basically just replicating curl's own state
> > data here.
> 
> curl before 7.33.0 returned CURLM_BAD_EASY_HANDLE.  This error
> code also happens if we pass a bad/corrupt easy handle;
> so it could be hiding an error on our end.

Hmm, yeah. So we would want to make it conditional on the version
anyway, so that we detected such bugs on more modern systems. That still
doesn't seem unreasonable to me, but your patch is also not very big, so
it's probably fine (the major risk with yours is simply that our state
and curl's state get out of sync).

-Peff

Re: [PATCH 4/3] http: check curl_multi_remove_handle error code

[Eric Wong]

Jeff King  wrote:
> On Wed, Sep 21, 2016 at 09:46:23PM +, Eric Wong wrote:
> 
> > ---8<---
> > Subject: [PATCH] http: check curl_multi_remove_handle error code
> > 
> > This should help detect bugs in future changes.  While we're at
> > it, fix a (probably innocuous) bug in our http_cleanup function
> > for users of older curl.
> > 
> > curl_multi_remove_handle was not idempotent until curl 7.33.0
> > with commit 84f3b3dd448399f9548468676e1bd1475dba8de5
> > ("curl_multi_remove_handle: allow multiple removes"),
> > so we track the "curlm" membership of the curl easy handle
> > ourselves with a new "in_multi" flag.
> 
> Does curl provide a meaningful error here? I'm just wondering if we
> could simply let curl handle this, and just ignore the error that comes
> from older versions. We're basically just replicating curl's own state
> data here.

curl before 7.33.0 returned CURLM_BAD_EASY_HANDLE.  This error
code also happens if we pass a bad/corrupt easy handle;
so it could be hiding an error on our end.

Re: [PATCH 4/3] http: check curl_multi_remove_handle error code

[Jeff King]

On Wed, Sep 21, 2016 at 09:46:23PM +, Eric Wong wrote:

> ---8<---
> Subject: [PATCH] http: check curl_multi_remove_handle error code
> 
> This should help detect bugs in future changes.  While we're at
> it, fix a (probably innocuous) bug in our http_cleanup function
> for users of older curl.
> 
> curl_multi_remove_handle was not idempotent until curl 7.33.0
> with commit 84f3b3dd448399f9548468676e1bd1475dba8de5
> ("curl_multi_remove_handle: allow multiple removes"),
> so we track the "curlm" membership of the curl easy handle
> ourselves with a new "in_multi" flag.

Does curl provide a meaningful error here? I'm just wondering if we
could simply let curl handle this, and just ignore the error that comes
from older versions. We're basically just replicating curl's own state
data here.

-Peff

[PATCH 4/3] http: check curl_multi_remove_handle error code

[Eric Wong]

Junio C Hamano  wrote:
> Eric Wong  writes:
> > The key patch here is 3/3 which seems like an obvious fix to
> > adding the problem of adding a curl easy handle to a curl multi
> > handle repeatedly.
> 
> Yeah, sounds like the right thing to do and 2/3 makes it really easy
> to read the resulting code.
> 
> > I will investigate those failures in a week or two when I regain
> > regular computer access.
> 
> Thanks. Will tentatively queue on 'pu' and wait for updates.

I'm comfortable with the original 3 patch series in 'next'
and being merged to 'master' and 'maint', soon.

I don't think the following 4/3 is strictly necessary now, so
I'd be more comfortable with it being tested in 'pu' or 'next'
for a longer period.

(online today, but not much tomorrow or another few days after)

---8<---
Subject: [PATCH] http: check curl_multi_remove_handle error code

This should help detect bugs in future changes.  While we're at
it, fix a (probably innocuous) bug in our http_cleanup function
for users of older curl.

curl_multi_remove_handle was not idempotent until curl 7.33.0
with commit 84f3b3dd448399f9548468676e1bd1475dba8de5
("curl_multi_remove_handle: allow multiple removes"),
so we track the "curlm" membership of the curl easy handle
ourselves with a new "in_multi" flag.

Tested with curl 7.26.0 and 7.38.0 on Debian 7.x (wheezy) and
Debian 8.x (jessie) respectively.

Signed-off-by: Eric Wong 
---
 http.c | 12 ++--
 http.h |  1 +
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/http.c b/http.c
index 82ed542..9f97749 100644
--- a/http.c
+++ b/http.c
@@ -204,7 +204,12 @@ static void finish_active_slot(struct active_request_slot 
*slot)
 static void xmulti_remove_handle(struct active_request_slot *slot)
 {
 #ifdef USE_CURL_MULTI
-   curl_multi_remove_handle(curlm, slot->curl);
+   CURLMcode code = curl_multi_remove_handle(curlm, slot->curl);
+
+   if (code != CURLM_OK)
+   die("curl_multi_remove_handle failed (%p): %s",
+   slot->curl, curl_multi_strerror(code));
+   slot->in_multi = 0;
 #endif
 }
 
@@ -888,7 +893,8 @@ void http_cleanup(void)
while (slot != NULL) {
struct active_request_slot *next = slot->next;
if (slot->curl != NULL) {
-   xmulti_remove_handle(slot);
+   if (slot->in_multi)
+   xmulti_remove_handle(slot);
curl_easy_cleanup(slot->curl);
}
free(slot);
@@ -965,6 +971,7 @@ struct active_request_slot *get_active_slot(void)
newslot = xmalloc(sizeof(*newslot));
newslot->curl = NULL;
newslot->in_use = 0;
+   newslot->in_multi = 0;
newslot->next = NULL;
 
slot = active_queue_head;
@@ -1033,6 +1040,7 @@ int start_active_slot(struct active_request_slot *slot)
slot->in_use = 0;
return 0;
}
+   slot->in_multi = 1;
 
/*
 * We know there must be something to do, since we just added
diff --git a/http.h b/http.h
index 5ab9d9c..3339d70 100644
--- a/http.h
+++ b/http.h
@@ -60,6 +60,7 @@ struct slot_results {
 struct active_request_slot {
CURL *curl;
int in_use;
+   int in_multi;
CURLcode curl_result;
long http_code;
int *finished;
-- 
EW