[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
?
Your comment

This issue is archived.

To comment on this conversation send an email to 26489@debbugs.gnu.org

To respond to this issue using the mumi CLI, first switch to it
mumi current 26489
Then, you may apply the latest patchset in this issue (with sign off)
mumi am -- -s
Or, compose a reply to this issue
mumi compose
Or, send patches to this issue
mumi send-email *.patch