(guix git) and guile-git finalizers.

(guix git) and guile-git finalizers.

[Mathieu Othacehe]

Hi !

While integrating (guix git) into Cuirass, I noticed unexpected
segfaults during "latest-repository-commit" evaluation.

Segfaults can be narrowed to the following lines:

--8<---------------cut here---------------start------------->8---
(use-modules (git))
(begin (libgit2-init!)
       (let ((rep (repository-open "some-git-repo")))
         (remote-fetch (remote-lookup rep "origin")))
       (libgit2-shutdown))
--8<---------------cut here---------------end--------------->8---

Note that, it's not 100% reproducible, but re-executing 4 or 5 times the
above snippet is often enough to observe the segfault.

Here's the gdb backtrace I collected :

--8<---------------cut here---------------start------------->8---
#0  0x00007fffef7eb181 in git_mwindow_put_pack () from /gnu/store/ilaffvq27kfy2la235hddkxcalj9r
#1  0x00007fffef7f2897 in pack_backend(float, long double,...)(...) () from /gnu/store/ilaffvq2
#2  0x00007fffef7eef72 in git_odb_free () from /gnu/store/ilaffvq27kfy2la235hddkxcalj9r2i7-libg
#3  0x00007fffef8134ce in git_repository.cleanup () from /gnu/store/ilaffvq27kfy2la235hddkxcalj
#4  0x00007fffef81351a in git_repository_free () from /gnu/store/ilaffvq27kfy2la235hddkxcalj9r2
#5  0x00007ffff7b01e37 in chained_finalizer () from /gnu/store/5zx29y44nrqj0s8h3jlvlj82k8hj4dxs
#6  0x00007ffff785170f in GC_invoke_finalizers () from /gnu/store/j4p7cnxb6byf1wra3yz6pk8av4g5m
#7  0x00007ffff7b02099 in scm_run_finalizers () from /gnu/store/5zx29y44nrqj0s8h3jlvlj82k8hj4dx
#8  0x00007ffff7b020f5 in finalization_thread_proc () from /gnu/store/5zx29y44nrqj0s8h3jlvlj82k
#9  0x00007ffff7af2cba in c_body () from /gnu/store/5zx29y44nrqj0s8h3jlvlj82k8hj4dxs-guile-2.2.
#10 0x00007ffff7b680c9 in vm_debug_engine () from /gnu/store/5zx29y44nrqj0s8h3jlvlj82k8hj4dxs-g
#11 0x00007ffff7b752aa in scm_call_n () from /gnu/store/5zx29y44nrqj0s8h3jlvlj82k8hj4dxs-guile-
#12 0x00007ffff7b644e6 in catch () from /gnu/store/5zx29y44nrqj0s8h3jlvlj82k8hj4dxs-guile-2.2.2
#13 0x00007ffff7af32a0 in scm_i_with_continuation_barrier () from /gnu/store/5zx29y44nrqj0s8h3j
#14 0x00007ffff7af3335 in scm_c_with_continuation_barrier () from /gnu/store/5zx29y44nrqj0s8h3j
#15 0x00007ffff7b6310c in with_guile () from /gnu/store/5zx29y44nrqj0s8h3jlvlj82k8hj4dxs-guile-
#16 0x00007ffff7858732 in GC_call_with_stack_base () from /gnu/store/j4p7cnxb6byf1wra3yz6pk8av4
#17 0x00007ffff7b63498 in scm_with_guile () from /gnu/store/5zx29y44nrqj0s8h3jlvlj82k8hj4dxs-gu
#18 0x00007ffff7625454 in start_thread () from /gnu/store/rmjlycdgiq8pfy5hfi42qhw3k7p6kdav-glib
#19 0x00007ffff5fed7bf in clone () from /gnu/store/rmjlycdgiq8pfy5hfi42qhw3k7p6kdav-glibc-2.25/
--8<---------------cut here---------------end--------------->8---

