From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTP id 8F58F3857033 for ; Fri, 14 May 2021 22:51:25 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 8F58F3857033 Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-246-ob5vIxbYN6O1bVYlgmNFhQ-1; Fri, 14 May 2021 18:51:19 -0400 X-MC-Unique: ob5vIxbYN6O1bVYlgmNFhQ-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 867DF1009460 for ; Fri, 14 May 2021 22:51:18 +0000 (UTC) Received: from redhat.com (ovpn-112-75.phx2.redhat.com [10.3.112.75]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 5565C2BFF7 for ; Fri, 14 May 2021 22:51:18 +0000 (UTC) Received: from fche by redhat.com with local (Exim 4.94.2) (envelope-from ) id 1lhgeL-0006dv-34 for elfutils-devel@sourceware.org; Fri, 14 May 2021 18:51:17 -0400 Date: Fri, 14 May 2021 18:51:17 -0400 From: "Frank Ch. Eigler" To: elfutils-devel@sourceware.org Subject: PR27859 PATCH: correct 404-latch in connection reuse Message-ID: <20210514225117.GA23897@redhat.com> MIME-Version: 1.0 User-Agent: Mutt/1.12.0 (2019-05-25) X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-Spam-Status: No, score=-12.0 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_BADIPHTTP, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: elfutils-devel@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Elfutils-devel mailing list List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 14 May 2021 22:51:27 -0000 Hi - (I'll deploy this fix to one of the public servers to confirm it there.) Author: Frank Ch. Eigler Date: Fri May 14 18:37:30 2021 -0400 PR27859: correct 404-latch bug in debuginfod client reuse PR27701 implemented curl handle reuse in debuginfod_client objects, but with an unexpected bug. Server responses returning an error "latched" because the curl_easy handles for error cases weren't all systematically removed from the curl multi handle. This prevented their proper re-addition the next time. This version of the code simplfies matters by making only the curl curl_multi handle long-lived. This turns out to be enough, because it can maintain a pool of long-lived http/https connections and related data, and lend them out to short-lived curl_easy handles. This mode handles errors or hung downloads even better, because the easy handles don't undergo complex state transitions between reuse. A new test case confirms this correction via the federating debuginfod instance (cleaning caches between subtests to make sure http* is being used and reused). diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog index 249385b6a3f7..21407dc2e524 100644 --- a/debuginfod/ChangeLog +++ b/debuginfod/ChangeLog @@ -1,3 +1,11 @@ +2021-05-14 Frank Ch. Eigler + + PR27859 + * debuginfod-client.c (debuginfod_client): Retain only + long-lived multi handle from PR27701 work. + (debuginfo_begin,debuginfod_end): ctor/dtor for surviving field only. + (debuginfod_query_server): Rework to reuse multi handle only. + 2021-04-19 Martin Liska * debuginfod-client.c (debuginfod_query_server): Use startswith. diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c index cb51c2611796..ee7eda24df9f 100644 --- a/debuginfod/debuginfod-client.c +++ b/debuginfod/debuginfod-client.c @@ -119,9 +119,8 @@ struct debuginfod_client /* File descriptor to output any verbose messages if > 0. */ int verbose_fd; - /* Count DEBUGINFOD_URLS elements and corresponding curl handles. */ - int num_urls; - CURL **server_handles; + /* Maintain a long-lived curl multi-handle, which keeps a + connection/tls/dns cache to recently seen servers. */ CURLM *server_mhandle; /* Can contain all other context, like cache_path, server_urls, @@ -541,12 +540,6 @@ debuginfod_query_server (debuginfod_client *c, /* Is there any server we can query? If not, don't do any work, just return with ENOSYS. Don't even access the cache. */ - if (c->num_urls == 0) - { - rc = -ENOSYS; - goto out; - } - urls_envvar = getenv(DEBUGINFOD_URLS_ENV_VAR); if (vfd >= 0) dprintf (vfd, "server urls \"%s\"\n", @@ -770,13 +763,20 @@ debuginfod_query_server (debuginfod_client *c, goto out0; } + /* Count number of URLs. */ + int num_urls = 0; + for (int i = 0; server_urls[i] != '\0'; i++) + if (server_urls[i] != url_delim_char + && (i == 0 || server_urls[i - 1] == url_delim_char)) + num_urls++; + CURLM *curlm = c->server_mhandle; assert (curlm != NULL); /* Tracks which handle should write to fd. Set to the first handle that is ready to write the target file to the cache. */ CURL *target_handle = NULL; - struct handle_data *data = malloc(sizeof(struct handle_data) * c->num_urls); + struct handle_data *data = malloc(sizeof(struct handle_data) * num_urls); if (data == NULL) { rc = -ENOMEM; @@ -786,7 +786,7 @@ debuginfod_query_server (debuginfod_client *c, /* thereafter, goto out1 on error. */ /* Initialize handle_data with default values. */ - for (int i = 0; i < c->num_urls; i++) + for (int i = 0; i < num_urls; i++) { data[i].handle = NULL; data[i].fd = -1; @@ -797,23 +797,20 @@ debuginfod_query_server (debuginfod_client *c, char *server_url = strtok_r(server_urls, url_delim, &strtok_saveptr); /* Initialize each handle. */ - for (int i = 0; i < c->num_urls && server_url != NULL; i++) + for (int i = 0; i < num_urls && server_url != NULL; i++) { if (vfd >= 0) dprintf (vfd, "init server %d %s\n", i, server_url); data[i].fd = fd; data[i].target_handle = &target_handle; - data[i].handle = c->server_handles[i]; - assert (data[i].handle != NULL); - curl_easy_reset(data[i].handle); // esp. previously sent http headers - data[i].client = c; - + data[i].handle = curl_easy_init(); if (data[i].handle == NULL) { rc = -ENETUNREACH; goto out1; } + data[i].client = c; /* Build handle url. Tolerate both http://foo:999 and http://foo:999/ forms */ @@ -869,7 +866,7 @@ debuginfod_query_server (debuginfod_client *c, /* Query servers in parallel. */ if (vfd >= 0) - dprintf (vfd, "query %d urls in parallel\n", c->num_urls); + dprintf (vfd, "query %d urls in parallel\n", num_urls); int still_running; long loops = 0; int committed_to = -1; @@ -882,7 +879,7 @@ debuginfod_query_server (debuginfod_client *c, /* If the target file has been found, abort the other queries. */ if (target_handle != NULL) { - for (int i = 0; i < c->num_urls; i++) + for (int i = 0; i < num_urls; i++) if (data[i].handle != target_handle) curl_multi_remove_handle(curlm, data[i].handle); else @@ -979,7 +976,7 @@ debuginfod_query_server (debuginfod_client *c, curl_easy_strerror (msg->data.result)); if (pnl) c->default_progressfn_printed_p = 0; - for (int i = 0; i < c->num_urls; i++) + for (int i = 0; i < num_urls; i++) if (msg->easy_handle == data[i].handle) { if (strlen (data[i].errbuf) > 0) @@ -1111,8 +1108,13 @@ debuginfod_query_server (debuginfod_client *c, /* Perhaps we need not give up right away; could retry or something ... */ } - curl_multi_remove_handle(curlm, verified_handle); - assert (verified_handle == target_handle); + /* remove all handles from multi */ + for (int i = 0; i < num_urls; i++) + { + curl_multi_remove_handle(curlm, data[i].handle); /* ok to repeat */ + curl_easy_cleanup (data[i].handle); + } + free (data); free (server_urls); @@ -1126,6 +1128,13 @@ debuginfod_query_server (debuginfod_client *c, /* error exits */ out1: + /* remove all handles from multi */ + for (int i = 0; i < num_urls; i++) + { + curl_multi_remove_handle(curlm, data[i].handle); /* ok to repeat */ + curl_easy_cleanup (data[i].handle); + } + unlink (target_cache_tmppath); close (fd); /* before the rmdir, otherwise it'll fail */ (void) rmdir (target_cache_dir); /* nop if not empty */ @@ -1174,7 +1183,6 @@ debuginfod_begin (void) { debuginfod_client *client; size_t size = sizeof (struct debuginfod_client); - const char* server_urls = NULL; client = (debuginfod_client *) calloc (1, size); if (client != NULL) @@ -1187,45 +1195,15 @@ debuginfod_begin (void) client->verbose_fd = -1; } - /* Count the DEBUGINFOD_URLS and create the long-lived curl handles. */ - client->num_urls = 0; - server_urls = getenv (DEBUGINFOD_URLS_ENV_VAR); - if (server_urls != NULL) - for (int i = 0; server_urls[i] != '\0'; i++) - if (server_urls[i] != url_delim_char - && (i == 0 || server_urls[i - 1] == url_delim_char)) - client->num_urls++; - - client->server_handles = calloc (client->num_urls, sizeof(CURL *)); - if (client->server_handles == NULL) - goto out1; - - // allocate N curl easy handles - for (int i=0; inum_urls; i++) - { - client->server_handles[i] = curl_easy_init (); - if (client->server_handles[i] == NULL) - { - for (i--; i >= 0; i--) - curl_easy_cleanup (client->server_handles[i]); - goto out2; - } - } - // allocate 1 curl multi handle client->server_mhandle = curl_multi_init (); if (client->server_mhandle == NULL) - goto out3; + goto out1; + + // extra future initialization goto out; - out3: - for (int i=0; inum_urls; i++) - curl_easy_cleanup (client->server_handles[i]); - - out2: - free (client->server_handles); - out1: free (client); client = NULL; @@ -1259,10 +1237,6 @@ debuginfod_end (debuginfod_client *client) if (client == NULL) return; - // assume that all the easy handles have already been removed from the multi handle - for (int i=0; inum_urls; i++) - curl_easy_cleanup (client->server_handles[i]); - free (client->server_handles); curl_multi_cleanup (client->server_mhandle); curl_slist_free_all (client->headers); free (client->url); diff --git a/tests/ChangeLog b/tests/ChangeLog index 4951e14fae67..38e92659f117 100644 --- a/tests/ChangeLog +++ b/tests/ChangeLog @@ -1,3 +1,9 @@ +2021-05-14 Frank Ch. Eigler + + PR27859 + * run-debuginfod-find.sh: Test absence of 404-latch bug in client + curl handle reuse. + 2021-04-19 Martin Liska * dwelf_elf_e_machine_string.c (main): Use startswith. diff --git a/tests/run-debuginfod-find.sh b/tests/run-debuginfod-find.sh index 64b8290a119e..9183cccb7201 100755 --- a/tests/run-debuginfod-find.sh +++ b/tests/run-debuginfod-find.sh @@ -559,12 +559,24 @@ curl -s http://127.0.0.1:$PORT1/metrics | grep 'scanned_bytes_total' # And generate a few errors into the second debuginfod's logs, for analysis just below curl -s http://127.0.0.1:$PORT2/badapi > /dev/null || true -curl -s http://127.0.0.1:$PORT2/buildid/deadbeef/debuginfo > /dev/null || true +curl -s http://127.0.0.1:$PORT2/buildid/deadbeef/debuginfo > /dev/null || true +# NB: this error is used to seed the 404 failure for the survive-404 tests # Confirm bad artifact types are rejected without leaving trace curl -s http://127.0.0.1:$PORT2/buildid/deadbeef/badtype > /dev/null || true (curl -s http://127.0.0.1:$PORT2/metrics | grep 'badtype') && false +# Confirm that reused curl connections survive 404 errors. +# The rm's force an uncached fetch +rm -f $DEBUGINFOD_CACHE_PATH/$BUILDID/debuginfo .client_cache*/$BUILDID/debuginfo +testrun ${abs_top_builddir}/debuginfod/debuginfod-find debuginfo $BUILDID +rm -f $DEBUGINFOD_CACHE_PATH/$BUILDID/debuginfo .client_cache*/$BUILDID/debuginfo +testrun ${abs_top_builddir}/debuginfod/debuginfod-find debuginfo $BUILDID +testrun ${abs_top_builddir}/debuginfod/debuginfod-find debuginfo $BUILDID +testrun ${abs_top_builddir}/debuginfod/debuginfod-find debuginfo $BUILDID +rm -f $DEBUGINFOD_CACHE_PATH/$BUILDID/debuginfo .client_cache*/$BUILDID/debuginfo +testrun ${abs_top_builddir}/debuginfod/debuginfod-find debuginfo $BUILDID + # Confirm that some debuginfod client pools are being used curl -s http://127.0.0.1:$PORT2/metrics | grep 'dc_pool_op.*reuse'