Message ID | 20210315161532.1716-1-mail@cbaines.net |
---|---|
State | Accepted |
Headers | show |
Series | [bug#47160,v2,1/2] scripts: substitute: Add back some error handling. | expand |
Context | Check | Description |
---|---|---|
cbaines/comparison | success | View comparision |
cbaines/git branch | success | View Git branch |
cbaines/applying patch | success | View Laminar job |
cbaines/issue | success | View issue |
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, > (call-)with-cached-connection is back, rebranded as > call-with-cached-connection-and-error-handling. > > * guix/scripts/substitute.scm (process-substitution): Retry once for some > errors when making HTTP requests to fetch substitutes. Please mention also the new procedure, and a “Fixes <https://bugs.gnu.org/47157>.” line > +(define (call-with-cached-connection-and-error-handling 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))) I find it marginally clearer to pass #:fresh? #t (as was done in the code removed in 7c85877fdf964694061e3192eac35723ebc047bf) than to rely on the closed-port side effect. I think it’s OK to remove ‘-and-error-handling’ because that doesn’t really tell much and because too many words obscure the message IMO, but that’s a detail. I also like the helper macro as was removed in 7c85877fdf964694061e3192eac35723ebc047bf. Apart from that LGTM. My limited testing suggests it’s working as intended. Thank you! Ludo’.
Ludovic Courtès <ludo@gnu.org> writes: > 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, >> (call-)with-cached-connection is back, rebranded as >> call-with-cached-connection-and-error-handling. >> >> * guix/scripts/substitute.scm (process-substitution): Retry once for some >> errors when making HTTP requests to fetch substitutes. > > Please mention also the new procedure, and a > “Fixes <https://bugs.gnu.org/47157>.” line > >> +(define (call-with-cached-connection-and-error-handling 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))) > > I find it marginally clearer to pass #:fresh? #t (as was done in > the code removed in 7c85877fdf964694061e3192eac35723ebc047bf) than to > rely on the closed-port side effect. > > I think it’s OK to remove ‘-and-error-handling’ because that doesn’t > really tell much and because too many words obscure the message IMO, but > that’s a detail. I also like the helper macro as was removed in > 7c85877fdf964694061e3192eac35723ebc047bf. Sure, I'll send a v3 set of patches shortly.
diff --git a/guix/scripts/substitute.scm b/guix/scripts/substitute.scm index 6892aa999b..16ba28455f 100755 --- a/guix/scripts/substitute.scm +++ b/guix/scripts/substitute.scm @@ -45,6 +45,7 @@ #:select (uri-abbreviation nar-uri-abbreviation (open-connection-for-uri . guix:open-connection-for-uri))) + #:autoload (gnutls) (error/invalid-session) #:use-module (guix progress) #:use-module ((guix build syscalls) #:select (set-thread-name)) @@ -377,6 +378,31 @@ server certificates." (drain-input socket) socket)))))))) +(define (call-with-cached-connection-and-error-handling 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)))))) + (define* (process-substitution store-item destination #:key cache-urls acl deduplicate? print-build-trace?) @@ -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-cached-connection-and-error-handling + uri + (lambda (port) + (http-fetch uri #:text? #f + #:port port + #:keep-alive? #t + #:buffered? #f)))))))) (else (leave (G_ "unsupported substitute URI scheme: ~a~%") (uri->string uri)))))