I supposed that what happends is the following race-condition :
* (libgit2-shutdown) is called
* the finalizer %repository-free is called in an other thread.
* because shutdown has already been called the finalizer fails and
segfaults.

Maybe the right answer is to disable auto finalization and call
run-finalizers as described here :

https://www.gnu.org/software/guile/manual/html_node/Foreign-Object-Memory-Management.html#Foreign-Object-Memory-Management

However I can't find guile counterparts for those libguile functions.

WDYT ?

Thanks,

Mathieu

Re: (guix git) and guile-git finalizers.

[Ludovic Courtès]

Hi!

Mathieu Othacehe <m.othacehe@gmail.com> skribis:

> While integrating (guix git) into Cuirass, I noticed unexpected
> segfaults during "latest-repository-commit" evaluation.
>
> Segfaults can be narrowed to the following lines:
>
> (use-modules (git))
> (begin (libgit2-init!)
>        (let ((rep (repository-open "some-git-repo")))
>          (remote-fetch (remote-lookup rep "origin")))
>        (libgit2-shutdown))
>
>
> Note that, it's not 100% reproducible, but re-executing 4 or 5 times the
> above snippet is often enough to observe the segfault.
>
> Here's the gdb backtrace I collected :
>
> #0  0x00007fffef7eb181 in git_mwindow_put_pack () from /gnu/store/ilaffvq27kfy2la235hddkxcalj9r
> #1  0x00007fffef7f2897 in pack_backend(float, long double,...)(...) () from /gnu/store/ilaffvq2
> #2  0x00007fffef7eef72 in git_odb_free () from /gnu/store/ilaffvq27kfy2la235hddkxcalj9r2i7-libg
> #3  0x00007fffef8134ce in git_repository.cleanup () from /gnu/store/ilaffvq27kfy2la235hddkxcalj
> #4  0x00007fffef81351a in git_repository_free () from /gnu/store/ilaffvq27kfy2la235hddkxcalj9r2
> #5  0x00007ffff7b01e37 in chained_finalizer () from /gnu/store/5zx29y44nrqj0s8h3jlvlj82k8hj4dxs
> #6  0x00007ffff785170f in GC_invoke_finalizers () from /gnu/store/j4p7cnxb6byf1wra3yz6pk8av4g5m
> #7  0x00007ffff7b02099 in scm_run_finalizers () from /gnu/store/5zx29y44nrqj0s8h3jlvlj82k8hj4dx
> #8  0x00007ffff7b020f5 in finalization_thread_proc () from /gnu/store/5zx29y44nrqj0s8h3jlvlj82k
> #9  0x00007ffff7af2cba in c_body () from /gnu/store/5zx29y44nrqj0s8h3jlvlj82k8hj4dxs-guile-2.2.
> #10 0x00007ffff7b680c9 in vm_debug_engine () from /gnu/store/5zx29y44nrqj0s8h3jlvlj82k8hj4dxs-g
> #11 0x00007ffff7b752aa in scm_call_n () from /gnu/store/5zx29y44nrqj0s8h3jlvlj82k8hj4dxs-guile-
> #12 0x00007ffff7b644e6 in catch () from /gnu/store/5zx29y44nrqj0s8h3jlvlj82k8hj4dxs-guile-2.2.2
> #13 0x00007ffff7af32a0 in scm_i_with_continuation_barrier () from /gnu/store/5zx29y44nrqj0s8h3j
> #14 0x00007ffff7af3335 in scm_c_with_continuation_barrier () from /gnu/store/5zx29y44nrqj0s8h3j
> #15 0x00007ffff7b6310c in with_guile () from /gnu/store/5zx29y44nrqj0s8h3jlvlj82k8hj4dxs-guile-
> #16 0x00007ffff7858732 in GC_call_with_stack_base () from /gnu/store/j4p7cnxb6byf1wra3yz6pk8av4
> #17 0x00007ffff7b63498 in scm_with_guile () from /gnu/store/5zx29y44nrqj0s8h3jlvlj82k8hj4dxs-gu
> #18 0x00007ffff7625454 in start_thread () from /gnu/store/rmjlycdgiq8pfy5hfi42qhw3k7p6kdav-glib
> #19 0x00007ffff5fed7bf in clone () from /gnu/store/rmjlycdgiq8pfy5hfi42qhw3k7p6kdav-glibc-2.25/
>
> I supposed that what happends is the following race-condition :
> * (libgit2-shutdown) is called
> * the finalizer %repository-free is called in an other thread.
> * because shutdown has already been called the finalizer fails and
> segfaults.

