Libguestfs - Feb 2023 - [PATCH nbdkit] curl: Try to share as much as possible between handles in the pool

On 2/22/23 16:01, Richard W.M. Jones wrote:
> Using the libcurl share interface we can share data between the
> separate curl easy handles in the pool.  For more about this see:
> 
> https://curl.se/libcurl/c/CURLSHOPT_SHARE.html
> https://gist.github.com/bagder/7eccf74f8b6d70b5abefeb7f288dba9b
> https://everything.curl.dev/libcurl/sharing
> ---
>  plugins/curl/curldefs.h |  3 +-
>  plugins/curl/curl.c     |  4 ++-
>  plugins/curl/pool.c     | 75 +++++++++++++++++++++++++++++++++++++++--
>  3 files changed, 78 insertions(+), 4 deletions(-)
> 
> diff --git a/plugins/curl/curldefs.h b/plugins/curl/curldefs.h
> index c2a3432fc..d614379d0 100644
> --- a/plugins/curl/curldefs.h
> +++ b/plugins/curl/curldefs.h
> @@ -117,9 +117,10 @@ struct curl_handle {
>  };
>  
>  /* pool.c */
> +extern void load_pool (void);
> +extern void unload_pool (void);
>  extern struct curl_handle *get_handle (void);
>  extern void put_handle (struct curl_handle *ch);
> -extern void free_all_handles (void);
>  
>  /* scripts.c */
>  extern int do_scripts (struct curl_handle *ch);
> diff --git a/plugins/curl/curl.c b/plugins/curl/curl.c
> index b5927b5b4..b8624a6f8 100644
> --- a/plugins/curl/curl.c
> +++ b/plugins/curl/curl.c
> @@ -101,6 +101,8 @@ curl_load (void)
>      nbdkit_error ("libcurl initialization failed: %d", (int) r);
>      exit (EXIT_FAILURE);
>    }
> +
> +  load_pool ();
>  }
>  
>  static void
> @@ -112,7 +114,7 @@ curl_unload (void)
>    free (password);
>    free (proxy_password);
>    scripts_unload ();
> -  free_all_handles ();
> +  unload_pool ();
>    curl_global_cleanup ();
>  }
>  
> diff --git a/plugins/curl/pool.c b/plugins/curl/pool.c
> index b6f4f8e5f..536123388 100644
> --- a/plugins/curl/pool.c
> +++ b/plugins/curl/pool.c
> @@ -52,6 +52,7 @@
>  
>  #include <nbdkit-plugin.h>
>  
> +#include "array-size.h"
>  #include "ascii-ctype.h"
>  #include "ascii-string.h"
>  #include "cleanup.h"
> @@ -73,6 +74,18 @@ static int debug_cb (CURL *handle, curl_infotype type,
>  static size_t header_cb (void *ptr, size_t size, size_t nmemb, void
*opaque);
>  static size_t write_cb (char *ptr, size_t size, size_t nmemb, void
*opaque);
>  static size_t read_cb (void *ptr, size_t size, size_t nmemb, void
*opaque);
> +static void lock_cb (CURL *handle, curl_lock_data data,
> +                     curl_lock_access access, void *userptr);
> +static void unlock_cb (CURL *handle, curl_lock_data data,
> +                       void *userptr);
> +
> +/* These locks protect access to the curl share data.  See:
> + * https://gist.github.com/bagder/7eccf74f8b6d70b5abefeb7f288dba9b
> + */
> +static pthread_rwlock_t lockarray[CURL_LOCK_DATA_LAST];
> +
> +/* Curl share data. */
> +static CURLSH *share;
>  
>  /* This lock protects access to the curl_handles vector below. */
>  static pthread_mutex_t lock = PTHREAD_MUTEX_INITIALIZER;
> @@ -90,18 +103,46 @@ static curl_handle_list curl_handles = empty_vector;
>  static pthread_cond_t cond = PTHREAD_COND_INITIALIZER;
>  static size_t in_use = 0, waiting = 0;
>  
> -/* Close and free all handles in the pool. */
> +/* Initialize pool structures. */
>  void
> -free_all_handles (void)
> +load_pool (void)
>  {
>    size_t i;
>  
> +  for (i = 0; i < ARRAY_SIZE (lockarray); ++i)
> +    pthread_rwlock_init (&lockarray[i], NULL);
error checking missing
> +
> +  share = curl_share_init ();
> +  if (share == NULL) {
> +    nbdkit_error ("curl_share_init: %m");
> +    exit (EXIT_FAILURE);
> +  }
> +  curl_share_setopt (share, CURLSHOPT_LOCKFUNC, lock_cb);
> +  curl_share_setopt (share, CURLSHOPT_UNLOCKFUNC, unlock_cb);
> +  curl_share_setopt (share, CURLSHOPT_SHARE, CURL_LOCK_DATA_CONNECT);
> +  curl_share_setopt (share, CURLSHOPT_SHARE, CURL_LOCK_DATA_COOKIE);
> +  curl_share_setopt (share, CURLSHOPT_SHARE, CURL_LOCK_DATA_DNS);
> +  curl_share_setopt (share, CURLSHOPT_SHARE, CURL_LOCK_DATA_SSL_SESSION);
error checking missing
> +}
> +
> +void
> +unload_pool (void)
> +{
> +  size_t i;
> +
> +  /* Close and free all handles in the pool. */
>    nbdkit_debug ("free_all_handles: number of curl handles allocated:
%zu",
>                  curl_handles.len);
>  
>    for (i = 0; i < curl_handles.len; ++i)
>      free_handle (curl_handles.ptr[i]);
>    curl_handle_list_reset (&curl_handles);
> +
> +  /* Free share data. */
> +  curl_share_cleanup (share);
> +
> +  for (i = 0; i < ARRAY_SIZE (lockarray); ++i)
> +    pthread_rwlock_destroy (&lockarray[i]);
>  }
>  
>  /* Get a handle from the pool.
> @@ -221,6 +262,9 @@ allocate_handle (void)
>      goto err;
>    }
>  
> +  /* Share settings with other handles. */
> +  curl_easy_setopt (ch->c, CURLOPT_SHARE, share);
> +
>    /* Various options we always set.
>     *
>     * NB: Both here and below constants must be explicitly long because
> @@ -519,3 +563,30 @@ free_handle (struct curl_handle *ch)
>      curl_slist_free_all (ch->headers_copy);
>    free (ch);
>  }
> +
> +static void
> +lock_cb (CURL *handle, curl_lock_data data, curl_lock_access access,
> +         void *userptr)
> +{
> +  assert (data < ARRAY_SIZE (lockarray));
> +
> +  switch (access) {
> +  case CURL_LOCK_ACCESS_SHARED:
> +    pthread_rwlock_rdlock (&lockarray[data]);
> +    break;
> +  case CURL_LOCK_ACCESS_SINGLE:
> +    pthread_rwlock_wrlock (&lockarray[data]);
> +    break;
we could at least assert the return values
> +  default:
> +    /* CURL_LOCK_ACCESS_NONE is never used in the current libcurl code. */
> +    abort ();
> +  }
> +}
> +
> +static void
> +unlock_cb (CURL *handle, curl_lock_data data, void *userptr)
> +{
> +  assert (data < ARRAY_SIZE (lockarray));
> +
> +  pthread_rwlock_unlock (&lockarray[data]);
same here
> +}
Seems sane to me in general, except for the fact that the documentation
at <https://curl.se/libcurl/c/CURLSHOPT_SHARE.html> writes:

