Subject: bug#41119: closed (Re: [bug#41119] [PATCH] fix some issues with (guix nar))

From: [email-masked] (GNU bug Tracking System)
Date: Mon, 11 May 2020 21:40:02 +0000

Your bug report

#41119: [PATCH] fix some issues with (guix nar)

which was filed against the guix-patches package, has been closed.

The explanation is attached below, along with your original report.
If you require more details, please reply to [email-masked].

-- 
41119: http://debbugs.gnu.org/cgi/bugreport.cgi?bug=41119
GNU Bug Tracking System
Contact [email-masked] with problems

Subject: Re: [bug#41119] [PATCH] fix some issues with (guix nar)
From: Ludovic Courtès <[email-masked]>
To: Caleb Ristvedt <[email-masked]>
Cc: [email-masked]
Date: Mon, 11 May 2020 23:39:03 +0200
List-Id: <debbugs-submit.debbugs.gnu.org>

This was pushed a couple of days ago:

  b338c41c82 nar: 'with-temporary-store-file' uses a single connection
  37edbc91e3 nar: 'finalize-store-file' follows proper store lock protocol.

Closing, and thanks again!

Ludo’.

Subject: [PATCH] fix some issues with (guix nar)
From: Caleb Ristvedt <[email-masked]>
To: [email-masked]
Date: Wed, 06 May 2020 22:52:11 -0500
List-Id: <debbugs-submit.debbugs.gnu.org>

I noticed two issues while looking at (guix nar):

1. The proper store-lock-handling protocol isn't used in
   FINALIZE-STORE-FILE. Lock acquisition needs to check for a deletion
   token, retrying if it exists, and lock release needs to delete the
   lock file and write the deletion token.

2. WITH-TEMPORARY-STORE-FILE opens a new daemon connection every time it
   retries with a new filename, and only closes any of them after the
   body has completed. So if we retry 20 times, we get 20 concurrent
   daemon connections. This also prevents the call to LOOP from being a
   tail call.

The attached patches resolve these issues. There are of course going to
be more places we need to (properly) acquire and release store locks as
guile-daemon code gets merged, but for now this should work as a bandaid
fix.

- reepca

From b2c66b443bd42e05820cfb3920c96f1894820587 Mon Sep 17 00:00:00 2001
From: Caleb Ristvedt <[email-masked]>
Date: Wed, 6 May 2020 11:48:21 -0500
Subject: [PATCH 1/2] nar: 'finalize-store-file' follows proper store lock
 protocol.

* guix/nar.scm (finalize-store-file): check for deletion token when acquiring
  lock, write deletion token and delete lock file before releasing lock.
---
 guix/nar.scm | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/guix/nar.scm b/guix/nar.scm
index 29636aa0f8..f91af72879 100644
--- a/guix/nar.scm
+++ b/guix/nar.scm
@@ -82,10 +82,19 @@
 REFERENCES and DERIVER.  When LOCK? is true, acquire exclusive locks on TARGET
 before attempting to register it; otherwise, assume TARGET's locks are already
 held."
+  ;; TODO: make this reusable
+  (define (acquire-lock filename)
+    (let ((port (lock-file filename)))
+      (if (zero? (stat:size (stat port)))
+          port
+          (begin
+            (close port)
+            (acquire-lock filename)))))
+
   (with-database %default-database-file db
     (unless (path-id db target)
       (let ((lock (and lock?
-                       (lock-file (string-append target ".lock")))))
+                       (acquire-lock (string-append target ".lock")))))
 
         (unless (path-id db target)
           ;; If FILE already exists, delete it (it's invalid anyway.)
@@ -102,6 +111,9 @@ held."
                          #:deriver deriver))
 
         (when lock?
+          (delete-file (string-append target ".lock"))
+          (display "d" lock)
+          (force-output lock)
           (unlock-file lock))))))
 
 (define (temporary-store-file)
-- 
2.26.2

From 43ee61b405b01038b3e7c84aba64521ab8a62236 Mon Sep 17 00:00:00 2001
From: Caleb Ristvedt <[email-masked]>
Date: Wed, 6 May 2020 11:52:16 -0500
Subject: [PATCH 2/2] nar: 'with-temporary-store-file' uses a single connection

Previously the 'with-store' form was entered every time a different temporary
file was tried.  This caused there to be as many simultaneous open connections
as there were attempts, and prevented the (loop ...) call from being a tail
call.  This change fixes that.

* guix/nar.scm (with-temporary-store-file): open connection once prior to
  entering the loop.
---
 guix/nar.scm | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/guix/nar.scm b/guix/nar.scm
index f91af72879..404cef8b97 100644
--- a/guix/nar.scm
+++ b/guix/nar.scm
@@ -126,8 +126,8 @@ held."
 (define-syntax-rule (with-temporary-store-file name body ...)
   "Evaluate BODY with NAME bound to the file name of a temporary store item
 protected from GC."
-  (let loop ((name (temporary-store-file)))
-    (with-store store
+  (with-store store
+    (let loop ((name (temporary-store-file)))
       ;; Add NAME to the current process' roots.  (Opening this connection to
       ;; the daemon allows us to reuse its code that deals with the
       ;; per-process roots file.)
-- 
2.26.2