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

["Ludovic Courtès" <ludo@gnu.org>]

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.

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