[bug#47288] [PATCH] guix: http-client: Tweak http-multiple-get error handling.

[Christopher Baines <mail@cbaines.net>]

[-- Attachment #1: Type: text/plain, Size: 6200 bytes --]


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

> What about the approach below:
>
> diff --git a/guix/http-client.scm b/guix/http-client.scm
> index 4b4c14ed0b..6351e2d051 100644
> --- a/guix/http-client.scm
> +++ b/guix/http-client.scm
> @@ -1,5 +1,5 @@
>  ;;; GNU Guix --- Functional package management for GNU
> -;;; Copyright © 2012, 2013, 2014, 2015, 2016, 2017, 2018, 2020 Ludovic Courtès <ludo@gnu.org>
> +;;; Copyright © 2012, 2013, 2014, 2015, 2016, 2017, 2018, 2020, 2021 Ludovic Courtès <ludo@gnu.org>
>  ;;; Copyright © 2015 Mark H Weaver <mhw@netris.org>
>  ;;; Copyright © 2012, 2015 Free Software Foundation, Inc.
>  ;;; Copyright © 2017 Tobias Geerinckx-Rice <me@tobias.gr>
> @@ -147,6 +147,27 @@ Raise an '&http-get-error' condition if downloading fails."
>                                  (uri->string uri) code
>                                  (response-reason-phrase resp))))))))))))
>  
> +(define-syntax-rule (false-if-networking-error exp)
> +  "Return #f if EXP triggers a network related exception."
> +  ;; FIXME: Duplicated from 'with-cached-connection'.
> +  (catch #t
> +    (lambda ()
> +      exp)
> +    (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)))
> +          #f
> +          (apply throw key args)))))
> +
>  (define* (http-multiple-get base-uri proc seed requests
>                              #:key port (verify-certificate? #t)
>                              (open-connection guix:open-connection-for-uri)
> @@ -219,42 +240,27 @@ returning."
>               (remainder
>                (connect p remainder result))))
>            ((head tail ...)
> -           (catch #t
> -             (lambda ()
> -               (let* ((resp   (read-response p))
> -                      (body   (response-body-port resp))
> -                      (result (proc head resp body result)))
> -                 ;; The server can choose to stop responding at any time,
> -                 ;; in which case we have to try again.  Check whether
> -                 ;; that is the case.  Note that even upon "Connection:
> -                 ;; close", we can read from BODY.
> -                 (match (assq 'connection (response-headers resp))
> -                   (('connection 'close)
> -                    (close-port p)
> -                    (connect #f                       ;try again
> -                             (drop requests (+ 1 processed))
> -                             result))
> -                   (_
> -                    (loop tail (+ 1 processed) result))))) ;keep going
> -             (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 p)
> -                     (connect #f      ; try again
> -                              (drop requests (+ 1 processed))
> -                              result))
> -                   (apply throw key args))))))))))
> +           (match (false-if-networking-error (read-response p))
> +             ((? response? resp)
> +              (let* ((body   (response-body-port resp))
> +                     (result (proc head resp body result)))

Given body is a port, and that port is passed to proc, I'm guessing it's
possible for networking things to go wrong inside proc.

> +                ;; The server can choose to stop responding at any time,
> +                ;; in which case we have to try again.  Check whether
> +                ;; that is the case.  Note that even upon "Connection:
> +                ;; close", we can read from BODY.
> +                (match (assq 'connection (response-headers resp))
> +                  (('connection 'close)
> +                   (close-port p)
> +                   (connect #f                    ;try again
> +                            (drop requests (+ 1 processed))
> +                            result))
> +                  (_
> +                   (loop tail (+ 1 processed) result)))))
> +             (#f
> +              (close-port p)
> +              (connect #f                         ; try again
> +                       (drop requests (+ 1 processed))

I realised earlier in this series of patches that this should actually
be processed, rather than (+ 1 processed) since proc can't have been run
for the current response.

> +                       result)))))))))
>  
>  \f
>  ;;;
>
> I believe it’s a bit more readable because it moves ‘catch’ out of sight
> and avoids the sort of “mini DSL” where we return lists of arguments.
>
> WDYT?

It looks OK to me, I think the only thing to figure out for sure is
whether it's safe to not include the activity on the body port in the
error handling.

Thanks,

Chris

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 987 bytes --]