[bug#50814,4/5] guix: Prepare the UI for continuable &warning exceptions.

* guix/store.scm (call-with-store): Use dynamic-wind so that continuable
exceptions are not broken by being re-raised as non-continuable.  This is
needed for a later commit that uses continuable exceptions from within
git-authenticate to signal warnings to the user.  The reason for this is that
this way tests can explicitly check that a warning was signalled in certain
situations.
* guix/ui.scm (call-with-error-handling): Handle &warning type exceptions by
printing them to the user, and then continuing at the place they were
signalled at.
* guix/diagnostics.scm (emit-formatted-warning): New exported
function.
---
 guix/diagnostics.scm |  4 ++++
 guix/store.scm       | 16 ++++++++++------
 guix/ui.scm          | 11 ++++++++++-
 3 files changed, 24 insertions(+), 7 deletions(-)

Attila Lendvai schreef op di 28-09-2021 om 18:24 [+0200]:
>  (define (call-with-store proc)
>    "Call PROC with an open store connection."
> -  (let ((store (open-connection)))
> +  (let ((store '()))
>      (define (thunk)
>        (parameterize ((current-store-protocol-version
>                        (store-connection-version store)))
>          (call-with-values (lambda () (proc store))
>            (lambda results
> -            (close-connection store)
>              (apply values results)))))
>  
> -    (with-exception-handler (lambda (exception)
> -                              (close-connection store)
> -                              (raise-exception exception))
> -      thunk)))
> +    (dynamic-wind
> +      (lambda ()
> +        (set! store (open-connection)))
> +      thunk
> +      (lambda ()
> +        (close-connection store)
> +        (set! store '())))))

Do we really need to close and open the connection again every time
a continuation is made and resumed?  This seems inefficient if a threading
mechanism implemented by continuations is used (such as guile-fibers),
and there are two threads (‘fibers’) communicating and waiting with/for
each other in a loop, causing many ‘context switches’ (i.e., many captured
and resumed continuations).

Also note that a connection has some state: to the guix-daemon, it acts as
a GC root for everything built with the connection, and everything added to
the store (with add-to-store & friends) with that connection ... Simply
reconnecting isn't sufficient.

Greetings,
Maxime

> Do we really need to close and open the connection again every time
> a continuation is made and resumed? This seems inefficient if a threading
> mechanism implemented by continuations is used (such as guile-fibers),
> and there are two threads (‘fibers’) communicating and waiting with/for
> each other in a loop, causing many ‘context switches’ (i.e., many captured
> and resumed continuations).
>
> Also note that a connection has some state: to the guix-daemon, it acts as
> a GC root for everything built with the connection, and everything added to
> the store (with add-to-store & friends) with that connection ... Simply
> reconnecting isn't sufficient.

pardon my ignorance wrt dynamic-wind and call/cc, but does that^ mean
that 1) i should simply leave the wind part of the dynamic-wind empty
and move back the open-connection call into the let... or that 2) the
entire idea of replacing the exception handler with an unwind-protect
is flawed?

if 2) then i'll try to smarten up the handler to use raise-continuable
if the exception is of type &warning.

or any better ideas?

- attila
PGP: 5D5F 45C7 DFCD 0A39

Attila Lendvai schreef op wo 29-09-2021 om 14:50 [+0000]:
> > Do we really need to close and open the connection again every time
> > a continuation is made and resumed? This seems inefficient if a threading
> > mechanism implemented by continuations is used (such as guile-fibers),
> > and there are two threads (‘fibers’) communicating and waiting with/for
> > each other in a loop, causing many ‘context switches’ (i.e., many captured
> > and resumed continuations).
> > 
> > Also note that a connection has some state: to the guix-daemon, it acts as
> > a GC root for everything built with the connection, and everything added to
> > the store (with add-to-store & friends) with that connection ... Simply
> > reconnecting isn't sufficient.
> 
> pardon my ignorance wrt dynamic-wind and call/cc, but does that^ mean
> that 1) i should simply leave the wind part of the dynamic-wind empty
> and move back the open-connection call into the let... or that 2) the
> entire idea of replacing the exception handler with an unwind-protect
> is flawed?

About 1): which 'wind part' of dynamic-wind are you referring to?
The in-guard or the out-guard?

If the out-guard is empty, then the reference to the old connection will
be overwritten when the fiber is paused and resumed, so the old connection
will eventually be GC'ed, thus the daemon forgets some GC roots, leading
to a rare GC bug.

If the in-guard is empty, then the after pausing the fiber and resuming it,
the connection will be closed while the fiber might still need it.

> if 2) then i'll try to smarten up the handler to use raise-continuable
> if the exception is of type &warning.

That should work.  Or simpler: always use raise-continuable.

> or any better ideas?

Conventionally, to emit warnings, the procedure 'warning' from
(guix diagnostics) is used.  See e.g. (guix ci), (guix deprecation), (guix gexp),
(guix import ...), various modules under (guix scripts ...), (guix upstream) ...

Is there any reason not to use this pre-existing procedure?

Greetings,
Maxime

> About 1): which 'wind part' of dynamic-wind are you referring to?
>
> The in-guard or the out-guard?
>
> If the out-guard is empty, then the reference to the old connection will
> be overwritten when the fiber is paused and resumed, so the old connection
> will eventually be GC'ed, thus the daemon forgets some GC roots, leading
> to a rare GC bug.
>
> If the in-guard is empty, then the after pausing the fiber and resuming it,
> the connection will be closed while the fiber might still need it.


ok, so this is a no-go. thanks for the clarification!


> Conventionally, to emit warnings, the procedure 'warning' from
> (guix diagnostics) is used. See e.g. (guix ci), (guix deprecation), (guix gexp),
> (guix import ...), various modules under (guix scripts ...), (guix upstream) ...
>
> Is there any reason not to use this pre-existing procedure?


in a more advanced UI it might be a different story, but in the
current setup the only reason is to be able to assert for the warning
in the tests.

is that worth it? shall i just user WARNING and forget about the test?

- attila
PGP: 5D5F 45C7 DFCD 0A39

Attila Lendvai schreef op wo 29-09-2021 om 21:22 [+0000]:
> > 
[...]
> 
> > Conventionally, to emit warnings, the procedure 'warning' from
> > (guix diagnostics) is used. See e.g. (guix ci), (guix deprecation), (guix gexp),
> > (guix import ...), various modules under (guix scripts ...), (guix upstream) ...
> > 
> > Is there any reason not to use this pre-existing procedure?
> 
> in a more advanced UI it might be a different story, but in the
> current setup the only reason is to be able to assert for the warning
> in the tests.
> 
> is that worth it? shall i just user WARNING and forget about the test?

Testing a warning is emitted seems nice.  You could parameterise guix-warning-port
and use call-with-output-sting to capture the warning, and use (not (string-null? ...))
to verify a warning has been emitted.  From a quick grep, it appears
tests/transformations.scm and tests/substitute.scm are doing things like this.

Greetings,
Maxime.