‘guix package’ should lock the profile

  • Done
  • quality assurance status badge
Details
2 participants
  • Julien Lepiller
  • Ludovic Courtès
Owner
unassigned
Submitted by
Ludovic Courtès
Severity
normal
L
L
Ludovic Courtès wrote on 25 Jun 2019 16:10
‘guix package’ should lock the profile
(address . bug-Guix@gnu.org)(address . julien@lepiller.eu)
875zotn4c1.fsf@gnu.org
The article at
things like:

For instance, after installing Icecat, I installed a few other desktop
programs and then found Icecat had disappeared from my path again.

It’s likely that the person ran several ‘guix package’ commands in
parallel, and that one undoed the effects of the other.

Julien suggested that ‘guix package’ could grab a lock file, and I guess
it could simply error out when the lock is already taken.

Ludo’.
J
J
Julien Lepiller wrote on 25 Oct 2019 21:44
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . bug-Guix@gnu.org)
20191025214451.284c3530@sybil.lepiller.eu
Le Tue, 25 Jun 2019 16:10:22 +0200,
Ludovic Courtès <ludo@gnu.org> a écrit :

Toggle quote (16 lines)
> The article at
> <https://distrowatch.com/weekly.php?issue=20190624#guixsd> mentions
> things like:
>
> For instance, after installing Icecat, I installed a few other
> desktop programs and then found Icecat had disappeared from my path
> again.
>
> It’s likely that the person ran several ‘guix package’ commands in
> parallel, and that one undoed the effects of the other.
>
> Julien suggested that ‘guix package’ could grab a lock file, and I
> guess it could simply error out when the lock is already taken.
>
> Ludo’.

Hi!

attached is a patch for guix package to grab a lock file. Note that I'm
using flock, so it won't work on NFS shares. The other option would be
to use fcntl but guile doesn't seem to implement the locking function
from it.
From 987e9711f1fa6bfd270e48ee5624f69696e7e5c4 Mon Sep 17 00:00:00 2001
From: Julien Lepiller <julien@lepiller.eu>
Date: Fri, 25 Oct 2019 21:39:21 +0200
Subject: [PATCH] guix: package: lock profiles when processing them.

* guix/scripts/package.scm (process-actions): Get a per-profile lock to
prevent concurrent actions on profiles.
---
guix/scripts/package.scm | 17 +++++++++++++++--
1 file changed, 15 insertions(+), 2 deletions(-)

