GNU bug report logs - #47160
[PATCH] scripts: substitute: Add back some error handling.

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 47160 in the body.
You can then email your comments to 47160 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: guix-patches <at> gnu.org
Subject: [PATCH] scripts: substitute: Add back some error handling.
Date: Mon, 15 Mar 2021 15:11:33 +0000

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

Message #8 received at 47160 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Christopher Baines <mail <at> cbaines.net>
Cc: 47160 <at> debbugs.gnu.org
Subject: Re: bug#47160: [PATCH] scripts: substitute: Add back some error
 handling.
Date: Mon, 15 Mar 2021 16:20:43 +0100

Hi,

Christopher Baines <mail <at> cbaines.net> skribis:

> 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.

[...]

> +  (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))))))

I think this should be at the top level for clarity.  It used to have
‘cached’ in its name because catching all these exceptions is something
you wouldn’t normally do; it only makes sense in the context of cached
connections.

>    (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))))))))

Does ‘call-with-connection-error-handling’ still make sense here?
There’s already ‘with-networking’ at the top level to do proper
networking error reporting.

Regarding <https://issues.guix.gnu.org/47157>, I would lean towards
perhaps reverting the connection/error-handling patch series and
starting anew from that “known state”.

This area is unfortunately quite tedious to test and to get right so I’d
err on the path of conservative, incremental changes.

Thought?

Ludo’.

Message #11 received at 47160 <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 47160 <at> debbugs.gnu.org
Subject: Re: bug#47160: [PATCH] scripts: substitute: Add back some error
 handling.
Date: Mon, 15 Mar 2021 16:15:00 +0000

[Message part 1 (text/plain, inline)]

Ludovic Courtès <ludo <at> gnu.org> writes:

> Hi,
>
> Christopher Baines <mail <at> cbaines.net> skribis:
>
>> 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.
>
> [...]
>
>> +  (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))))))
>
> I think this should be at the top level for clarity.  It used to have
> ‘cached’ in its name because catching all these exceptions is something
> you wouldn’t normally do; it only makes sense in the context of cached
> connections.

I initially tried to just put the error handling in just where it's
needed, but that was difficult since the http-fetch bit needs to happen
again when there's a relevant error.

The two things: getting a port which maybe is a cached connection and
handling some errors plus potentially re-running proc is difficult to
capture in a name, but "call-with-cached-connection-and-error-handling"
is an improvement over "with-cached-connection" I think.

>>    (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))))))))
>
> Does ‘call-with-connection-error-handling’ still make sense here?
> There’s already ‘with-networking’ at the top level to do proper
> networking error reporting.

So, looking back, the call-with-connection-error-handling error handling
was related to (call-)with-cached-connection, but it was only relevant
inside of fetch-narinfos, as that's when open-connection-for-uri/maybe
was passed in to call-with-cached-connection.

Which means no, I think it can be removed, at least that's more
consistent with the older behaviour.

I'll send some updated patches.

> Regarding <https://issues.guix.gnu.org/47157>, I would lean towards
> perhaps reverting the connection/error-handling patch series and
> starting anew from that “known state”.
>
> This area is unfortunately quite tedious to test and to get right so I’d
> err on the path of conservative, incremental changes.
>
> Thought?

My preference is still to try and move forward and to make the error
handling easier to see in the code.

Particularly with this change, I think the problem was introduced in
this commit [1], but I think it's hard to tell from the diff, since the
error handling and retrying is within with-cached-connection.

1: https://git.savannah.gnu.org/cgit/guix.git/commit/?id=f50f5751fff4cfc6d5abba9681054569694b7a5c

That commit was one of the commits where I was making small incremental
changes prior to actually getting to the changes I was looking at
making, but a breakage was still introduced.

What I was thinking about with this patch was how to make the error
handling being added back here easier to see, and thus harder to
break/remove.

[signature.asc (application/pgp-signature, inline)]

Message #14 received at 47160 <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: 47160 <at> debbugs.gnu.org
Subject: [PATCH v2 2/2] scripts: substitute: Tweak error reporting in
 process-substitution.
Date: Mon, 15 Mar 2021 16:15:32 +0000

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.
---
 guix/scripts/substitute.scm | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/guix/scripts/substitute.scm b/guix/scripts/substitute.scm
