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

[Christopher Baines]

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.

http-multiple-get-perf.scm
Description: Text document

signature.asc
Description: PGP signature