GNU bug report logs - #66387
[PATCH] home: services: Fix race condition when detecting first login

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 66387 in the body.
You can then email your comments to 66387 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: Carlo Zancanaro <carlo <at> zancanaro.id.au>
To: guix-patches <at> gnu.org
Subject: [PATCH] home: services: Fix race condition when detecting first login
Date: Sat,  7 Oct 2023 22:59:05 +1100

* gnu/home/services.scm (compute-on-first-login-script): Use open to
atomically check whether a file exists and create it if not.
---

I run Guix Home on NixOS with SDDM as my display manager. When I log in, I find that there are two shepherd processes running. Looking at the on-first-login script I noticed a race condition that I suspect was causing the issue.

I believe the "open" incantation I have used is atomic, except on NFS, so this should ensure that the gexps are only run once. I have confirmed that this patch fixes my problem. With this patch, when I login I only have a single shepherd process.

 gnu/home/services.scm | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/gnu/home/services.scm b/gnu/home/services.scm
index 8d53f2f4d3..95ef66e091 100644
--- a/gnu/home/services.scm
+++ b/gnu/home/services.scm
@@ -2,6 +2,7 @@
 ;;; Copyright © 2021-2023 Andrew Tropin <andrew <at> trop.in>
 ;;; Copyright © 2021 Xinglu Chen <public <at> yoctocell.xyz>
 ;;; Copyright © 2022-2023 Ludovic Courtès <ludo <at> gnu.org>
