Message ID | 87r1k250za.fsf_-_@gnu.org |
---|---|
State | Accepted |
Headers | show |
Series | [bug#47288] guix: http-client: Tweak http-multiple-get error handling. | expand |
Context | Check | Description |
---|---|---|
cbaines/submitting builds | success | |
cbaines/comparison | success | View comparision |
cbaines/git branch | success | View Git branch |
cbaines/applying patch | success | View Laminar job |
cbaines/issue | success | View issue |
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))))))))) > > > ;;; > > 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
Hi, Christopher Baines <mail@cbaines.net> skribis: > Ludovic Courtès <ludo@gnu.org> writes: [...] >> + (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. Yes, but how is this different from a regular read(2) call as made by ‘get-bytevector-n’ or whatever? We wouldn’t write every read(2) call in ‘catch’ because in general any error there is indeed exceptional. I think the only bit that’s “less exceptional” here is that, because we’re reusing cached connection, we know that the first read(2) or the first write(2) to that port can trigger one of these errors—which, we know are not “exceptional”. Errors in subsequent read(2) or write(2) calls remain exceptional/unrecoverable and should be treated as such IMO. Does that make sense? (In that sense, I think wrapping every ‘read-response’ call rather than just the first one is already too much, but that’s okay.) >> + ;; 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. Oh, something to fix in a subsequent commit, then. All in all, I propose to go with this patch if that’s fine with you. Thanks! Ludo’.
Hi! I went ahead and pushed 45fce38fb0b6c6796906149ade145b8d3594c1c6 along these lines. ‘guix weather’ runs to completion and things seem to work fine. Let me know if you notice anything wrong. Thank you for your work and for your patience on this issue! Ludo’.
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))) + ;; 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)) + result))))))))) ;;;