| Message ID | CAGE_+C5pnASOsrDr4ehNj-deYbSTr=pRgPcWqq5VSQs-Y08ttQ@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/5] remote-curl: avoid hang if curl asks for more data after eof | expand |
On Tue, Nov 14, 2023 at 07:34:55PM -0800, Jiří Hruška wrote: > `get_active_slot()` makes sure that the reused cURL handles it gives > out are as good as fresh ones, by resetting all options that other code > might have set on them back to defaults. > > But this does not apply to `CURLOPT_POSTFIELDSIZE_LARGE` yet, which can > stay set from a previous request. For example, an earlier probe request > with just a flush packet "0000" leaves it set to 4. > > The problem seems harmless in practice, but it can be confusing to see > a negative amount of remaining bytes to upload when inspecting libcurl > internals while debugging networking-related issues, for example. > > So reset also this option to its default value (which is -1, not 0). > > Signed-off-by: Jiri Hruska <jirka@fud.cz> > --- > http.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/http.c b/http.c > index 8f71bf00d8..14f2fbb82e 100644 > --- a/http.c > +++ b/http.c > @@ -1454,6 +1454,7 @@ struct active_request_slot *get_active_slot(void) > curl_easy_setopt(slot->curl, CURLOPT_READFUNCTION, NULL); > curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, NULL); > curl_easy_setopt(slot->curl, CURLOPT_POSTFIELDS, NULL); > + curl_easy_setopt(slot->curl, CURLOPT_POSTFIELDSIZE_LARGE, (curl_off_t)-1); > curl_easy_setopt(slot->curl, CURLOPT_UPLOAD, 0); > curl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1); > curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 1); It feels quite easy for this list to grow stale whenever we start to set a new option somewhere else. Is there a specific reason why we can't instead use `curl_easy_reset()` here? Quoting its description: > Re-initializes all options previously set on a specified CURL handle > to the default values. This puts back the handle to the same state as > it was in when it was just created with curl_easy_init. > > It does not change the following information kept in the handle: live > connections, the Session ID cache, the DNS cache, the cookies, the > shares or the alt-svc cache. From my naive point of view it sounds like exactly what we're after. Most of the code in question was introduced in 9094950d73 (http: prevent segfault during curl handle reuse, 2006-05-31), where we used to support libcurl at least back to v7.7. `curl_easy_reset()` on the other hand had only been introduced with v7.12.1 of libcurl, so maybe that's the reason why it's not used here? I dunno, might as well be that there is a good reason why we don't use it here. But if we can, then I'd argue it would be a great cleanup to convert to `curl_easy_reset()` here instead of piling onto the list of options. Patrick
> It feels quite easy for this list to grow stale whenever we start to set > a new option somewhere else. Is there a specific reason why we can't > instead use `curl_easy_reset()` here? Good point, makes sense. Thanks, Patrick, I will use that instead.
diff --git a/http.c b/http.c index 8f71bf00d8..14f2fbb82e 100644 --- a/http.c +++ b/http.c @@ -1454,6 +1454,7 @@ struct active_request_slot *get_active_slot(void) curl_easy_setopt(slot->curl, CURLOPT_READFUNCTION, NULL); curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, NULL); curl_easy_setopt(slot->curl, CURLOPT_POSTFIELDS, NULL); + curl_easy_setopt(slot->curl, CURLOPT_POSTFIELDSIZE_LARGE, (curl_off_t)-1); curl_easy_setopt(slot->curl, CURLOPT_UPLOAD, 0); curl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1); curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 1);
`get_active_slot()` makes sure that the reused cURL handles it gives out are as good as fresh ones, by resetting all options that other code might have set on them back to defaults. But this does not apply to `CURLOPT_POSTFIELDSIZE_LARGE` yet, which can stay set from a previous request. For example, an earlier probe request with just a flush packet "0000" leaves it set to 4. The problem seems harmless in practice, but it can be confusing to see a negative amount of remaining bytes to upload when inspecting libcurl internals while debugging networking-related issues, for example. So reset also this option to its default value (which is -1, not 0). Signed-off-by: Jiri Hruska <jirka@fud.cz> --- http.c | 1 + 1 file changed, 1 insertion(+)