On Wed, Apr 03, 2024 at 08:34:37AM +0200, Patrick Steinhardt wrote:

quoted

Can't we refactor this code to instead use `curl_easy_reset()`? That
function already resets most of the data we want to reset and would also
end up setting `POSFIELDSIZE = -1` via `Curl_init_userdefined()`. So
wouldn't the following be a more sensible fix?

[...]
Oh well, the answer is "no", or at least not as easily as this, as the
failing tests tell us. I guess it resets more data than we actually want
it to reset, but I didn't dig any deeper than that.

Yeah. The curl setup is really in two parts:

  1. we make a handle in get_curl_handle(), which also sets up a bunch
     of options. We use that to make a single "curl_default" handle, and
     then when we want a new handle we curl_easy_duphandle() that

  2. when we want to make a request we call get_active_slot(), which
     will either return an already-used handle or duphandle() a new one.
     And then reset some options, but also do some more setup.

Your patch touches spot (2), so it's erasing all of the setup done in
(1). I don't think there's a way to say "go back to state when we called
duphandle(), but keep reusing connections, etc".

Possibly it could be solved by pushing all of the setup from (1) into
(2). Though that would probably require separating out some config
handling, etc, from the actual curl_easy_setopt() calls (we wouldn't
want to complain about invalid config for _every_ request, for example,
just once per program run).

This is all also a bit more complicated than it needs to be for
smart-http. The dumb-http code may want to have several handles going at
once to do individual object/pack downloads. Whereas for smart http, we
really are only working with one handle, and doing one request at a
time. I don't know if we'll ever drop dumb-http support, but there would
probably be a lot of cleanup possibilities if we did.

-Peff