"""
CURL_LOCK_DATA_CONNECT

Put the connection cache in the share object and make all easy handles
using this share object share the connection cache.

Note that due to a known bug, it is not safe to share connections this
way between multiple concurrent threads. [...]
"""

Ugh, what? If it's not safe to share the connection cache between
threads, then CURL_LOCK_DATA_CONNECT is effectively unusable, and no
connection pooling can be done. How does that *not* make this whole curl
facility useless?

The facility in general looks super weird; what sense does it make *not*
to share some particular CURL_LOCK_DATA_xxx?

Laszlo

On Wed, Feb 22, 2023 at 04:52:35PM +0100, Laszlo Ersek wrote:
...
> Seems sane to me in general, except for the fact that the documentation
> at <https://curl.se/libcurl/c/CURLSHOPT_SHARE.html> writes:
> 
> """
> CURL_LOCK_DATA_CONNECT
> 
> Put the connection cache in the share object and make all easy handles
> using this share object share the connection cache.
> 
> Note that due to a known bug, it is not safe to share connections this
> way between multiple concurrent threads. [...]
> """
> 
> Ugh, what? If it's not safe to share the connection cache between
> threads, then CURL_LOCK_DATA_CONNECT is effectively unusable, and no
> connection pooling can be done. How does that *not* make this whole curl
> facility useless?
>
> The facility in general looks super weird; what sense does it make *not*
> to share some particular CURL_LOCK_DATA_xxx?
I can only conclude this cannot be true.  Daniel Stenberg wrote this
code which definitely uses threads:

https://gist.github.com/bagder/7eccf74f8b6d70b5abefeb7f288dba9b

Also I did a lot of testing and didn't hit any obvious threading bugs.

Nevertheless I'm not planning to integrate this patch any time soon.
> Laszlo
Thanks, Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
nbdkit - Flexible, fast NBD server with plugins
https://gitlab.com/nbdkit/nbdkit