unofficial mirror of guix-patches@gnu.org 
 help / color / mirror / Atom feed
* bug#26489: [PATCH] substitute: Ignore bad responses.
@ 2017-04-14  0:27 Tobias Geerinckx-Rice
  2017-04-14  9:54 ` Ludovic Courtès
  0 siblings, 1 reply; 8+ messages in thread
From: Tobias Geerinckx-Rice @ 2017-04-14  0:27 UTC (permalink / raw)
  To: 26489

* 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(-)

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* bug#26489: [PATCH] substitute: Ignore bad responses.
  2017-04-14  0:27 bug#26489: [PATCH] substitute: Ignore bad responses Tobias Geerinckx-Rice
@ 2017-04-14  9:54 ` Ludovic Courtès
  2017-04-28 20:56   ` Tobias Geerinckx-Rice
  0 siblings, 1 reply; 8+ messages in thread
From: Ludovic Courtès @ 2017-04-14  9:54 UTC (permalink / raw)
  To: Tobias Geerinckx-Rice; +Cc: 26489

Howdy!

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

> * 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’.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* bug#26489: [PATCH] substitute: Ignore bad responses.
  2017-04-14  9:54 ` Ludovic Courtès
@ 2017-04-28 20:56   ` Tobias Geerinckx-Rice
  2017-05-01 13:14     ` Ludovic Courtès
  0 siblings, 1 reply; 8+ messages in thread
From: Tobias Geerinckx-Rice @ 2017-04-28 20:56 UTC (permalink / raw)
  To: ludo; +Cc: 26489


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

Ludo',

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

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

Kind regards,

T G-R



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 504 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* bug#26489: [PATCH] substitute: Ignore bad responses.
  2017-04-28 20:56   ` Tobias Geerinckx-Rice
@ 2017-05-01 13:14     ` Ludovic Courtès
  2020-12-18 20:19       ` [bug#26489] " zimoun
  0 siblings, 1 reply; 8+ messages in thread
From: Ludovic Courtès @ 2017-05-01 13:14 UTC (permalink / raw)
  To: Tobias Geerinckx-Rice; +Cc: 26489

Hi!

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

> 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’.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [bug#26489] [PATCH] substitute: Ignore bad responses.
  2017-05-01 13:14     ` Ludovic Courtès
@ 2020-12-18 20:19       ` zimoun
  2020-12-20 13:39         ` Ludovic Courtès
  0 siblings, 1 reply; 8+ messages in thread
From: zimoun @ 2020-12-18 20:19 UTC (permalink / raw)
  To: Tobias Geerinckx-Rice; +Cc: 26489

Hi Tobias,

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

   <http://issues.guix.gnu.org/issue/26489>

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?


All the best,
simon




^ permalink raw reply	[flat|nested] 8+ messages in thread

* [bug#26489] [PATCH] substitute: Ignore bad responses.
  2020-12-18 20:19       ` [bug#26489] " zimoun
@ 2020-12-20 13:39         ` Ludovic Courtès
  2021-01-11 13:08           ` zimoun
  0 siblings, 1 reply; 8+ messages in thread
From: Ludovic Courtès @ 2020-12-20 13:39 UTC (permalink / raw)
  To: zimoun; +Cc: 26489

Hi,

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

> 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’.




^ permalink raw reply	[flat|nested] 8+ messages in thread

* [bug#26489] [PATCH] substitute: Ignore bad responses.
  2020-12-20 13:39         ` Ludovic Courtès
@ 2021-01-11 13:08           ` zimoun
  2021-01-15 19:52             ` Julien Lepiller
  0 siblings, 1 reply; 8+ messages in thread
From: zimoun @ 2021-01-11 13:08 UTC (permalink / raw)
  To: Ludovic Courtès; +Cc: 26489

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




^ permalink raw reply	[flat|nested] 8+ messages in thread

* [bug#26489] [PATCH] substitute: Ignore bad responses.
  2021-01-11 13:08           ` zimoun
@ 2021-01-15 19:52             ` Julien Lepiller
  0 siblings, 0 replies; 8+ messages in thread
From: Julien Lepiller @ 2021-01-15 19:52 UTC (permalink / raw)
  To: zimoun; +Cc: 26489

Le Mon, 11 Jan 2021 14:08:22 +0100,
zimoun <zimon.toutoune@gmail.com> a écrit :

> 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
https://issues.guix.gnu.org/45828




^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2021-01-15 19:53 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-14  0:27 bug#26489: [PATCH] substitute: Ignore bad responses Tobias Geerinckx-Rice
2017-04-14  9:54 ` Ludovic Courtès
2017-04-28 20:56   ` Tobias Geerinckx-Rice
2017-05-01 13:14     ` Ludovic Courtès
2020-12-18 20:19       ` [bug#26489] " zimoun
2020-12-20 13:39         ` Ludovic Courtès
2021-01-11 13:08           ` zimoun
2021-01-15 19:52             ` Julien Lepiller

unofficial mirror of guix-patches@gnu.org 

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://yhetil.org/guix-patches/1 guix-patches/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 guix-patches guix-patches/ https://yhetil.org/guix-patches \
		guix-patches@gnu.org
	public-inbox-index guix-patches

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://news.yhetil.org/yhetil.gnu.guix.patches


AGPL code for this site: git clone http://ou63pmih66umazou.onion/public-inbox.git