index 16ba28455f..997e2565e0 100755
--- a/guix/scripts/substitute.scm
+++ b/guix/scripts/substitute.scm
@@ -447,16 +447,13 @@ the current output port."
              (warning (G_ "while fetching ~a: server is somewhat slow~%")
                       (uri->string uri))
              (warning (G_ "try `--no-substitutes' if the problem persists~%")))
-           (call-with-connection-error-handling
+           (call-with-cached-connection-and-error-handling
             uri
-            (lambda ()
-              (call-with-cached-connection-and-error-handling
-               uri
-               (lambda (port)
-                 (http-fetch uri #:text? #f
-                             #:port port
-                             #:keep-alive? #t
-                             #:buffered? #f))))))))
+            (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

Message #17 received at 47160 <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: 47160 <at> debbugs.gnu.org
Subject: [PATCH v2 1/2] scripts: substitute: Add back some error handling.
Date: Mon, 15 Mar 2021 16:15:31 +0000

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,
(call-)with-cached-connection is back, rebranded as
call-with-cached-connection-and-error-handling.

* 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..16ba28455f 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))
@@ -377,6 +378,31 @@ server certificates."
                     (drain-input socket)
                     socket))))))))
 
+(define (call-with-cached-connection-and-error-handling 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* (process-substitution store-item destination
                                #:key cache-urls acl
                                deduplicate? print-build-trace?)
@@ -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-cached-connection-and-error-handling
+               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

Message #20 received at 47160 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Christopher Baines <mail <at> cbaines.net>
Cc: 47160 <at> debbugs.gnu.org
Subject: Re: bug#47160: [PATCH] scripts: substitute: Add back some error
 handling.
Date: Mon, 15 Mar 2021 21:51:02 +0100

Christopher Baines <mail <at> cbaines.net> skribis:

> Ludovic Courtès <ludo <at> gnu.org> writes:

[...]

>> Regarding <https://issues.guix.gnu.org/47157>, I would lean towards
>> perhaps reverting the connection/error-handling patch series and
>> starting anew from that “known state”.
>>
>> This area is unfortunately quite tedious to test and to get right so I’d
>> err on the path of conservative, incremental changes.
>>
>> Thought?
>
> My preference is still to try and move forward and to make the error
> handling easier to see in the code.
>
> Particularly with this change, I think the problem was introduced in
> this commit [1], but I think it's hard to tell from the diff, since the
> error handling and retrying is within with-cached-connection.
>
> 1: https://git.savannah.gnu.org/cgit/guix.git/commit/?id=f50f5751fff4cfc6d5abba9681054569694b7a5c
>
> That commit was one of the commits where I was making small incremental
> changes prior to actually getting to the changes I was looking at
> making, but a breakage was still introduced.
>
> What I was thinking about with this patch was how to make the error
> handling being added back here easier to see, and thus harder to
> break/remove.

OK.

Though I’m still unsure what the patch series starting at
7b812f7c84c43455cdd68a0e51b6ded018afcc8e was about.  What was the end
goal?

I also wonder if it introduced other issues.  For
example, 7b812f7c84c43455cdd68a0e51b6ded018afcc8e replaced a reference
to ‘open-connection-for-uri/cached’ by one to
‘open-connection-for-uri/maybe’.  Are we still using cached connections?

Commit f50f5751fff4cfc6d5abba9681054569694b7a5c no longer passes the
#:port parameter to ‘http-fetch’.

Commit 20c08a8a45d0f137ead7c05e720456b2aea44402 does other things but at
first sight I’m not sure what the effect is.

If you’re confident we can move forward to fix the bug, that’s great
(though we’ll need a good deal of testing), but I’d still like to
clarify these points later on.

Ludo’.

Message #23 received at 47160 <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 47160 <at> debbugs.gnu.org
Subject: Re: bug#47160: [PATCH] scripts: substitute: Add back some error
 handling.
Date: Mon, 15 Mar 2021 21:33:31 +0000

[Message part 1 (text/plain, inline)]

Ludovic Courtès <ludo <at> gnu.org> writes:

> Christopher Baines <mail <at> cbaines.net> skribis:
>
>> Ludovic Courtès <ludo <at> gnu.org> writes:
>
> [...]
>
>>> Regarding <https://issues.guix.gnu.org/47157>, I would lean towards
>>> perhaps reverting the connection/error-handling patch series and
>>> starting anew from that “known state”.
>>>
>>> This area is unfortunately quite tedious to test and to get right so I’d
>>> err on the path of conservative, incremental changes.
>>>
>>> Thought?
>>
>> My preference is still to try and move forward and to make the error
>> handling easier to see in the code.
>>
>> Particularly with this change, I think the problem was introduced in
>> this commit [1], but I think it's hard to tell from the diff, since the
>> error handling and retrying is within with-cached-connection.
>>
>> 1: https://git.savannah.gnu.org/cgit/guix.git/commit/?id=f50f5751fff4cfc6d5abba9681054569694b7a5c
>>
>> That commit was one of the commits where I was making small incremental
>> changes prior to actually getting to the changes I was looking at
>> making, but a breakage was still introduced.
>>
>> What I was thinking about with this patch was how to make the error
>> handling being added back here easier to see, and thus harder to
>> break/remove.
>
> OK.
>
> Though I’m still unsure what the patch series starting at
> 7b812f7c84c43455cdd68a0e51b6ded018afcc8e was about.  What was the end
> goal?

So that was part of the creation of the (guix substitutes) module,
unpicking the code in the script to separate out some of the connection
caching was a prerequisite (discussion starts here
https://issues.guix.gnu.org/45409#5 ).

I think separating out that module is still a good thing. It's allowed
for improvements in guix, the weather script doesn't now call in to the
substitute script code for example. I'd also like the separation for
things like the Guix Build Coordinator, which currently attempts to use
the substitute code from Guix.

> I also wonder if it introduced other issues.  For
> example, 7b812f7c84c43455cdd68a0e51b6ded018afcc8e replaced a reference
> to ‘open-connection-for-uri/cached’ by one to
> ‘open-connection-for-uri/maybe’.  Are we still using cached
> connections?

At least on that commit, open-connection-for-uri/maybe calls
open-connection-for-uri/cached, so yes, still using cached connections.

> Commit f50f5751fff4cfc6d5abba9681054569694b7a5c no longer passes the
> #:port parameter to ‘http-fetch’.

Yeah, that change is sort of fine if you're just looking at how the
port/connection is handled, but that area is being fixed up here, and
because closing the port is something that happens, it's better to also
pass the port in.

> Commit 20c08a8a45d0f137ead7c05e720456b2aea44402 does other things but at
> first sight I’m not sure what the effect is.

So, open-connection-for-uri/maybe is like
open-connection-for-uri/cached, but it catches a couple of exceptions
relating to not being able to connect to a substitute server, it also
remembers about showing the messages.

The second commit here is changing that slightly, to not apply to
process-substitution, however I do think that code might have applied in
the past (as open-connection-for-uri/maybe was used I believe). But I
think you're right in saying there's probably some overlap between the
error handling here and done by with-networking.

> If you’re confident we can move forward to fix the bug, that’s great
> (though we’ll need a good deal of testing), but I’d still like to
> clarify these points later on.

Well, the changes I'm suggesting here seem reasonable to me. As for
testing, checking things basically work is easy enough, but I don't
currently have many ideas for how to test for when fetching things
doesn't go to plan (which can of course happen).

[signature.asc (application/pgp-signature, inline)]

Message #26 received at 47160 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Christopher Baines <mail <at> cbaines.net>
Cc: 47160 <at> debbugs.gnu.org
Subject: Re: bug#47160: [PATCH] scripts: substitute: Add back some error
 handling.
Date: Tue, 16 Mar 2021 21:34:56 +0100

Howdy!

Christopher Baines <mail <at> cbaines.net> skribis:

> Ludovic Courtès <ludo <at> gnu.org> writes:

[...]

>> Though I’m still unsure what the patch series starting at
>> 7b812f7c84c43455cdd68a0e51b6ded018afcc8e was about.  What was the end
>> goal?
>
> So that was part of the creation of the (guix substitutes) module,
> unpicking the code in the script to separate out some of the connection
> caching was a prerequisite (discussion starts here
> https://issues.guix.gnu.org/45409#5 ).
>
> I think separating out that module is still a good thing. It's allowed
> for improvements in guix, the weather script doesn't now call in to the
> substitute script code for example. I'd also like the separation for
> things like the Guix Build Coordinator, which currently attempts to use
> the substitute code from Guix.

Right, I agree this is a worthy goal.  Untangling the stateful bits is
the hard part, as we see.  :-)

>> I also wonder if it introduced other issues.  For
>> example, 7b812f7c84c43455cdd68a0e51b6ded018afcc8e replaced a reference
>> to ‘open-connection-for-uri/cached’ by one to
>> ‘open-connection-for-uri/maybe’.  Are we still using cached
>> connections?
>
> At least on that commit, open-connection-for-uri/maybe calls
> open-connection-for-uri/cached, so yes, still using cached connections.

OK.

>> Commit f50f5751fff4cfc6d5abba9681054569694b7a5c no longer passes the
>> #:port parameter to ‘http-fetch’.
>
> Yeah, that change is sort of fine if you're just looking at how the
> port/connection is handled, but that area is being fixed up here, and
> because closing the port is something that happens, it's better to also
> pass the port in.

OK.

>> Commit 20c08a8a45d0f137ead7c05e720456b2aea44402 does other things but at
>> first sight I’m not sure what the effect is.
>
> So, open-connection-for-uri/maybe is like
> open-connection-for-uri/cached, but it catches a couple of exceptions
> relating to not being able to connect to a substitute server, it also
> remembers about showing the messages.
>
> The second commit here is changing that slightly, to not apply to
> process-substitution, however I do think that code might have applied in
> the past (as open-connection-for-uri/maybe was used I believe). But I
> think you're right in saying there's probably some overlap between the
> error handling here and done by with-networking.

Alright.

>> If you’re confident we can move forward to fix the bug, that’s great
>> (though we’ll need a good deal of testing), but I’d still like to
>> clarify these points later on.
>
> Well, the changes I'm suggesting here seem reasonable to me. As for
> testing, checking things basically work is easy enough, but I don't
> currently have many ideas for how to test for when fetching things
> doesn't go to plan (which can of course happen).

I’ll do some testing of v2 on my end and report back.

Thanks for the explanations!

Ludo’.

Message #29 received at 47160 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Christopher Baines <mail <at> cbaines.net>
Cc: 47160 <at> debbugs.gnu.org
Subject: Re: bug#47160: [PATCH] scripts: substitute: Add back some error
 handling.
Date: Tue, 16 Mar 2021 22:30:19 +0100

Christopher Baines <mail <at> cbaines.net> skribis:

> 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,
> (call-)with-cached-connection is back, rebranded as
> call-with-cached-connection-and-error-handling.
>
> * guix/scripts/substitute.scm (process-substitution): Retry once for some
> errors when making HTTP requests to fetch substitutes.

Please mention also the new procedure, and a
“Fixes <https://bugs.gnu.org/47157>.” line

> +(define (call-with-cached-connection-and-error-handling 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)))

I find it marginally clearer to pass #:fresh? #t (as was done in
the code removed in 7c85877fdf964694061e3192eac35723ebc047bf) than to
rely on the closed-port side effect.

I think it’s OK to remove ‘-and-error-handling’ because that doesn’t
really tell much and because too many words obscure the message IMO, but
that’s a detail.  I also like the helper macro as was removed in
7c85877fdf964694061e3192eac35723ebc047bf.

Apart from that LGTM.

My limited testing suggests it’s working as intended.

Thank you!

Ludo’.

Message #32 received at 47160 <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 47160 <at> debbugs.gnu.org
Subject: Re: bug#47160: [PATCH] scripts: substitute: Add back some error
 handling.
Date: Tue, 16 Mar 2021 23:11:40 +0000

[Message part 1 (text/plain, inline)]

Ludovic Courtès <ludo <at> gnu.org> writes:

> Christopher Baines <mail <at> cbaines.net> skribis:
>
>> 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,
>> (call-)with-cached-connection is back, rebranded as
>> call-with-cached-connection-and-error-handling.
>>
>> * guix/scripts/substitute.scm (process-substitution): Retry once for some
>> errors when making HTTP requests to fetch substitutes.
>
> Please mention also the new procedure, and a
> “Fixes <https://bugs.gnu.org/47157>.” line
>
>> +(define (call-with-cached-connection-and-error-handling 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)))
>
> I find it marginally clearer to pass #:fresh? #t (as was done in
> the code removed in 7c85877fdf964694061e3192eac35723ebc047bf) than to
> rely on the closed-port side effect.
>
> I think it’s OK to remove ‘-and-error-handling’ because that doesn’t
> really tell much and because too many words obscure the message IMO, but
> that’s a detail.  I also like the helper macro as was removed in
> 7c85877fdf964694061e3192eac35723ebc047bf.

