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

[Christopher Baines]

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

> Hi,
>
> Christopher Baines <mail@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
Description: PGP signature