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

[Christopher Baines <mail@cbaines.net>]

[-- Attachment #1.1: Type: text/plain, Size: 3664 bytes --]


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

> As discussed at <https://issues.guix.gnu.org/47283>, I’m still unsure
> these exceptions need to be caught within ‘http-multiple-get’ and at
> each iteration.
>
> Just minor comments:
>
> Christopher Baines <mail@cbaines.net> skribis:
>
>> +               (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)
>> +                        (list 'connect
>> +                              #f
>>                                (drop requests (+ 1 processed))
>>                                result))
>> -                   (apply throw key args))))))))))
>> +                       (_
>> +                        (list 'loop tail (+ 1 processed) result)))))
>> +                 (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))1
>> +                                (memq key
>> +                                      '(bad-response
>> +                                        bad-header
>> +                                        bad-header-component)))
>> +                       (begin
>> +                         (close-port p)
>> +                         (list 'connect
>> +                               #f
>> +                               requests
>> +                               result))
>> +                       (apply throw key args))))
>> +             (('connect . args)
>> +              (apply connect args))
>> +             (('loop . args)
>> +              (apply loop args)))))))))
>
> This is not new to this patch, but I think the whole exception handling
> bit should be factorized and written in such a way that
> ‘http-multiple-get’ still first on a horizontal screen (even though mine
> is actually vertical :-)).  Otherwise one might easily overlook the core
> logic of the function.

I've sent a v3 now, which makes a few changes to the original patch, and
includes a second patch for a potential refactoring.

I tested three variants for performance, http-multiple-get with no error
handling, the first v3 patch and the first and second v3 patches, and at
least with the test I'm using, the performance seems similar. Assuming
the performance is lower with error handling, the impact seems to be
within the margin of error, at least for test I was using.


[-- Attachment #1.2: http-multiple-get-perf.scm --]
[-- Type: text/plain, Size: 1406 bytes --]

(use-modules (ice-9 binary-ports)
             (srfi srfi-19)
             (web uri)
             (web request)
             (web response)
             (guix http-client))

(define (call-with-time-logging requests thunk)
  (let ((start   (current-time time-utc)))
    (call-with-values
        thunk
      (lambda vals
        (let* ((end     (current-time time-utc))
               (elapsed (time-difference end start)))
          (display
           (format #f
                   "~f seconds (~f microseconds per request)~%"
                   (+ (time-second elapsed)
                      (/ (time-nanosecond elapsed) 1e9))
                   (* (/ (+ (time-second elapsed)
                            (/ (time-nanosecond elapsed) 1e9))
                         requests)
                      1e6)))
          (apply values vals))))))

(define requests
  (map (lambda _
         (build-request (string->uri "http://localhost/does-not-exist")
                        #:method 'GET))
       (iota 200000)))

(call-with-time-logging
 (length requests)
 (lambda ()
   (http-multiple-get (string->uri "http://localhost/")
                      (lambda (request response port result)
                        (get-bytevector-n
                         port
                         (response-content-length response))
                        '())
                      '()
                      requests)))

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