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

[Caleb Ristvedt <caleb.ristvedt@cune.org>]

[-- Attachment #1.1: Type: text/plain, Size: 825 bytes --]

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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: FINALIZE-STORE-FILE fix --]
[-- Type: text/x-patch, Size: 1739 bytes --]

From b2c66b443bd42e05820cfb3920c96f1894820587 Mon Sep 17 00:00:00 2001
From: Caleb Ristvedt <caleb.ristvedt@cune.org>
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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.3: WITH-TEMPORARY-STORE-FILE fix --]
[-- Type: text/x-patch, Size: 1340 bytes --]

From 43ee61b405b01038b3e7c84aba64521ab8a62236 Mon Sep 17 00:00:00 2001
From: Caleb Ristvedt <caleb.ristvedt@cune.org>
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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]