Could be!

I think using ‘libgit2-shutdown’ is risky because as you can see it can
be tricky to implement it correctly.  So my first recommendation would
be to not try to use it.  :-)  Guile cannot really unload modules ATM
anyway.

To fix this, I think ‘libgit2-shutdown’ needs to call
‘scm_run_finalizers’ before calling ‘git_shutdown’, but I’m not sure if
it’s enough.

HTH,
Ludo’.

Re: (guix git) and guile-git finalizers.

[Mathieu Othacehe]

Hi Ludo,

> To fix this, I think ‘libgit2-shutdown’ needs to call
> ‘scm_run_finalizers’ before calling ‘git_shutdown’, but I’m not sure if
> it’s enough.

I tried this approach (using dynamic-call), and indeed, it's not enough.
Before, removing (libgit2-shutdown) from (guix-git), I'll explore the
guardian solution proposed by Andy.

Thanks for your help,

Mathieu

Re: (guix git) and guile-git finalizers.

[Mathieu Othacehe]

[-- Attachment #1: Type: text/plain, Size: 581 bytes --]


Hi !

I may have found something that replaces finalizers. I followed Andy and
Amirouche advices and used guardians, as in guile-sqlite3.

The idea here is to create a guardian per pointer-type to finalize. A
pumper function that operates on this guardian is also created. This
pumper function knows the git_libgit2_xxx function to call to free the
pointers stored in guardians.

Finally, all pumpers are registered into a hook that is run before
shutting down.

I tried it on "repository" pointers but it can be easily extended to the
other finalizers.

WDYT ?

Thanks,

Mathieu

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Replace-pointer-finalizer-by-guardians.patch --]
[-- Type: text/x-diff, Size: 3269 bytes --]

From 84bd5aa7f1c5bd01c721d475f8fdfad5533c71b5 Mon Sep 17 00:00:00 2001
From: Mathieu Othacehe <m.othacehe@gmail.com>
Date: Mon, 19 Jun 2017 17:48:08 +0200
Subject: [PATCH] Replace pointer finalizer by guardians.

* git/bindings.scm (%pumpers-hook): New exported variable,
(define-element-guardian): New exported macro.
* git/repository.scm (%repository-free): Remove and replace by a call
  to define-element-guardian macro.
 (pointer->repository): Do not set a pointer finalizer and add the
  pointer to the guardian generated by define-element-guardian
  instead.
---
 git/bindings.scm   | 37 ++++++++++++++++++++++++++++++++++---
 git/repository.scm |  4 ++--
 2 files changed, 36 insertions(+), 5 deletions(-)

diff --git a/git/bindings.scm b/git/bindings.scm
index 189369e..29eab73 100644
--- a/git/bindings.scm
+++ b/git/bindings.scm
@@ -30,7 +30,9 @@
             make-buffer
             free-buffer
             buffer-content
-            buffer-content/string))
+            buffer-content/string
+            %pumpers-hook
+            define-element-guardian))
 
 ;; DRAFT!
 
