[PATCH] substitute: Ignore bad responses.

  • Done
  • quality assurance status badge
Details
4 participants
  • Julien Lepiller
  • Ludovic Courtès
  • Tobias Geerinckx-Rice
  • zimoun
Owner
unassigned
Submitted by
Tobias Geerinckx-Rice
Severity
normal
T
T
Tobias Geerinckx-Rice wrote on 14 Apr 2017 02:27
(address . guix-patches@gnu.org)
20170414002755.32672-1-me@tobias.gr
* guix/scripts/substitute.scm (http-multiple-get): Catch BAD-RESPONSE
exceptions and keep going.
---

Guix,

One weird HTTP response from a server will kill ‘guix substitute’:

updating list of substitutes from 'https://foo'... 50.0%Backtrace:
...
guix/ui.scm:1229:8: In procedure run-guix-command:
guix/ui.scm:1229:8: Throw to key `bad-response' with args
`("Bad Response-Line: ~s" (""))'.
error: build failed: substituter `substitute' died unexpectedly

Attached is a patch to ignore such bad responses. The offending .narinfo
will be ignored for that session, and not cached at all. The result:

updating list of substitutes from 'https://bar'... 100.0%
updating list of substitutes from 'https://foo'... 2.9% (bad response)
updating list of substitutes from 'https://foo'... 5.9% (bad response)

As a nice bonus, guix doesn't keel over and die.

Is this the best solution? A good one? Should it be made more obvious
that only READ-RESPONSE can throw, and that PROC will never be called
with, a bad response? No idea. I haven't had enough free time to learn
good the Guile like I'd so hoped to do at the beginning of the year. :c

Be gentle, dear reader, and all that,

T G-R

guix/scripts/substitute.scm | 30 ++++++++++++++++++------------
1 file changed, 18 insertions(+), 12 deletions(-)

Toggle diff (43 lines)
diff --git a/guix/scripts/substitute.scm b/guix/scripts/substitute.scm
index d3bccf4dd..7eccf9831 100755
--- a/guix/scripts/substitute.scm
+++ b/guix/scripts/substitute.scm
@@ -564,18 +564,24 @@ initial connection on which HTTP requests are sent."
(()
(reverse result))
((head tail ...)
- (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-connection p)
- (connect #f tail result)) ;try again
- (_
- (loop tail result)))))))))) ;keep going
+ (catch 'bad-response
+ (lambda ()
+ (let* ((resp (read-response p))
+ (body (response-body-port resp))
+ (result (proc head resp body result)))
+ ;; The server can stop responding at any time, in which case
+ ;; we have to try again. Check whether that's the case. Note
+ ;; that we can read from BODY even upon "Connection: close".
+ (match (assq 'connection (response-headers resp))
+ (('connection 'close)
+ (close-connection p)
+ (connect #f tail result)) ; try again
+ (_
+ (loop tail result))))) ; keep going
+ (lambda args
+ ;; This message appears on the same line as the progress report.
+ (format (current-error-port) " (bad response)~%")
+ (loop tail result))))))))) ; keep going
(define (read-to-eof port)
"Read from PORT until EOF is reached. The data are discarded."
--
2.12.2
L
L
Ludovic Courtès wrote on 14 Apr 2017 11:54
(name . Tobias Geerinckx-Rice)(address . me@tobias.gr)(address . 26489@debbugs.gnu.org)
87lgr3gvqf.fsf@gnu.org
Howdy!

Tobias Geerinckx-Rice <me@tobias.gr> skribis:

Toggle quote (18 lines)
> * guix/scripts/substitute.scm (http-multiple-get): Catch BAD-RESPONSE
> exceptions and keep going.
> ---
>
> Guix,
>
> One weird HTTP response from a server will kill ‘guix substitute’:
>
> updating list of substitutes from 'https://foo'... 50.0%Backtrace:
> ...
> guix/ui.scm:1229:8: In procedure run-guix-command:
> guix/ui.scm:1229:8: Throw to key `bad-response' with args
> `("Bad Response-Line: ~s" (""))'.
> error: build failed: substituter `substitute' died unexpectedly
>
> Attached is a patch to ignore such bad responses. The offending .narinfo
> will be ignored for that session, and not cached at all. The result:

I’m sure you expect this question: what bad responses did you get in
practice? :-)

