(address . guix-patches@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 <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(-)
Toggle diff (37 lines)
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 <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(-)
Toggle diff (17 lines)
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
-----BEGIN PGP SIGNATURE-----
iQEzBAEBCAAdFiEEdNapMPRLm4SepVYGwWaqSV9/GJwFAl6zhesACgkQwWaqSV9/
GJx4+Af6AyCZByhnQmIT9akppywz1Mut+YKV7IWkKBLifnbuqbaQl3faBbo4io6l
1++rq1FypEKUlepgQLwUGWfEdR21WPPxwL+LopllcqTklZO45WB7PsOr1wIQjvW1
/mxbbLIU9de37gzNl4caKO1Ijlra2fKmWzFqbSpy5h17dg2Q+1LFf0epLOwyOq7E
m1fpvcaPp8IOj2X/Bb25XDLCopkJB5NYdJYoT8yAsXNQd3ORmTw4GnS9NoYTYVR8
7w6fdlAhDP6xjVKyJPwPos/u7T7loskn8wxBi85TFZG1RcAha1ib2TuAi83xEzam
Y/pM8cn76UL7SWUuJ9i3IwJ58Rj75Q==
=FFN3
-----END PGP SIGNATURE-----