Sure, I'll send a v3 set of patches shortly.

[signature.asc (application/pgp-signature, inline)]

Message #35 received at 47160 <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: 47160 <at> debbugs.gnu.org
Subject: [PATCH 2/2] scripts: substitute: Tweak error reporting in
 process-substitution.
Date: Tue, 16 Mar 2021 23:46:28 +0000

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.
---
 guix/scripts/substitute.scm | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/guix/scripts/substitute.scm b/guix/scripts/substitute.scm
index 812f2999ab..2bbbafe204 100755
--- a/guix/scripts/substitute.scm
+++ b/guix/scripts/substitute.scm
@@ -448,14 +448,11 @@ the current output port."
              (warning (G_ "while fetching ~a: server is somewhat slow~%")
                       (uri->string uri))
              (warning (G_ "try `--no-substitutes' if the problem persists~%")))
-           (call-with-connection-error-handling
-            uri
-            (lambda ()
-              (with-cached-connection uri port
-                (http-fetch uri #:text? #f
-                            #:port port
-                            #:keep-alive? #t
-                            #:buffered? #f)))))))
+           (with-cached-connection uri 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

Message #38 received at 47160 <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: 47160 <at> debbugs.gnu.org
Subject: [PATCH 1/2] scripts: substitute: Add back some error handling.
Date: Tue, 16 Mar 2021 23:46:27 +0000

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.
---
 guix/scripts/substitute.scm | 39 ++++++++++++++++++++++++++++++++-----
 1 file changed, 34 insertions(+), 5 deletions(-)

diff --git a/guix/scripts/substitute.scm b/guix/scripts/substitute.scm
index 6892aa999b..812f2999ab 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))
@@ -377,6 +378,32 @@ server certificates."
                     (drain-input socket)
                     socket))))))))
 
