* [RFC 1/3] http: warn on curl_multi_add_handle failures
2016-09-13 0:25 ` [RFC 0/3] http: avoid repeatedly adding curl easy to curlm Eric Wong
@ 2016-09-13 0:25 ` Eric Wong
2016-09-13 0:25 ` [RFC 2/3] http: consolidate #ifdefs for curl_multi_remove_handle Eric Wong
` (3 subsequent siblings)
4 siblings, 0 replies; 15+ messages in thread
From: Eric Wong @ 2016-09-13 0:25 UTC (permalink / raw)
To: Yaroslav Halchenko; +Cc: git, Jeff King, Junio C Hamano
This will be useful for tracking down curl usage errors.
Signed-off-by: Eric Wong <e@80x24.org>
---
http.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/http.c b/http.c
index cd40b01..cac5db9 100644
--- a/http.c
+++ b/http.c
@@ -1022,6 +1022,8 @@ int start_active_slot(struct active_request_slot *slot)
if (curlm_result != CURLM_OK &&
curlm_result != CURLM_CALL_MULTI_PERFORM) {
+ warning("curl_multi_add_handle failed: %s",
+ curl_multi_strerror(curlm_result));
active_requests--;
slot->in_use = 0;
return 0;
--
EW
^ permalink raw reply related [flat|nested] 15+ messages in thread* [RFC 2/3] http: consolidate #ifdefs for curl_multi_remove_handle
2016-09-13 0:25 ` [RFC 0/3] http: avoid repeatedly adding curl easy to curlm Eric Wong
2016-09-13 0:25 ` [RFC 1/3] http: warn on curl_multi_add_handle failures Eric Wong
@ 2016-09-13 0:25 ` Eric Wong
2016-09-13 0:25 ` [RFC 3/3] http: always remove curl easy from curlm session on release Eric Wong
` (2 subsequent siblings)
4 siblings, 0 replies; 15+ messages in thread
From: Eric Wong @ 2016-09-13 0:25 UTC (permalink / raw)
To: Yaroslav Halchenko; +Cc: git, Jeff King, Junio C Hamano
I find #ifdefs makes code difficult-to-follow.
An early version of this patch had error checking for
curl_multi_remove_handle calls, but caused some tests (e.g.
t5541) to fail under curl 7.26.0 on old Debian wheezy.
Signed-off-by: Eric Wong <e@80x24.org>
---
http.c | 17 ++++++++++-------
1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/http.c b/http.c
index cac5db9..a7eaf7b 100644
--- a/http.c
+++ b/http.c
@@ -201,6 +201,13 @@ static void finish_active_slot(struct active_request_slot *slot)
slot->callback_func(slot->callback_data);
}
+static void xmulti_remove_handle(struct active_request_slot *slot)
+{
+#ifdef USE_CURL_MULTI
+ curl_multi_remove_handle(curlm, slot->curl);
+#endif
+}
+
#ifdef USE_CURL_MULTI
static void process_curl_messages(void)
{
@@ -216,7 +223,7 @@ static void process_curl_messages(void)
slot->curl != curl_message->easy_handle)
slot = slot->next;
if (slot != NULL) {
- curl_multi_remove_handle(curlm, slot->curl);
+ xmulti_remove_handle(slot);
slot->curl_result = curl_result;
finish_active_slot(slot);
} else {
@@ -881,9 +888,7 @@ void http_cleanup(void)
while (slot != NULL) {
struct active_request_slot *next = slot->next;
if (slot->curl != NULL) {
-#ifdef USE_CURL_MULTI
- curl_multi_remove_handle(curlm, slot->curl);
-#endif
+ xmulti_remove_handle(slot);
curl_easy_cleanup(slot->curl);
}
free(slot);
@@ -1164,9 +1169,7 @@ static void release_active_slot(struct active_request_slot *slot)
{
closedown_active_slot(slot);
if (slot->curl && curl_session_count > min_curl_sessions) {
-#ifdef USE_CURL_MULTI
- curl_multi_remove_handle(curlm, slot->curl);
-#endif
+ xmulti_remove_handle(slot);
curl_easy_cleanup(slot->curl);
slot->curl = NULL;
curl_session_count--;
--
EW
^ permalink raw reply related [flat|nested] 15+ messages in thread* [RFC 3/3] http: always remove curl easy from curlm session on release
2016-09-13 0:25 ` [RFC 0/3] http: avoid repeatedly adding curl easy to curlm Eric Wong
2016-09-13 0:25 ` [RFC 1/3] http: warn on curl_multi_add_handle failures Eric Wong
2016-09-13 0:25 ` [RFC 2/3] http: consolidate #ifdefs for curl_multi_remove_handle Eric Wong
@ 2016-09-13 0:25 ` Eric Wong
2016-09-13 20:32 ` [RFC 0/3] http: avoid repeatedly adding curl easy to curlm Junio C Hamano
2016-09-14 11:59 ` [RFC 0/3] http: avoid repeatedly adding curl easy to curlm Yaroslav Halchenko
4 siblings, 0 replies; 15+ messages in thread
From: Eric Wong @ 2016-09-13 0:25 UTC (permalink / raw)
To: Yaroslav Halchenko; +Cc: git, Jeff King, Junio C Hamano
We must call curl_multi_remove_handle when releasing the slot to
prevent subsequent calls to curl_multi_add_handle from failing
with CURLM_ADDED_ALREADY (in curl 7.32.1+; older versions
returned CURLM_BAD_EASY_HANDLE)
Signed-off-by: Eric Wong <e@80x24.org>
---
http.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/http.c b/http.c
index a7eaf7b..3c4c3f5 100644
--- a/http.c
+++ b/http.c
@@ -1168,11 +1168,13 @@ void run_active_slot(struct active_request_slot *slot)
static void release_active_slot(struct active_request_slot *slot)
{
closedown_active_slot(slot);
- if (slot->curl && curl_session_count > min_curl_sessions) {
+ if (slot->curl) {
xmulti_remove_handle(slot);
- curl_easy_cleanup(slot->curl);
- slot->curl = NULL;
- curl_session_count--;
+ if (curl_session_count > min_curl_sessions) {
+ curl_easy_cleanup(slot->curl);
+ slot->curl = NULL;
+ curl_session_count--;
+ }
}
#ifdef USE_CURL_MULTI
fill_active_slots();
--
EW
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [RFC 0/3] http: avoid repeatedly adding curl easy to curlm
2016-09-13 0:25 ` [RFC 0/3] http: avoid repeatedly adding curl easy to curlm Eric Wong
` (2 preceding siblings ...)
2016-09-13 0:25 ` [RFC 3/3] http: always remove curl easy from curlm session on release Eric Wong
@ 2016-09-13 20:32 ` Junio C Hamano
2016-09-21 21:46 ` [PATCH 4/3] http: check curl_multi_remove_handle error code Eric Wong
2016-09-14 11:59 ` [RFC 0/3] http: avoid repeatedly adding curl easy to curlm Yaroslav Halchenko
4 siblings, 1 reply; 15+ messages in thread
From: Junio C Hamano @ 2016-09-13 20:32 UTC (permalink / raw)
To: Eric Wong; +Cc: Yaroslav Halchenko, git, Jeff King
Eric Wong <e@80x24.org> 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.
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH 4/3] http: check curl_multi_remove_handle error code
2016-09-13 20:32 ` [RFC 0/3] http: avoid repeatedly adding curl easy to curlm Junio C Hamano
@ 2016-09-21 21:46 ` Eric Wong
2016-09-21 22:22 ` Jeff King
0 siblings, 1 reply; 15+ messages in thread
From: Eric Wong @ 2016-09-21 21:46 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Yaroslav Halchenko, git, Jeff King
Junio C Hamano <gitster@pobox.com> wrote:
> Eric Wong <e@80x24.org> 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 <e@80x24.org>
---
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
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH 4/3] http: check curl_multi_remove_handle error code
2016-09-21 21:46 ` [PATCH 4/3] http: check curl_multi_remove_handle error code Eric Wong
@ 2016-09-21 22:22 ` Jeff King
2016-09-21 22:29 ` Eric Wong
0 siblings, 1 reply; 15+ messages in thread
From: Jeff King @ 2016-09-21 22:22 UTC (permalink / raw)
To: Eric Wong; +Cc: Junio C Hamano, Yaroslav Halchenko, git
On Wed, Sep 21, 2016 at 09:46:23PM +0000, 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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 4/3] http: check curl_multi_remove_handle error code
2016-09-21 22:22 ` Jeff King
@ 2016-09-21 22:29 ` Eric Wong
2016-09-21 22:31 ` Jeff King
0 siblings, 1 reply; 15+ messages in thread
From: Eric Wong @ 2016-09-21 22:29 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Yaroslav Halchenko, git
Jeff King <peff@peff.net> wrote:
> On Wed, Sep 21, 2016 at 09:46:23PM +0000, 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.
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 4/3] http: check curl_multi_remove_handle error code
2016-09-21 22:29 ` Eric Wong
@ 2016-09-21 22:31 ` Jeff King
0 siblings, 0 replies; 15+ messages in thread
From: Jeff King @ 2016-09-21 22:31 UTC (permalink / raw)
To: Eric Wong; +Cc: Junio C Hamano, Yaroslav Halchenko, git
On Wed, Sep 21, 2016 at 10:29:59PM +0000, Eric Wong wrote:
> Jeff King <peff@peff.net> wrote:
> > On Wed, Sep 21, 2016 at 09:46:23PM +0000, 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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC 0/3] http: avoid repeatedly adding curl easy to curlm
2016-09-13 0:25 ` [RFC 0/3] http: avoid repeatedly adding curl easy to curlm Eric Wong
` (3 preceding siblings ...)
2016-09-13 20:32 ` [RFC 0/3] http: avoid repeatedly adding curl easy to curlm Junio C Hamano
@ 2016-09-14 11:59 ` Yaroslav Halchenko
4 siblings, 0 replies; 15+ messages in thread
From: Yaroslav Halchenko @ 2016-09-14 11:59 UTC (permalink / raw)
To: Eric Wong; +Cc: git, Jeff King, Junio C Hamano
On Tue, 13 Sep 2016, Eric Wong wrote:
> What is unclear to me is how only Yaroslav's repository seems to
> trigger this bug after all these years...
Thank you Eric very much for tracking down this issue! Since issue is
intermittent, I guess people just didn't bother going through reporting
if they got an error once in a while, and/or could have attributed
to somehow misran update-server-info.
> However, I am fairly sure this fixes the bug Yaroslav
> encountered. This patch series is also needed for 2.9.3 and
> perhaps older maintenance tracks for distros.
FWIW I have tested your branch locally - and do not observe that bug any
longer. Thanks again!
--
Yaroslav O. Halchenko
Center for Open Neuroscience http://centerforopenneuroscience.org
Dartmouth College, 419 Moore Hall, Hinman Box 6207, Hanover, NH 03755
Phone: +1 (603) 646-9834 Fax: +1 (603) 646-1419
WWW: http://www.linkedin.com/in/yarik
^ permalink raw reply [flat|nested] 15+ messages in thread