Toggle diff (37 lines)
diff --git a/guix/scripts/package.scm b/guix/scripts/package.scm
index 1a58d43e5c..e4f0f416f5 100644
--- a/guix/scripts/package.scm
+++ b/guix/scripts/package.scm
@@ -876,7 +876,17 @@ processed, #f otherwise."
(package-version item)
(manifest-entry-version entry))))))
- ;; First, process roll-backs, generation removals, etc.
+ ;; First, acquire a lock on the profile, to ensure only one guix process
+ ;; is modifying it at a time.
+ (define lock-file (open (string-append profile ".lock") O_CREAT))
+ (catch 'system-error
+ (lambda _
+ (flock lock-file (logior LOCK_EX LOCK_NB)))
+ (lambda (key . args)
+ (leave (G_ "profile ~a is being locked by another guix process.~%")
+ profile)))
+
+ ;; Then, process roll-backs, generation removals, etc.
(for-each (match-lambda
((key . arg)
(and=> (assoc-ref %actions key)
@@ -905,7 +915,10 @@ processed, #f otherwise."
#:allow-collisions? allow-collisions?
#:bootstrap? bootstrap?
#:use-substitutes? substitutes?
- #:dry-run? dry-run?))))
+ #:dry-run? dry-run?)))
+
+ ;; Finaly, close the lock file
+ (close lock-file))
;;;
--
2.22.0
L
L
Ludovic Courtès wrote on 25 Oct 2019 23:21
(name . Julien Lepiller)(address . julien@lepiller.eu)(address . 36375@debbugs.gnu.org)
87tv7wldwh.fsf@gnu.org
Hello,

Julien Lepiller <julien@lepiller.eu> skribis:

Toggle quote (5 lines)
> attached is a patch for guix package to grab a lock file. Note that I'm
> using flock, so it won't work on NFS shares. The other option would be
> to use fcntl but guile doesn't seem to implement the locking function
> from it.

(guix build syscalls) has it though, so you should probably use it. :-)

Toggle quote (8 lines)
> From 987e9711f1fa6bfd270e48ee5624f69696e7e5c4 Mon Sep 17 00:00:00 2001
> From: Julien Lepiller <julien@lepiller.eu>
> Date: Fri, 25 Oct 2019 21:39:21 +0200
> Subject: [PATCH] guix: package: lock profiles when processing them.
>
> * guix/scripts/package.scm (process-actions): Get a per-profile lock to
> prevent concurrent actions on profiles.

[...]

Toggle quote (11 lines)
> - ;; First, process roll-backs, generation removals, etc.
> + ;; First, acquire a lock on the profile, to ensure only one guix process
> + ;; is modifying it at a time.
> + (define lock-file (open (string-append profile ".lock") O_CREAT))
> + (catch 'system-error
> + (lambda _
> + (flock lock-file (logior LOCK_EX LOCK_NB)))
> + (lambda (key . args)
> + (leave (G_ "profile ~a is being locked by another guix process.~%")
> + profile)))

Nitpick: "profile ~a is locked by another process~%".

Toggle quote (14 lines)
> + ;; Then, process roll-backs, generation removals, etc.
> (for-each (match-lambda
> ((key . arg)
> (and=> (assoc-ref %actions key)
> @@ -905,7 +915,10 @@ processed, #f otherwise."
> #:allow-collisions? allow-collisions?
> #:bootstrap? bootstrap?
> #:use-substitutes? substitutes?
> - #:dry-run? dry-run?))))
> + #:dry-run? dry-run?)))
> +
> + ;; Finaly, close the lock file
> + (close lock-file))

I’d recommend wrapping the body in ‘with-file-lock’ (from (guix build
syscalls)), which handles non-local exits.

However you’d first need to add a #:wait? argument to ‘with-file-lock’
and perhaps an additional argument to handle the already-locked case.
Or maybe call that ‘with-file-lock/no-wait’.

How does that sound?

Thanks for working on it!

Ludo’.
J
J
Julien Lepiller wrote on 26 Oct 2019 00:08
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 36375@debbugs.gnu.org)
20191026000806.7eb6342b@sybil.lepiller.eu
Le Fri, 25 Oct 2019 23:21:34 +0200,
Ludovic Courtès <ludo@gnu.org> a écrit :
Toggle quote (14 lines)
>
> I’d recommend wrapping the body in ‘with-file-lock’ (from (guix build
> syscalls)), which handles non-local exits.
>
> However you’d first need to add a #:wait? argument to ‘with-file-lock’
> and perhaps an additional argument to handle the already-locked case.
> Or maybe call that ‘with-file-lock/no-wait’.
>
> How does that sound?
>
> Thanks for working on it!
>
> Ludo’.

Thanks! here is a new patch with your suggestions.
Attachment: 0001-guix-package-lock-profiles-when-processing-them.patch
L
L
Ludovic Courtès wrote on 6 Nov 2019 14:24
Re: bug#36375: [PATCH] Re: ‘guix package ’ should lock the profile
(name . Julien Lepiller)(address . julien@lepiller.eu)(address . 36375@debbugs.gnu.org)
87mud95e8p.fsf@gnu.org
Hello!

Julien Lepiller <julien@lepiller.eu> skribis:

Toggle quote (10 lines)
>>From 5d86226f318a111cc1bdf5a6f044c6f540f51b45 Mon Sep 17 00:00:00 2001
> From: Julien Lepiller <julien@lepiller.eu>
> Date: Fri, 25 Oct 2019 21:39:21 +0200
> Subject: [PATCH] guix: package: lock profiles when processing them.
>
> * guix/scripts/package.scm (process-actions): Get a per-profile lock to
> prevent concurrent actions on profiles.
> * guix/build/syscalls.scm (with-file-lock/no-wait): New procedure.
> (lock-file): Take a #:wait? key.

Nice! Could you make the syscalls.scm changes a separate patch?

Toggle quote (14 lines)
> +(define (call-with-file-lock/no-wait file thunk handler)
> + (let ((port (catch 'system-error
> + (lambda ()
> + (catch 'flock-error
> + (lambda ()
> + (lock-file file #:wait? #f))
> + handler))
> + (lambda args
> + ;; When using the statically-linked Guile in the initrd,
> + ;; 'fcntl-flock' returns ENOSYS unconditionally. Ignore
> + ;; that error since we're typically the only process running
> + ;; at this point.
> + (if (or (= ENOSYS (system-error-errno args)) (= 'flock-error args))

Please remove tabs. :-)

This is wrong because (1) ‘args’ is always a list, and (2) ‘=’ is
defined for numbers, not for symbols and lists.

I think you actually want to catch two exceptions here: ‘system-error’
and ‘flock-error’. For that, you have to do:

(catch #t
(lambda ()
(lock-file …))
(lambda (key . args)
(match key
('flock-error …)
('system-error
(if (= ENOSYS (system-error-errno (cons key args)))
…))
(_
(apply throw key args)))))
Does that make sense?

Toggle quote (5 lines)
> + ;; First, acquire a lock on the profile, to ensure only one guix process
> + ;; is modifying it at a time.
> + (with-file-lock/no-wait
> + (string-append profile ".lock")

Nitpick: I’d move the lock file name on the same line as
‘with-file-lock/no-wait’.

Toggle quote (4 lines)
> + (lambda (key . args)
> + (leave (G_ "profile ~a is locked by another guix process.~%")
> + profile))

s/guix// and remove the trailing period.

Could you add a test for that in tests/guix-package.sh?

One way to do it may be to do something like:

echo '(sleep 60) > /…/manifest.scm
guix package -m /…/manifest.scm -p whatever &
pid=$!
if guix install emacs -p whatever; then false; else true; fi
kill $pid

Could you send updated patches?

Thanks!

Ludo’.
J
J
Julien Lepiller wrote on 7 Nov 2019 22:19
Re: bug#36375: [PATCH] Re: ‘guix package’ should lock the profile
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 36375@debbugs.gnu.org)
20191107221936.1ff9bed6@sybil.lepiller.eu
Le Wed, 06 Nov 2019 14:24:54 +0100,
Ludovic Courtès <ludo@gnu.org> a écrit :

Toggle quote (88 lines)
> Hello!
>
> Julien Lepiller <julien@lepiller.eu> skribis:
>
> >>From 5d86226f318a111cc1bdf5a6f044c6f540f51b45 Mon Sep 17 00:00:00
> >>2001
> > From: Julien Lepiller <julien@lepiller.eu>
> > Date: Fri, 25 Oct 2019 21:39:21 +0200
> > Subject: [PATCH] guix: package: lock profiles when processing them.
> >
> > * guix/scripts/package.scm (process-actions): Get a per-profile
> > lock to prevent concurrent actions on profiles.
> > * guix/build/syscalls.scm (with-file-lock/no-wait): New procedure.
> > (lock-file): Take a #:wait? key.
>
> Nice! Could you make the syscalls.scm changes a separate patch?
>
> > +(define (call-with-file-lock/no-wait file thunk handler)
> > + (let ((port (catch 'system-error
> > + (lambda ()
> > + (catch 'flock-error
> > + (lambda ()
> > + (lock-file file #:wait? #f))
> > + handler))
> > + (lambda args
> > + ;; When using the statically-linked Guile in the
> > initrd,
> > + ;; 'fcntl-flock' returns ENOSYS
> > unconditionally. Ignore
> > + ;; that error since we're typically the only
> > process running
> > + ;; at this point.
> > + (if (or (= ENOSYS (system-error-errno args)) (=
> > 'flock-error args))
>
> Please remove tabs. :-)
>
> This is wrong because (1) ‘args’ is always a list, and (2) ‘=’ is
> defined for numbers, not for symbols and lists.
>
> I think you actually want to catch two exceptions here: ‘system-error’
> and ‘flock-error’. For that, you have to do:
>
> (catch #t
> (lambda ()
> (lock-file …))
> (lambda (key . args)
> (match key
> ('flock-error …)
> ('system-error
> (if (= ENOSYS (system-error-errno (cons key args)))
> …))
> (_
> (apply throw key args)))))
>
> Does that make sense?
>
> > + ;; First, acquire a lock on the profile, to ensure only one guix
> > process
> > + ;; is modifying it at a time.
> > + (with-file-lock/no-wait
> > + (string-append profile ".lock")
>
> Nitpick: I’d move the lock file name on the same line as
> ‘with-file-lock/no-wait’.
>
> > + (lambda (key . args)
> > + (leave (G_ "profile ~a is locked by another guix process.~%")
> > + profile))
>
> s/guix// and remove the trailing period.
>
> Could you add a test for that in tests/guix-package.sh?
>
> One way to do it may be to do something like:
>
> echo '(sleep 60) > /…/manifest.scm
> guix package -m /…/manifest.scm -p whatever &
> pid=$!
> if guix install emacs -p whatever; then false; else true; fi
> kill $pid
>
> Could you send updated patches?
>
> Thanks!
>
> Ludo’.

Attached are updated patches! I also made sure the new test passes.
From 71a85b5a8aac6c0bd5a1a4e3b52e409b2112df7a Mon Sep 17 00:00:00 2001
From: Julien Lepiller <julien@lepiller.eu>
Date: Thu, 7 Nov 2019 21:50:54 +0100
Subject: [PATCH 1/2] guix: Add file-locking with no wait.

* guix/build/syscalls.scm (with-file-lock/no-wait): New procedure.
(lock-file): Take a #:wait? key.
---
guix/build/syscalls.scm | 35 +++++++++++++++++++++++++++++++++--
1 file changed, 33 insertions(+), 2 deletions(-)

Toggle diff (68 lines)
diff --git a/guix/build/syscalls.scm b/guix/build/syscalls.scm
index bbf2531c79..a5a9c92a42 100644
--- a/guix/build/syscalls.scm
+++ b/guix/build/syscalls.scm
@@ -80,6 +80,7 @@
lock-file
unlock-file
with-file-lock
+ with-file-lock/no-wait
set-thread-name
thread-name
@@ -1087,10 +1088,10 @@ exception if it's already taken."
;; Presumably we got EAGAIN or so.
(throw 'flock-error err))))))
-(define (lock-file file)
+(define* (lock-file file #:key (wait? #t))
"Wait and acquire an exclusive lock on FILE. Return an open port."
(let ((port (open-file file "w0")))
- (fcntl-flock port 'write-lock)
+ (fcntl-flock port 'write-lock #:wait? wait?)
port))
(define (unlock-file port)
@@ -1119,10 +1120,40 @@ exception if it's already taken."
(when port
(unlock-file port))))))
+(define (call-with-file-lock/no-wait file thunk handler)
+ (let ((port (catch #t
+ (lambda ()
+ (lock-file file #:wait? #f))
+ (lambda (key . args)
+ (match key
+ ('flock-error
+ (handler args))
+ ('system-error
+ ;; When using the statically-linked Guile in the initrd,
+ ;; 'fcntl-flock' returns ENOSYS unconditionally. Ignore
+ ;; that error since we're typically the only process running
+ ;; at this point.
+ (if (= ENOSYS (system-error-errno (cons key args)))
+ #f
+ (apply throw args)))
+ (_ (apply throw key args)))))))
+ (dynamic-wind
+ (lambda ()
+ #t)
+ thunk
+ (lambda ()
+ (when port
+ (unlock-file port))))))
+
(define-syntax-rule (with-file-lock file exp ...)
"Wait to acquire a lock on FILE and evaluate EXP in that context."
(call-with-file-lock file (lambda () exp ...)))
+(define-syntax-rule (with-file-lock/no-wait file handler exp ...)
+ "Try to acquire a lock on FILE and evaluate EXP in that context. Execute
+handler if the lock is already held by another process."
+ (call-with-file-lock/no-wait file (lambda () exp ...) handler))
+
;;;
;;; Miscellaneous, aka. 'prctl'.
--
2.22.0
Attachment: 0002-guix-package-lock-profiles-when-processing-them.patch
L
L
Ludovic Courtès wrote on 8 Nov 2019 21:03
Re: bug#36375: [PATCH] Re: ‘guix package ’ should lock the profile
(name . Julien Lepiller)(address . julien@lepiller.eu)(address . 36375@debbugs.gnu.org)
871ruif85i.fsf@gnu.org
Hello!

Julien Lepiller <julien@lepiller.eu> skribis:

Toggle quote (8 lines)
> From 71a85b5a8aac6c0bd5a1a4e3b52e409b2112df7a Mon Sep 17 00:00:00 2001
> From: Julien Lepiller <julien@lepiller.eu>
> Date: Thu, 7 Nov 2019 21:50:54 +0100
> Subject: [PATCH 1/2] guix: Add file-locking with no wait.
>
> * guix/build/syscalls.scm (with-file-lock/no-wait): New procedure.
> (lock-file): Take a #:wait? key.

[...]

Toggle quote (9 lines)
> From 50c792e155d1207127f10ff0c0360442b7736a64 Mon Sep 17 00:00:00 2001
> From: Julien Lepiller <julien@lepiller.eu>
> Date: Fri, 25 Oct 2019 21:39:21 +0200
> Subject: [PATCH 2/2] guix: package: lock profiles when processing them.
>
> * guix/scripts/package.scm (process-actions): Get a per-profile lock to
> prevent concurrent actions on profiles.
> * tests/guix-package.sh: Add test.

LGTM!

I tested ‘with-file-lock’ on an NFSv3 mount, and ‘F_SETLKW’ is correctly
implemented, FWIW.

Thank you!

Ludo’.
J
J
Julien Lepiller wrote on 8 Nov 2019 22:03
Re: bug#36375: [PATCH] Re: ‘guix package’ should lock the profile
(address . 36375-done@debbugs.gnu.org)
20191108220304.7dd04d45@sybil.lepiller.eu
Le Fri, 08 Nov 2019 21:03:05 +0100,
Ludovic Courtès <ludo@gnu.org> a écrit :

Toggle quote (33 lines)
> Hello!
>
> Julien Lepiller <julien@lepiller.eu> skribis:
>
> > From 71a85b5a8aac6c0bd5a1a4e3b52e409b2112df7a Mon Sep 17 00:00:00
> > 2001 From: Julien Lepiller <julien@lepiller.eu>
> > Date: Thu, 7 Nov 2019 21:50:54 +0100
> > Subject: [PATCH 1/2] guix: Add file-locking with no wait.
> >
> > * guix/build/syscalls.scm (with-file-lock/no-wait): New procedure.
> > (lock-file): Take a #:wait? key.
>
> [...]
>
> > From 50c792e155d1207127f10ff0c0360442b7736a64 Mon Sep 17 00:00:00
> > 2001 From: Julien Lepiller <julien@lepiller.eu>
> > Date: Fri, 25 Oct 2019 21:39:21 +0200
> > Subject: [PATCH 2/2] guix: package: lock profiles when processing
> > them.
> >
> > * guix/scripts/package.scm (process-actions): Get a per-profile
> > lock to prevent concurrent actions on profiles.
> > * tests/guix-package.sh: Add test.
>
> LGTM!
>
> I tested ‘with-file-lock’ on an NFSv3 mount, and ‘F_SETLKW’ is
> correctly implemented, FWIW.
>
> Thank you!
>
> Ludo’.

Pushed as f49e9131889775a74a85c1f9b29f108030337b8b and
b1fb663404894268b5ee92c040f12c52c0bee425.
Closed
?