Subject: bug#47160: closed (Re: bug#47160: [PATCH] scripts: substitute: Add back some error handling.)

From: [email-masked] (GNU bug Tracking System)
Date: Wed, 17 Mar 2021 20:47:02 +0000

Your bug report

#47160: [PATCH] scripts: substitute: Add back some error handling.

which was filed against the guix-patches package, has been closed.

The explanation is attached below, along with your original report.
If you require more details, please reply to [email-masked].

-- 
47160: http://debbugs.gnu.org/cgi/bugreport.cgi?bug=47160
GNU Bug Tracking System
Contact [email-masked] with problems

Subject: Re: bug#47160: [PATCH] scripts: substitute: Add back some error handling.
From: Christopher Baines <[email-masked]>
To: Ludovic Courtès <[email-masked]>
Cc: [email-masked]
Date: Wed, 17 Mar 2021 20:46:05 +0000
List-Id: <debbugs-submit.debbugs.gnu.org>

Ludovic Courtès <[email-masked]> writes:

> Howdy!
>
> Christopher Baines <[email-masked]> skribis:
>
>> In f50f5751fff4cfc6d5abba9681054569694b7a5c, the way fetch was called within
>> process-substitution was changed.  As call-with-cached-connection actually
>> includes important error handling for the opening of a HTTP request, this
>> change removed some error handling.  This commit adds that back.
>>
>> Fixes <https://bugs.gnu.org/47157>.
>>
>> * guix/scripts/substitute.scm (call-with-cached-connection): New procedure.
>> (with-cached-connection): New syntax rule.
>> (process-substitution): Retry once for some errors when making HTTP requests
>> to fetch substitutes.
>
> [...]
>
>> The call-with-connection-error-handling was added in
>> 20c08a8a45d0f137ead7c05e720456b2aea44402, but that error handling was
>> previously inside of open-connection-for-uri/maybe, which is related
>> to (call-)with-cached-connection which was used in process-substitution, but
>> only actually used with call-with-cached-connection when used in
>> fetch-narinfos.
>>
>> There's some handling for similar errors within with-networking, which is used
>> within process-substitution.
>>
>> * guix/scripts/substitute.scm (process-substitution): Remove
>> call-with-connection-error-handling call.
>
> Both LGTM, thank you!

Great, pushed as b48204259aa9cad80c5b23a4060e2d796007ec7a.

Note that this won't have any affect on the substitute script for most
users until the guix package is updated to include these changes.

Chris

Subject: [PATCH] scripts: substitute: Add back some error handling.
From: Christopher Baines <[email-masked]>
To: [email-masked]
Date: Mon, 15 Mar 2021 15:11:33 +0000
List-Id: <debbugs-submit.debbugs.gnu.org>

In f50f5751fff4cfc6d5abba9681054569694b7a5c, the way fetch was called within
process-substitution was changed.  As with-cached-connection actually includes
important error handling for the opening of a HTTP request (when using a
cached connection), this change removed some error handling.

This commit adds that error handling back,
with-cached-connection/call-with-cached-connection is back, rebranded as
call-with-fresh-connection-retry.

* guix/scripts/substitute.scm (process-substitution): Retry once for some
errors when making HTTP requests to fetch substitutes.
---
 guix/scripts/substitute.scm | 38 ++++++++++++++++++++++++++++++++-----
 1 file changed, 33 insertions(+), 5 deletions(-)

diff --git a/guix/scripts/substitute.scm b/guix/scripts/substitute.scm
index 6892aa999b..2c9b45023f 100755
--- a/guix/scripts/substitute.scm
+++ b/guix/scripts/substitute.scm
@@ -45,6 +45,7 @@
                 #:select (uri-abbreviation nar-uri-abbreviation
                           (open-connection-for-uri
                            . guix:open-connection-for-uri)))
+  #:autoload   (gnutls) (error/invalid-session)
   #:use-module (guix progress)
   #:use-module ((guix build syscalls)
                 #:select (set-thread-name))
@@ -401,6 +402,31 @@ the current output port."
     (apply dump-file/deduplicate
            (append args (list #:store (%store-prefix)))))
 
+  (define (call-with-fresh-connection-retry uri proc)
+    (define (get-port)
+      (open-connection-for-uri/cached uri
+                                      #:verify-certificate? #f))
+
+    (let ((port (get-port)))
+      (catch #t
+        (lambda ()
+          (proc port))
+        (lambda (key . args)
+          ;; If PORT was cached and the server closed the connection in the
+          ;; meantime, we get EPIPE.  In that case, open a fresh connection
+          ;; and retry.  We might also get 'bad-response or a similar
+          ;; exception from (web response) later on, once we've sent the
+          ;; request, or a ERROR/INVALID-SESSION from GnuTLS.
+          (if (or (and (eq? key 'system-error)
+                       (= EPIPE (system-error-errno `(,key ,@args))))
+                  (and (eq? key 'gnutls-error)
+                       (eq? (first args) error/invalid-session))
+                  (memq key '(bad-response bad-header bad-header-component)))
+              (begin
+                (close-port port)       ; close the port to get a fresh one
+                (proc (get-port)))
+              (apply throw key args))))))
+
   (define (fetch uri)
     (case (uri-scheme uri)
       ((file)
@@ -424,11 +450,13 @@ the current output port."
            (call-with-connection-error-handling
             uri
             (lambda ()
-              (http-fetch uri #:text? #f
-                          #:open-connection open-connection-for-uri/cached
-                          #:keep-alive? #t
-                          #:buffered? #f
-                          #:verify-certificate? #f))))))
+              (call-with-fresh-connection-retry
+               uri
+               (lambda (port)
+                 (http-fetch uri #:text? #f
+                             #:port port
+                             #:keep-alive? #t
+                             #:buffered? #f))))))))
       (else
        (leave (G_ "unsupported substitute URI scheme: ~a~%")
               (uri->string uri)))))
-- 
2.30.1