GNU bug report logs - #36375
‘guix package’ should lock the profile

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 36375 in the body.
You can then email your comments to 36375 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: bug-Guix <at> gnu.org
Cc: julien <at> lepiller.eu
Subject: ‘guix package’ should lock the profile
Date: Tue, 25 Jun 2019 16:10:22 +0200

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

Message #8 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Julien Lepiller <julien <at> lepiller.eu>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: bug-Guix <at> gnu.org
Subject: [PATCH] Re: ‘guix package’ should lock the
 profile
Date: Fri, 25 Oct 2019 21:44:51 +0200

[Message part 1 (text/plain, inline)]

Le Tue, 25 Jun 2019 16:10:22 +0200,
Ludovic Courtès <ludo <at> gnu.org> a écrit :

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

[0001-guix-package-lock-profiles-when-processing-them.patch (text/x-patch, attachment)]

Message #11 received at 36375 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Julien Lepiller <julien <at> lepiller.eu>
Cc: 36375 <at> debbugs.gnu.org
Subject: Re: [PATCH] Re: ‘guix package’ should
 lock the profile
Date: Fri, 25 Oct 2019 23:21:34 +0200

Hello,

Julien Lepiller <julien <at> lepiller.eu> skribis:

> 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.  :-)

> From 987e9711f1fa6bfd270e48ee5624f69696e7e5c4 Mon Sep 17 00:00:00 2001
> From: Julien Lepiller <julien <at> 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.

[...]

> -  ;; 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~%".

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

Message #14 received at 36375 <at> debbugs.gnu.org (full text, mbox):

From: Julien Lepiller <julien <at> lepiller.eu>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 36375 <at> debbugs.gnu.org
Subject: Re: [PATCH] Re: ‘guix package’ should lock
 the profile
Date: Sat, 26 Oct 2019 00:08:06 +0200

[Message part 1 (text/plain, inline)]

Le Fri, 25 Oct 2019 23:21:34 +0200,
Ludovic Courtès <ludo <at> gnu.org> a écrit :
> 
> 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.

[0001-guix-package-lock-profiles-when-processing-them.patch (text/x-patch, attachment)]

Message #17 received at 36375 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Julien Lepiller <julien <at> lepiller.eu>
Cc: 36375 <at> debbugs.gnu.org
Subject: Re: bug#36375: [PATCH] Re: ‘guix package’ should lock the profile
Date: Wed, 06 Nov 2019 14:24:54 +0100

Hello!

Julien Lepiller <julien <at> lepiller.eu> skribis:

>>From 5d86226f318a111cc1bdf5a6f044c6f540f51b45 Mon Sep 17 00:00:00 2001
> From: Julien Lepiller <julien <at> 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’.

Message #20 received at 36375 <at> debbugs.gnu.org (full text, mbox):

From: Julien Lepiller <julien <at> lepiller.eu>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 36375 <at> debbugs.gnu.org
Subject: Re: bug#36375: [PATCH] Re: ‘guix package’
 should lock the profile
Date: Thu, 7 Nov 2019 22:19:36 +0100

[Message part 1 (text/plain, inline)]

Le Wed, 06 Nov 2019 14:24:54 +0100,
Ludovic Courtès <ludo <at> gnu.org> a écrit :

> Hello!
> 
> Julien Lepiller <julien <at> lepiller.eu> skribis:
> 
> >>From 5d86226f318a111cc1bdf5a6f044c6f540f51b45 Mon Sep 17 00:00:00
> >>2001  
> > From: Julien Lepiller <julien <at> 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.

[0001-guix-Add-file-locking-with-no-wait.patch (text/x-patch, attachment)]

[0002-guix-package-lock-profiles-when-processing-them.patch (text/x-patch, attachment)]

Message #23 received at 36375 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Julien Lepiller <julien <at> lepiller.eu>
Cc: 36375 <at> debbugs.gnu.org
Subject: Re: bug#36375: [PATCH] Re: ‘guix package’ should lock the profile
Date: Fri, 08 Nov 2019 21:03:05 +0100

Hello!

Julien Lepiller <julien <at> lepiller.eu> skribis:

> From 71a85b5a8aac6c0bd5a1a4e3b52e409b2112df7a Mon Sep 17 00:00:00 2001
> From: Julien Lepiller <julien <at> 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 <at> 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’.

Message #28 received at 36375-done <at> debbugs.gnu.org (full text, mbox):

From: Julien Lepiller <julien <at> lepiller.eu>
To: 36375-done <at> debbugs.gnu.org
Subject: Re: bug#36375: [PATCH] Re: ‘guix package’
 should lock the profile
Date: Fri, 8 Nov 2019 22:03:04 +0100

Le Fri, 08 Nov 2019 21:03:05 +0100,
Ludovic Courtès <ludo <at> gnu.org> a écrit :

> Hello!
> 
> Julien Lepiller <julien <at> lepiller.eu> skribis:
> 
> > From 71a85b5a8aac6c0bd5a1a4e3b52e409b2112df7a Mon Sep 17 00:00:00
> > 2001 From: Julien Lepiller <julien <at> 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 <at> 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.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.