@@ -207,8 +209,37 @@
 (define libgit2-opts
   (libgit2->procedure int "git_libgit2_init" `(,int)))
 
-(define-public libgit2-shutdown
-  (libgit2->procedure int "git_libgit2_shutdown" '()))
+(define %pumpers-hook (make-hook))
+
+(define-syntax define-element-guardian
+  (lambda (x)
+    (define-syntax-rule (id ctx parts ...)
+      "Assemble PARTS into a raw (unhygienic)  identifier."
+      (datum->syntax ctx (symbol-append (syntax->datum parts) ...)))
+    (syntax-case x ()
+      ((_ element finalizer hook)
+       (with-syntax
+           ((finalizer-name (id #'element #'element #'-free))
+            (guardian-name  (id #'element #'element #'-guardian))
+            (pumper-name    (id #'element #'pump- #'element #'-guardian)))
+         #'(begin
+             (define finalizer-name
+               (let ((proc (libgit2->procedure void finalizer '(*))))
+                 (lambda (pointer)
+                   (proc pointer))))
+             (define guardian-name (make-guardian))
+             (define (pumper-name)
+               (let ((pointer (guardian-name)))
+                 (when pointer
+                   (finalizer-name pointer)
+                   (pumper-name))))
+             (add-hook! hook pumper-name)))))))
+
+(define-public (libgit2-shutdown)
+  ;; Before shutting down, try to finalize pointers kept in guardians.
+  (run-hook %pumpers-hook)
+  (let ((proc (libgit2->procedure int "git_libgit2_shutdown" '())))
+    (proc)))
 
 (define libgit2-version
   (let ((proc (libgit2->procedure void "git_libgit2_version" '(* * *))))
diff --git a/git/repository.scm b/git/repository.scm
index f82dff4..646dbe4 100644
--- a/git/repository.scm
+++ b/git/repository.scm
@@ -89,10 +89,10 @@
 
 ;; FIXME: https://libgit2.github.com/libgit2/#HEAD/group/repository/git_repository_fetchhead_foreach
 
-(define %repository-free (dynamic-func "git_repository_free" libgit2))
+(define-element-guardian repository "git_repository_free" %pumpers-hook)
 
 (define (pointer->repository! pointer)
-  (set-pointer-finalizer! pointer %repository-free)
+  (repository-guardian pointer)
   (pointer->repository pointer))
 
 (define repository-get-namespace
-- 
2.1.4

Re: (guix git) and guile-git finalizers.

[Andy Wingo]

Hi :)

Just some code review.

On Mon 19 Jun 2017 18:01, Mathieu Othacehe <m.othacehe@gmail.com> writes:

> The idea here is to create a guardian per pointer-type to finalize. A
> pumper function that operates on this guardian is also created. This
> pumper function knows the git_libgit2_xxx function to call to free the
> pointers stored in guardians.

Neat.  A couple problems though.

  (1) All finalizable objects will be kept alive until they are removed
  from the guardian.  With guardians, you usually want to incrementally
  visit it during program execution: say, every time you allocate a new
  repository object.  Or via after-gc-hook.

  (2) Whether an object is visible in the guardian is not a very
  deterministic property.  It won't be returned from a guardian until
  all other references are gone, but there's no guarantee about when it
  will be returned -- depends on GC and many other factors.  So it's
  possible that repository objects are still "alive" after you shut down
  libgit.

Why do you feel the need to "shut down" libgit?  Honestly I would punt.
Most other libraries aren't like that.

Andy

Re: (guix git) and guile-git finalizers.

[Mathieu Othacehe]

[-- Attachment #1: Type: text/plain, Size: 320 bytes --]


Hi Andy,

> Why do you feel the need to "shut down" libgit?  Honestly I would punt.
> Most other libraries aren't like that.

Thanks for the review and pointing out those two issues. So our best
option may be to stop using libgit2-shutdown in (guix git).

I attached a patch, what do you guys think ?

Thanks,

Mathieu

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-guix-git-Stop-using-libgit2-shutdown.patch --]
[-- Type: text/x-patch, Size: 1168 bytes --]

From d8336eb41574d41bd36330c26f56da8090c8907e Mon Sep 17 00:00:00 2001
From: Mathieu Othacehe <m.othacehe@gmail.com>
Date: Sat, 1 Jul 2017 12:14:05 +0200
Subject: [PATCH] guix: git: Stop using libgit2-shutdown.

* guix/git.scm (with-libgit2): Stop calling (libgit2-shutdown) to prevent
segfaults when pointer finalizers are run.
---
 guix/git.scm | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/guix/git.scm b/guix/git.scm
index 17a6784ae..406c81734 100644
--- a/guix/git.scm
+++ b/guix/git.scm
@@ -34,13 +34,12 @@
   (make-parameter "/var/cache/guix/checkouts"))
 
 (define-syntax-rule (with-libgit2 thunk ...)
-  (dynamic-wind
-    (lambda ()
-      (libgit2-init!))
-    (lambda ()
-      thunk ...)
-    (lambda ()
-      (libgit2-shutdown))))
+  (begin
+    ;; XXX: The right thing to do would be to call (libgit2-shutdown) here,
+    ;; but pointer finalizers used in guile-git may be called after shutdown,
+    ;; resulting in a segfault. Hence, let's skip shutdown call for now.
+    (libgit2-init!)
+    thunk ...))
 
 (define* (url-cache-directory url
                               #:optional (cache-directory
-- 
2.13.1

Re: (guix git) and guile-git finalizers.

[Ludovic Courtès]

Mathieu Othacehe <m.othacehe@gmail.com> skribis:

> From d8336eb41574d41bd36330c26f56da8090c8907e Mon Sep 17 00:00:00 2001
> From: Mathieu Othacehe <m.othacehe@gmail.com>
> Date: Sat, 1 Jul 2017 12:14:05 +0200
> Subject: [PATCH] guix: git: Stop using libgit2-shutdown.
>
> * guix/git.scm (with-libgit2): Stop calling (libgit2-shutdown) to prevent
> segfaults when pointer finalizers are run.

Fine with me, by far the simplest and safest solution.  :-)

Thanks,
Ludo’.

Re: (guix git) and guile-git finalizers.

[Mathieu Othacehe]

> Fine with me, by far the simplest and safest solution.  :-)

Ok thanks ! Pushed as b02469d29.

Mathieu

Re: (guix git) and guile-git finalizers.

[Andy Wingo]

On Mon 12 Jun 2017 14:44, Mathieu Othacehe <m.othacehe@gmail.com> writes:

> Maybe the right answer is to disable auto finalization and call
> run-finalizers as described here :
>
> https://www.gnu.org/software/guile/manual/html_node/Foreign-Object-Memory-Management.html#Foreign-Object-Memory-Management
>
> However I can't find guile counterparts for those libguile functions.

Nice sleuthing!

If you want to manually handle finalization at 'safe' points from
Scheme, guardians are the things to use.  Add your "finalizable" object
to a guardian, and periodically "pump" the guardian to see if anything
is finalizable; you can run the finalizer there.

Andy

Re: (guix git) and guile-git finalizers.

[Mathieu Othacehe]

Hi Andy,

> Nice sleuthing!

Thanks :)

>
> If you want to manually handle finalization at 'safe' points from
> Scheme, guardians are the things to use.  Add your "finalizable" object
> to a guardian, and periodically "pump" the guardian to see if anything
> is finalizable; you can run the finalizer there.

So, the idea would be to create a global guardian with :

--8<---------------cut here---------------start------------->8---
(define pointer-guardian (make-guardian))
--8<---------------cut here---------------end--------------->8---

and replace those kind of functions :

--8<---------------cut here---------------start------------->8---
(define (pointer->commit! pointer)
  (set-pointer-finalizer! pointer %commit-free)
  (pointer->commit pointer))
--8<---------------cut here---------------end--------------->8---

by,

--8<---------------cut here---------------start------------->8---
(define (pointer->commit! pointer)
  (pointer-guardian pointer)
  (pointer->commit pointer))
--8<---------------cut here---------------end--------------->8---

But then, at shutdown, when I'll call (pointer-guardian) to get the list
of free-able pointers, how can I know that %commit-free has to be called
on commit pointers, %reference-free on reference pointers, ... ?

Thanks,

Mathieu