+(define (call-with-cached-connection uri proc)
+  (let ((port (open-connection-for-uri/cached uri
+                                              #:verify-certificate? #f)))
+    (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)))
+            (proc (open-connection-for-uri/cached uri
+                                                  #:verify-certificate? #f
+                                                  #:fresh? #t))
+            (apply throw key args))))))
+
+(define-syntax-rule (with-cached-connection uri port exp ...)
+  "Bind PORT with EXP... to a socket connected to URI."
+  (call-with-cached-connection uri (lambda (port) exp ...)))
+
 (define* (process-substitution store-item destination
                                #:key cache-urls acl
                                deduplicate? print-build-trace?)
@@ -424,11 +451,11 @@ 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))))))
+              (with-cached-connection uri port
+                (http-fetch uri #:text? #f
+                            #:port port
+                            #:keep-alive? #t
+                            #:buffered? #f)))))))
       (else
        (leave (G_ "unsupported substitute URI scheme: ~a~%")
               (uri->string uri)))))
@@ -715,6 +742,8 @@ if needed, as expected by the daemon's agent."
 ;;; Local Variables:
 ;;; eval: (put 'with-timeout 'scheme-indent-function 1)
 ;;; eval: (put 'with-redirected-error-port 'scheme-indent-function 0)
+;;; eval: (put 'with-cached-connection 'scheme-indent-function 2)
+;;; eval: (put 'call-with-cached-connection 'scheme-indent-function 1)
 ;;; End:
 
 ;;; substitute.scm ends here
-- 
2.30.1

Message #41 received at 47160 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Christopher Baines <mail <at> cbaines.net>
Cc: 47160 <at> debbugs.gnu.org
Subject: Re: bug#47160: [PATCH] scripts: substitute: Add back some error
 handling.
Date: Wed, 17 Mar 2021 21:18:13 +0100

Howdy!

Christopher Baines <mail <at> cbaines.net> 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!

Ludo’.

Message #46 received at 47160-done <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 47160-done <at> debbugs.gnu.org
Subject: Re: bug#47160: [PATCH] scripts: substitute: Add back some error
 handling.
Date: Wed, 17 Mar 2021 20:46:05 +0000

[Message part 1 (text/plain, inline)]

Ludovic Courtès <ludo <at> gnu.org> writes:

> Howdy!
>
> Christopher Baines <mail <at> cbaines.net> 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

[signature.asc (application/pgp-signature, inline)]

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.