PATCH: PR27701: debuginfod clients with long-lived curl handles

[Mark Wielaard]

Hi Frank,

On Fri, Apr 23, 2021 at 03:43:09PM -0400, Frank Ch. Eigler via Elfutils-devel wrote:
>     PR27701: debuginfod client: encourage reused debuginfod_client objects
>     
>     Client objects now carry long-lived curl handles for outgoing
>     connections.  This makes it more efficient for multiple sequential
>     queries, because the TCP connections and/or TLS state info are kept
>     around awhile, avoiding O(100ms) setup latencies.  debuginfod is
>     adjusted to take advantage of this for federation.  Other clients
>     should gradually do this too, perhaps including elfutils itself (in
>     the libdwfl->debuginfod_client hooks).
>     
>     A large gdb session with 117 debuginfo downloads was observed to run
>     twice as fast (45s vs. 1m30s wall-clock time), just in nuking this
>     extra setup latency.  This was tested via a debuginfod intermediary:
>     it should be even faster once gdb reuses its own debuginfod_client.

This is really nice. We should indeed also reuse a debuginfod_client
handle in libdwfl for a Dwfl or Dwfl_Module when we figure
out/document the multi-thread use case. The way debuginfod uses it is
multi-thread safe since it makes sure only one thread at a time uses
the same debuginfod_client handle.

The code looks good. I have only two comments:

> @@ -1095,6 +1087,11 @@ debuginfod_query_server (debuginfod_client *c,
>  
>  /* general purpose exit */
>   out:
> +  /* Reset sent headers */
> +  curl_slist_free_all (c->headers);
> +  c->headers = NULL;
> +  c->user_agent_set_p = 0;
> +  
>    /* Conclude the last \r status line */
>    /* Another possibility is to use the ANSI CSI n K EL "Erase in Line"
>       code.  That way, the previously printed messages would be erased,
[...]
> @@ -175,8 +177,8 @@ of the client object.
>  
>  .SS "HTTP HEADER"
>  
> -Before a lookup function is initiated, a client application may
> -add HTTP request headers to future downloads.
> +Before each lookup function is initiated, a client application may
> +add HTTP request headers.  These are reset after each lookup function.

Why do we need to reset the headers? Is that because we don't have
debuginfod_remove_http_header? Should we maybe add that (or a
reset_http_header)?

> @@ -90,10 +113,6 @@ wait_ready()
>    done;
>  
>    if [ $timeout -eq 0 ]; then
> -      echo "metric $what never changed to $value on port $port"

I think we want to keep this message ^ it helps with knowing which
exact metric timed out.

> -      curl -s http://127.0.0.1:$port/metrics
> -      echo "logs for debuginfod with port $port"
> -      cat vlog$port
>      exit 1;
>    fi
>  }

Cheers,

Mark