Message ID | 20210928162406.27205-4-attila@lendvai.name |
---|---|
State | New |
Headers | show |
Series | [bug#50814,1/5] tests: Smarten up git repository testing framework. | expand |
Context | Check | Description |
---|---|---|
cbaines/comparison | success | View comparision |
cbaines/git branch | success | View Git branch |
cbaines/applying patch | success | View Laminar job |
cbaines/issue | success | View issue |
cbaines/comparison | success | View comparision |
cbaines/git branch | success | View Git branch |
cbaines/applying patch | success | View Laminar job |
cbaines/issue | success | View issue |
cbaines/comparison | success | View comparision |
cbaines/git branch | success | View Git branch |
cbaines/applying patch | success | View Laminar job |
cbaines/issue | success | View issue |
cbaines/comparison | success | View comparision |
cbaines/git branch | success | View Git branch |
cbaines/applying patch | success | View Laminar job |
cbaines/issue | success | View issue |
cbaines/comparison | success | View comparision |
cbaines/git branch | success | View Git branch |
cbaines/applying patch | success | View Laminar job |
cbaines/issue | success | View issue |
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.
diff --git a/guix/diagnostics.scm b/guix/diagnostics.scm index 6a792febd4..343213fb45 100644 --- a/guix/diagnostics.scm +++ b/guix/diagnostics.scm @@ -48,6 +48,7 @@ formatted-message? formatted-message-string formatted-message-arguments + emit-formatted-warning &fix-hint fix-hint? @@ -161,6 +162,9 @@ messages." (report-error args ...) (exit 1))) +(define* (emit-formatted-warning fmt . args) + (emit-diagnostic fmt args #:prefix (G_ "warning: ") #:colors %warning-color)) + (define* (emit-diagnostic fmt args #:key location (colors (color)) (prefix "")) "Report diagnostic message FMT with the given ARGS and the specified diff --git a/guix/store.scm b/guix/store.scm index 89a719bcfc..33d4039037 100644 --- a/guix/store.scm +++ b/guix/store.scm @@ -45,6 +45,8 @@ #:use-module (srfi srfi-34) #:use-module (srfi srfi-35) #:use-module (srfi srfi-39) + #:use-module ((rnrs conditions) + #:select (warning?)) #:use-module (ice-9 match) #:use-module (ice-9 vlist) #:use-module (ice-9 popen) @@ -651,19 +653,21 @@ connection. Use with care." (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 '()))))) (define-syntax-rule (with-store store exp ...) "Bind STORE to an open connection to the store and evaluate EXPs; diff --git a/guix/ui.scm b/guix/ui.scm index 1428c254b3..88940f99ef 100644 --- a/guix/ui.scm +++ b/guix/ui.scm @@ -69,6 +69,8 @@ #:use-module (srfi srfi-31) #:use-module (srfi srfi-34) #:use-module (srfi srfi-35) + #:use-module ((rnrs conditions) + #:select (warning?)) #:autoload (ice-9 ftw) (scandir) #:use-module (ice-9 match) #:use-module (ice-9 format) @@ -689,7 +691,14 @@ evaluating the tests and bodies of CLAUSES." (and (not (port-closed? port)) (port-filename port))) - (guard* (c ((package-input-error? c) + (guard* (c ((warning? c) + (if (formatted-message? c) + (apply emit-formatted-warning + (formatted-message-string c) + (formatted-message-arguments c)) + (emit-formatted-warning "~a" c)) + '()) + ((package-input-error? c) (let* ((package (package-error-package c)) (input (package-error-invalid-input c)) (location (package-location package))