Package: guix-patches;
Reported by: Christopher Baines <mail <at> cbaines.net>
Date: Sun, 21 Mar 2021 00:44:02 UTC
Severity: normal
Tags: patch
Done: Ludovic Courtès <ludo <at> gnu.org>
Bug is archived. No further changes may be made.
Message #32 received at 47288 <at> debbugs.gnu.org (full text, mbox):
From: Christopher Baines <mail <at> cbaines.net> To: Ludovic Courtès <ludo <at> gnu.org> Cc: 47288 <at> debbugs.gnu.org Subject: Re: bug#47288: [PATCH] guix: http-client: Tweak http-multiple-get error handling. Date: Fri, 26 Mar 2021 08:39:41 +0000
[Message part 1 (text/plain, inline)]
Ludovic Courtès <ludo <at> 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 <at> gnu.org>
> +;;; Copyright © 2012, 2013, 2014, 2015, 2016, 2017, 2018, 2020, 2021 Ludovic Courtès <ludo <at> gnu.org>
> ;;; Copyright © 2015 Mark H Weaver <mhw <at> netris.org>
> ;;; Copyright © 2012, 2015 Free Software Foundation, Inc.
> ;;; Copyright © 2017 Tobias Geerinckx-Rice <me <at> 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)))))))))
>
>
> ;;;
>
> 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
[signature.asc (application/pgp-signature, inline)]
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.