Usually that is a sign of a broken HTTP server. Of course it’s
widespread enough, we’d better handle it, either in Guix or directly in
(web client) in Guile; OTOH, if it’s a genuine problem, we’d better not
hide it.

Thanks,
Ludo’.
T
T
Tobias Geerinckx-Rice wrote on 28 Apr 2017 22:56
(address . ludo@gnu.org)(address . 26489@debbugs.gnu.org)
ca4baf38-3b11-1c8a-a7b1-167f6500c247@tobias.gr
Ludo',

I should really send this message in my Drafts folder since 14/4... :-/

On 14/04/17 11:54, Ludovic Courtès wrote:
Toggle quote (16 lines)
> Tobias Geerinckx-Rice <me@tobias.gr> skribis:
>> One weird HTTP response from a server will kill ‘guix substitute’:
>>
>> updating list of substitutes from 'https://foo'... 50.0%Backtrace:
>> ... guix/ui.scm:1229:8: In procedure run-guix-command:
>> guix/ui.scm:1229:8: Throw to key `bad-response' with args `("Bad
>> Response-Line: ~s" (""))'. error: build failed: substituter
>> `substitute' died unexpectedly
>>
>> Attached is a patch to ignore such bad responses. The offending
>> .narinfo will be ignored for that session, and not cached at all.
>> The result:
>
> I’m sure you expect this question: what bad responses did you get in
> practice? :-)

In fact, not really. The error message looked unambiguous to me: the
HTTP response (the first line returned to the client, e.g. "HTTP/1.1 200
OK") was simply empty, throwing an exception.

Interestingly, a newline seems to be required.

Using http://bad.http.response.tobias.gras a substitute server triggers

Toggle quote (2 lines)
> Usually that is a sign of a broken HTTP server.

I think it's actually something in-between me and the server. I'll take
a closer look next time this happens.

Toggle quote (3 lines)
> Of course it’s widespread enough, we’d better handle it, either in
> Guix or directly in (web client) in Guile;

As I read it, (web client) considers throwing a BAD-RESPONSE exception
the best or only way to deal with an error like this. I agree.

Toggle quote (2 lines)
> OTOH, if it’s a genuine problem, we’d better not hide it.

Well, we don't hide it, per se. Hence the error message.

I think throwing an unhandled exception is definitely the wrong thing to
do here — this kills even ‘guix --keep-going --fallback’. I'm less sure
about the right place to do it

Kind regards,

T G-R
Attachment: signature.asc
L
L
Ludovic Courtès wrote on 1 May 2017 15:14
(name . Tobias Geerinckx-Rice)(address . me@tobias.gr)(address . 26489@debbugs.gnu.org)
87o9vc3eij.fsf@gnu.org
Hi!

Tobias Geerinckx-Rice <me@tobias.gr> skribis:

Toggle quote (45 lines)
> On 14/04/17 11:54, Ludovic Courtès wrote:
>> Tobias Geerinckx-Rice <me@tobias.gr> skribis:
>>> One weird HTTP response from a server will kill ‘guix substitute’:
>>>
>>> updating list of substitutes from 'https://foo'... 50.0%Backtrace:
>>> ... guix/ui.scm:1229:8: In procedure run-guix-command:
>>> guix/ui.scm:1229:8: Throw to key `bad-response' with args `("Bad
>>> Response-Line: ~s" (""))'. error: build failed: substituter
>>> `substitute' died unexpectedly
>>>
>>> Attached is a patch to ignore such bad responses. The offending
>>> .narinfo will be ignored for that session, and not cached at all.
>>> The result:
>>
>> I’m sure you expect this question: what bad responses did you get in
>> practice? :-)
>
> In fact, not really. The error message looked unambiguous to me: the
> HTTP response (the first line returned to the client, e.g. "HTTP/1.1 200
> OK") was simply empty, throwing an exception.
>
> Interestingly, a newline seems to be required.
>
> Using http://bad.http.response.tobias.gr as a substitute server triggers
> it. http://no.http.response.tobias.gr does not.
>
>> Usually that is a sign of a broken HTTP server.
>
> I think it's actually something in-between me and the server. I'll take
> a closer look next time this happens.
>
>> Of course it’s widespread enough, we’d better handle it, either in
>> Guix or directly in (web client) in Guile;
>
> As I read it, (web client) considers throwing a BAD-RESPONSE exception
> the best or only way to deal with an error like this. I agree.
>
>> OTOH, if it’s a genuine problem, we’d better not hide it.
>
> Well, we don't hide it, per se. Hence the error message.
>
> I think throwing an unhandled exception is definitely the wrong thing to
> do here — this kills even ‘guix --keep-going --fallback’. I'm less sure
> about the right place to do it

Oh right, if that kills --fallback, that’s a problem.

Back to your initial patch, what about moving ‘bad-response’ handling to
the call site of ‘http-multiple-get’ instead of having it in
‘http-multiple-get’? (That way, ‘http-multiple-get’ would behave like
‘http-get’ in this respect.)

Upon a ‘bad-response’, ‘fetch-narinfos’ would return #f or the empty
list or the partial narinfo list it has built so far.

WDYT?

I’d be happy with a patch along these lines!

Thank you,
Ludo’.
Z
Z
zimoun wrote on 18 Dec 2020 21:19
(name . Tobias Geerinckx-Rice)(address . me@tobias.gr)
861rfmvpdn.fsf@gmail.com
Hi Tobias,

Your patch #26489 sent a couple of years ago since lost in the vacuum of
the crazy Debbugs. Patch is here:


On Mon, 01 May 2017 at 15:14, ludo@gnu.org (Ludovic Courtès) wrote:

Toggle quote (18 lines)
>> I think throwing an unhandled exception is definitely the wrong thing to
>> do here — this kills even ‘guix --keep-going --fallback’. I'm less sure
>> about the right place to do it
>
> Oh right, if that kills --fallback, that’s a problem.
>
> Back to your initial patch, what about moving ‘bad-response’ handling to
> the call site of ‘http-multiple-get’ instead of having it in
> ‘http-multiple-get’? (That way, ‘http-multiple-get’ would behave like
> ‘http-get’ in this respect.)
>
> Upon a ‘bad-response’, ‘fetch-narinfos’ would return #f or the empty
> list or the partial narinfo list it has built so far.
>
> WDYT?
>
> I’d be happy with a patch along these lines!

Tobias, do you plan to rework/rebase it?


All the best,
simon
L
L
Ludovic Courtès wrote on 20 Dec 2020 14:39
(name . zimoun)(address . zimon.toutoune@gmail.com)
87o8io8ul7.fsf@gnu.org
Hi,

zimoun <zimon.toutoune@gmail.com> skribis:

Toggle quote (22 lines)
> On Mon, 01 May 2017 at 15:14, ludo@gnu.org (Ludovic Courtès) wrote:
>
>>> I think throwing an unhandled exception is definitely the wrong thing to
>>> do here — this kills even ‘guix --keep-going --fallback’. I'm less sure
>>> about the right place to do it
>>
>> Oh right, if that kills --fallback, that’s a problem.
>>
>> Back to your initial patch, what about moving ‘bad-response’ handling to
>> the call site of ‘http-multiple-get’ instead of having it in
>> ‘http-multiple-get’? (That way, ‘http-multiple-get’ would behave like
>> ‘http-get’ in this respect.)
>>
>> Upon a ‘bad-response’, ‘fetch-narinfos’ would return #f or the empty
>> list or the partial narinfo list it has built so far.
>>
>> WDYT?
>>
>> I’d be happy with a patch along these lines!
>
> Tobias, do you plan to rework/rebase it?

In the meantime, commit 5ff521452b9ec2aae9ed8e4bb7bdc250a581f203 added
‘bad-response’ handling (and more). We should take a closer look, but
it may be that this issue is now addressed.

Ludo’.
Z
Z
zimoun wrote on 11 Jan 2021 14:08
Re: [bug#26489] [PATCH] substitute: Ignore bad responses.
(name . Ludovic Courtès)(address . ludo@gnu.org)
861rertyd5.fsf@gmail.com
Hi Tobias,

On Sun, 20 Dec 2020 at 14:39, Ludovic Courtès <ludo@gnu.org> wrote:

Toggle quote (8 lines)
>>> I’d be happy with a patch along these lines!
>>
>> Tobias, do you plan to rework/rebase it?
>
> In the meantime, commit 5ff521452b9ec2aae9ed8e4bb7bdc250a581f203 added
> ‘bad-response’ handling (and more). We should take a closer look, but
> it may be that this issue is now addressed.

Since you reported the use-case and the patch, could you confirm that
the commit 5ff521 addresses the issue?

I think it does but maybe I am missing something; probably I am. :-)


Cheers,
simon
J
J
Julien Lepiller wrote on 15 Jan 2021 20:52
(name . zimoun)(address . zimon.toutoune@gmail.com)
20210115205224.7f775008@tachikoma.lepiller.eu
Le Mon, 11 Jan 2021 14:08:22 +0100,
zimoun <zimon.toutoune@gmail.com> a écrit :

Toggle quote (24 lines)
> Hi Tobias,
>
> On Sun, 20 Dec 2020 at 14:39, Ludovic Courtès <ludo@gnu.org> wrote:
>
> >>> I’d be happy with a patch along these lines!
> >>
> >> Tobias, do you plan to rework/rebase it?
> >
> > In the meantime, commit 5ff521452b9ec2aae9ed8e4bb7bdc250a581f203
> > added ‘bad-response’ handling (and more). We should take a closer
> > look, but it may be that this issue is now addressed.
>
> Since you reported the use-case and the patch, could you confirm that
> the commit 5ff521 addresses the issue?
>
> I think it does but maybe I am missing something; probably I am. :-)
>
>
> Cheers,
> simon
>
>
>

It seems not, since a few days ago we had exactly that problem. See
Z
Z
zimoun wrote on 9 Feb 2021 02:05
86v9b2jbj5.fsf@gmail.com
Hi,

On Fri, 15 Jan 2021 at 20:52, Julien Lepiller <julien@lepiller.eu> wrote:
Toggle quote (20 lines)
> Le Mon, 11 Jan 2021 14:08:22 +0100,
> zimoun <zimon.toutoune@gmail.com> a écrit :
>> On Sun, 20 Dec 2020 at 14:39, Ludovic Courtès <ludo@gnu.org> wrote:
>>
>> >>> I’d be happy with a patch along these lines!
>> >>
>> >> Tobias, do you plan to rework/rebase it?
>> >
>> > In the meantime, commit 5ff521452b9ec2aae9ed8e4bb7bdc250a581f203
>> > added ‘bad-response’ handling (and more). We should take a closer
>> > look, but it may be that this issue is now addressed.
>>
>> Since you reported the use-case and the patch, could you confirm that
>> the commit 5ff521 addresses the issue?
>>
>> I think it does but maybe I am missing something; probably I am. :-)
>
> It seems not, since a few days ago we had exactly that problem. See
> https://issues.guix.gnu.org/45828

Bug#45828 is now closed. I think it makes sense to close this one too.

If no objection, I am proposing to close this old bug. :-)


Cheers,
simon
L
L
Ludovic Courtès wrote on 9 Feb 2021 09:42
(name . zimoun)(address . zimon.toutoune@gmail.com)
87h7mlvdho.fsf@gnu.org
Hi,

zimoun <zimon.toutoune@gmail.com> skribis:

Toggle quote (25 lines)
> On Fri, 15 Jan 2021 at 20:52, Julien Lepiller <julien@lepiller.eu> wrote:
>> Le Mon, 11 Jan 2021 14:08:22 +0100,
>> zimoun <zimon.toutoune@gmail.com> a écrit :
>>> On Sun, 20 Dec 2020 at 14:39, Ludovic Courtès <ludo@gnu.org> wrote:
>>>
>>> >>> I’d be happy with a patch along these lines!
>>> >>
>>> >> Tobias, do you plan to rework/rebase it?
>>> >
>>> > In the meantime, commit 5ff521452b9ec2aae9ed8e4bb7bdc250a581f203
>>> > added ‘bad-response’ handling (and more). We should take a closer
>>> > look, but it may be that this issue is now addressed.
>>>
>>> Since you reported the use-case and the patch, could you confirm that
>>> the commit 5ff521 addresses the issue?
>>>
>>> I think it does but maybe I am missing something; probably I am. :-)
>>
>> It seems not, since a few days ago we had exactly that problem. See
>> https://issues.guix.gnu.org/45828
>
> Bug#45828 is now closed. I think it makes sense to close this one too.
>
> If no objection, I am proposing to close this old bug. :-)

Sounds good, done!

Ludo’.
Closed
?