+;;; Copyright © 2023 Carlo Zancanaro <carlo <at> zancanaro.id.au>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -412,20 +413,29 @@ (define (compute-on-first-login-script _ gexps)
      #~(begin
          (use-modules (guix i18n)
                       (guix diagnostics))
+
+       (define (claim-first-run file-name)
+         (catch #t
+           (lambda ()
+             ;; This incantation will raise an error if the file at
+             ;; flag-file-path already exists, and will create it otherwise.
+             (close (open file-name (logior O_CREAT O_EXCL)))
+             #t)
+           (lambda (e)
+             #f)))
+
        #$%initialize-gettext
 
        (let* ((xdg-runtime-dir (or (getenv "XDG_RUNTIME_DIR")
                                    (format #f "/run/user/~a" (getuid))))
               (flag-file-path (string-append
-                               xdg-runtime-dir "/on-first-login-executed"))
-              (touch (lambda (file-name)
-                       (call-with-output-file file-name (const #t)))))
+                               xdg-runtime-dir "/on-first-login-executed")))
          ;; XDG_RUNTIME_DIR dissapears on logout, that means such trick
          ;; allows to launch on-first-login script on first login only
          ;; after complete logout/reboot.
          (if (file-exists? xdg-runtime-dir)
-             (unless (file-exists? flag-file-path)
-               (begin #$@gexps (touch flag-file-path)))
+             (when (claim-first-run flag-file-path)
+               #$@gexps)
              ;; TRANSLATORS: 'on-first-login' is the name of a service and
              ;; shouldn't be translated
              (warning (G_ "XDG_RUNTIME_DIR doesn't exists, on-first-login script

base-commit: b566e1a98a74d84d3978cffefd05295602c9445d
-- 
2.41.0

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

From: Carlo Zancanaro <carlo <at> zancanaro.id.au>
Cc: 66387 <at> debbugs.gnu.org
Subject: Re: [bug#66387] [PATCH] home: services: Fix race condition when
 detecting first login
Date: Tue, 10 Oct 2023 00:08:06 +1100

On Sat, Oct 07 2023, Carlo Zancanaro wrote:
> @@ -412,20 +413,29 @@ (define (compute-on-first-login-script _ 
> gexps)
>       #~(begin
>           (use-modules (guix i18n)
>                        (guix diagnostics))
> +
> +       (define (claim-first-run file-name)
> +         (catch #t
> +           (lambda ()
> +             ;; This incantation will raise an error if the 
> file at
> +             ;; flag-file-path already exists, and will create 
> it otherwise.
> +             (close (open file-name (logior O_CREAT O_EXCL)))
> +             #t)
> +           (lambda (e)

Whoops, just noticed this should be _, not (e). It still works, 
because it crashes the script which stops the gexps from running, 
but writes a message to the console. I'll send an updated patch 
tomorrow.

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

From: Carlo Zancanaro <carlo <at> zancanaro.id.au>
To: 66387 <at> debbugs.gnu.org
Subject: [PATCH] home: services: Fix race condition when detecting first login
Date: Wed, 11 Oct 2023 22:57:19 +1100

* gnu/home/services.scm (compute-on-first-login-script): Use open to
atomically check whether a file exists and create it if not.
---
 gnu/home/services.scm | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/gnu/home/services.scm b/gnu/home/services.scm
index 8d53f2f4d3..7137925b30 100644
--- a/gnu/home/services.scm
+++ b/gnu/home/services.scm
@@ -2,6 +2,7 @@
 ;;; Copyright © 2021-2023 Andrew Tropin <andrew <at> trop.in>
 ;;; Copyright © 2021 Xinglu Chen <public <at> yoctocell.xyz>
 ;;; Copyright © 2022-2023 Ludovic Courtès <ludo <at> gnu.org>
+;;; Copyright © 2023 Carlo Zancanaro <carlo <at> zancanaro.id.au>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -412,20 +413,29 @@ (define (compute-on-first-login-script _ gexps)
      #~(begin
          (use-modules (guix i18n)
                       (guix diagnostics))
+
+       (define (claim-first-run file-name)
+         (catch #t
+           (lambda ()
+             ;; This incantation will raise an error if the file at
+             ;; flag-file-path already exists, and will create it otherwise.
+             (close (open file-name (logior O_CREAT O_EXCL)))
+             #t)
+           (lambda _
+             #f)))
+
        #$%initialize-gettext
 
        (let* ((xdg-runtime-dir (or (getenv "XDG_RUNTIME_DIR")
                                    (format #f "/run/user/~a" (getuid))))
               (flag-file-path (string-append
-                               xdg-runtime-dir "/on-first-login-executed"))
-              (touch (lambda (file-name)
-                       (call-with-output-file file-name (const #t)))))
+                               xdg-runtime-dir "/on-first-login-executed")))
          ;; XDG_RUNTIME_DIR dissapears on logout, that means such trick
          ;; allows to launch on-first-login script on first login only
          ;; after complete logout/reboot.
          (if (file-exists? xdg-runtime-dir)
-             (unless (file-exists? flag-file-path)
-               (begin #$@gexps (touch flag-file-path)))
+             (when (claim-first-run flag-file-path)
+               #$@gexps)
              ;; TRANSLATORS: 'on-first-login' is the name of a service and
              ;; shouldn't be translated
              (warning (G_ "XDG_RUNTIME_DIR doesn't exists, on-first-login script

base-commit: 9ad9113fc238ee8de5191a5e15b5153fd149e9fa
-- 
2.41.0

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

From: Ludovic Courtès <ludo <at> gnu.org>
To: Carlo Zancanaro <carlo <at> zancanaro.id.au>
Cc: 66387-done <at> debbugs.gnu.org, Andrew Tropin <andrew <at> trop.in>,
 paren <at> disroot.org
Subject: Re: [bug#66387] [PATCH] home: services: Fix race condition when
 detecting first login
Date: Thu, 19 Oct 2023 22:29:34 +0200

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

Hi Carlo,

Carlo Zancanaro <carlo <at> zancanaro.id.au> skribis:

> * gnu/home/services.scm (compute-on-first-login-script): Use open to
> atomically check whether a file exists and create it if not.

Good catch!

I made the following cosmetic changes (‘open-fdes’ is cheaper than
‘open’) and applied it.

Thanks!

Ludo’.

[Message part 2 (text/x-patch, inline)]

diff --git a/gnu/home/services.scm b/gnu/home/services.scm
index 7137925b30..651c068f79 100644
--- a/gnu/home/services.scm
+++ b/gnu/home/services.scm
@@ -414,15 +414,15 @@ (define (compute-on-first-login-script _ gexps)
          (use-modules (guix i18n)
                       (guix diagnostics))
 
-       (define (claim-first-run file-name)
+       (define (claim-first-run file)
          (catch #t
            (lambda ()
-             ;; This incantation will raise an error if the file at
-             ;; flag-file-path already exists, and will create it otherwise.
-             (close (open file-name (logior O_CREAT O_EXCL)))
+             ;; This incantation raises an error if FILE already exists, and
+             ;; creates it otherwise.
+             (close-fdes
+              (open-fdes file (logior O_CREAT O_EXCL O_CLOEXEC)))
              #t)
-           (lambda _
-             #f)))
+           (const #f)))
 
        #$%initialize-gettext
 

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

From: Andrew Tropin <andrew <at> trop.in>
To: Ludovic Courtès <ludo <at> gnu.org>, Carlo Zancanaro
 <carlo <at> zancanaro.id.au>
Cc: 66387-done <at> debbugs.gnu.org, paren <at> disroot.org
Subject: Re: [bug#66387] [PATCH] home: services: Fix race condition when
 detecting first login
Date: Fri, 20 Oct 2023 10:55:46 +0400

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

On 2023-10-19 22:29, Ludovic Courtès wrote:

> Hi Carlo,
>
> Carlo Zancanaro <carlo <at> zancanaro.id.au> skribis:
>
>> * gnu/home/services.scm (compute-on-first-login-script): Use open to
>> atomically check whether a file exists and create it if not.
>
> Good catch!
>
> I made the following cosmetic changes (‘open-fdes’ is cheaper than
> ‘open’) and applied it.
>
> Thanks!
>
> Ludo’.
>
> diff --git a/gnu/home/services.scm b/gnu/home/services.scm
> index 7137925b30..651c068f79 100644
> --- a/gnu/home/services.scm
> +++ b/gnu/home/services.scm
> @@ -414,15 +414,15 @@ (define (compute-on-first-login-script _ gexps)
>           (use-modules (guix i18n)
>                        (guix diagnostics))
>  
> -       (define (claim-first-run file-name)
> +       (define (claim-first-run file)

Very nice trick!  Carol, Ludo, Thank you for the fix!

>           (catch #t
>             (lambda ()
> -             ;; This incantation will raise an error if the file at
> -             ;; flag-file-path already exists, and will create it otherwise.
> -             (close (open file-name (logior O_CREAT O_EXCL)))
> +             ;; This incantation raises an error if FILE already exists, and
> +             ;; creates it otherwise.
> +             (close-fdes
> +              (open-fdes file (logior O_CREAT O_EXCL O_CLOEXEC)))
>               #t)
> -           (lambda _
> -             #f)))
> +           (const #f)))
>  
>         #$%initialize-gettext
>  

-- 
Best regards,
Andrew Tropin

[signature.asc (application/pgp-signature, inline)]

Previous Next

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