| Message ID | 20220303211444.19928-3-ludo@gnu.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | 'github' importer gracefully handles rate limiting | 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 |
Ludovic Courtès schreef op do 03-03-2022 om 22:14 [+0100]: > Previously PORT would be closed unconditionally, which broke redirects > when #:keep-alive? #t is given. > > * guix/http-client.scm (http-fetch): Make 'port' a parameter of 'loop'. > Upon 3xx responses, do not close PORT is KEEP-ALIVE? is true, but consume > RESP's body. Add second argument to 'loop'. > --- HTTP things can become complicated, with lots of edge cases. Could an appropriate test be added to prevent this becoming buggy in the future, in case of future changed to 'http-fetch'? And a few source code comments about what is going on exactly? Greetings, Maxime.
Hello, I committed an updated version of these patches, taking some of your suggestions into account: a8d3033da6 import: github: Reuse HTTP connection for the /tags URL fallback. 8786c2e8d7 http-client: Correctly handle redirects when #:keep-alive? #t. 55e8e283ae import: github: Gracefully handle rate limit exhaustion. ecad9b2213 http-client: Add response headers to '&http-get-error'. 049aefddb2 tests: Add (guix http-client) tests. The first patch adds tests for ‘http-fetch’, as you suggested, which allowed me to find a thinko in the keep-alive patch. The test doesn’t test keep-alive support though, because the server in (guix tests http) doesn’t support it currently (I tried to retrofit it based on (web server http) but that’s not really possible because we’d need to either access internals of the <http> record type, specifically it’s poll set, or re-implement it entirely.) Thanks a lot for your help! Ludo’.
diff --git a/guix/http-client.scm b/guix/http-client.scm index 4b01e31165..4784497e5f 100644 --- a/guix/http-client.scm +++ b/guix/http-client.scm @@ -102,12 +102,12 @@ (define* (http-fetch uri #:key port (text? #f) (buffered? #t) Raise an '&http-get-error' condition if downloading fails." (let loop ((uri (if (string? uri) (string->uri uri) - uri))) - (let ((port (or port (open-connection uri - #:verify-certificate? - verify-certificate? - #:timeout timeout))) - (headers (match (uri-userinfo uri) + uri)) + (port (or port (open-connection uri + #:verify-certificate? + verify-certificate? + #:timeout timeout)))) + (let ((headers (match (uri-userinfo uri) ((? string? str) (cons (cons 'Authorization (string-append "Basic " @@ -131,11 +131,19 @@ (define* (http-fetch uri #:key port (text? #f) (buffered? #t) 303 ; see other 307 ; temporary redirection 308) ; permanent redirection - (let ((uri (resolve-uri-reference (response-location resp) uri))) - (close-port port) + (let ((host (uri-host uri)) + (uri (resolve-uri-reference (response-location resp) uri))) + (if keep-alive? + (dump-port data (%make-void-port "w0") + (response-content-length resp)) + (close-port port)) (format log-port (G_ "following redirection to `~a'...~%") (uri->string uri)) - (loop uri))) + (loop uri + (and keep-alive? + (or (not (uri-host uri)) + (string=? host (uri-host uri))) + port)))) (else (raise (condition (&http-get-error (uri uri)