GNU bug report logs - #76589
[PATCH] Exclude the finalizer thread from the ‘all-threads’ result.

Previous Next

To reply to this bug, email your comments to 76589 AT debbugs.gnu.org.

Toggle the display of automated, internal messages from the tracker.

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

From: Ludovic Courtès <ludo <at> gnu.org>
To: bug-guile <at> gnu.org
Cc: Simon Josefsson <simon <at> josefsson.org>,
 Ludovic Courtès <ludo <at> gnu.org>
Subject: [PATCH] Exclude the finalizer thread from the ‘all-threads’ result.
Date: Wed, 26 Feb 2025 15:55:20 +0100

Fixes <https://bugs.gnu.org/76343>.

Fixes a bug whereby “echo '(environ)' | guile” would wrongfully trigger
the multiple-thread warning.

* libguile/finalizers.c (finalizer_thread): New variable.
(finalization_thread_proc): Set it.
(scm_i_is_finalizer_thread): New function.
(run_finalization_thread): Clear FINALIZER_THREAD.
* libguile/finalizers.h (scm_i_is_finalizer_thread): New declaration.
* libguile/threads.c (scm_all_threads): Use it.
* NEWS: Update.

Reported-by: Simon Josefsson <simon <at> josefsson.org>
---
 NEWS                  |  8 +++++++-
 libguile/finalizers.c | 21 ++++++++++++++++++---
 libguile/finalizers.h |  5 ++++-
 libguile/threads.c    |  7 +++++--
 4 files changed, 34 insertions(+), 7 deletions(-)

diff --git a/NEWS b/NEWS
index 3328a03cf..2a59874d4 100644
--- a/NEWS
+++ b/NEWS
@@ -1,5 +1,5 @@
 Guile NEWS --- history of user-visible changes.
-Copyright (C) 1996-2024 Free Software Foundation, Inc.
+Copyright (C) 1996-2025 Free Software Foundation, Inc.
 See the end for copying conditions.
 
 Please send Guile bug reports to bug-guile <at> gnu.org.
@@ -69,6 +69,12 @@ every line in a file.
 ** Immutable stringbufs are now 8-byte aligned on all systems
    Previously they could end up with an alignment that violated the type
    tag for their type (e.g. ending up tagged as immediates SCM_IMP()).
+** 'all-threads' no longer includes the finalizer thread
+   (<https://bugs.gnu.org/76343>)
+   Previously 'all-threads' would include the finalizer thread.  This,
+   in turn, would trigger warnings from 'primitive-fork' and 'environ'
+   suggesting they are being called in a multi-threaded context, when in
+   fact user code did not create any thread.
 
 
 Changes in 3.0.10 (since 3.0.9)
diff --git a/libguile/finalizers.c b/libguile/finalizers.c
index 1370755bf..47495b595 100644
--- a/libguile/finalizers.c
+++ b/libguile/finalizers.c
@@ -1,4 +1,4 @@
-/* Copyright 2012-2014,2018-2020,2022
+/* Copyright 2012-2014,2018-2020,2022,2025
      Free Software Foundation, Inc.
 
    This file is part of Guile.
@@ -37,6 +37,7 @@
 #include "gsubr.h"
 #include "init.h"
 #include "threads.h"
+#include "atomics-internal.h"
 
 #include "finalizers.h"
 
@@ -217,10 +218,14 @@ read_finalization_pipe_data (void *data)
 
   return NULL;
 }
-  
+
+static scm_i_pthread_t finalizer_thread;
+
 static void*
 finalization_thread_proc (void *unused)
 {
+  scm_atomic_set_pointer ((void **) &finalizer_thread,
+                          (void *) pthread_self ());
   while (1)
     {
       struct finalization_pipe_data data;
@@ -255,10 +260,20 @@ finalization_thread_proc (void *unused)
     }
 }
 
+int
+scm_i_is_finalizer_thread (struct scm_thread *t)
+{
+  scm_i_pthread_t us =
+    (scm_i_pthread_t) scm_atomic_ref_pointer ((void **) &finalizer_thread);
+  return pthread_equal (t->pthread, us);
+}
+
 static void*
 run_finalization_thread (void *arg)
 {
-  return scm_with_guile (finalization_thread_proc, arg);
+  void *res = scm_with_guile (finalization_thread_proc, arg);
+  scm_atomic_set_pointer ((void **) &finalizer_thread, NULL);
+  return res;
 }
 
 static void
diff --git a/libguile/finalizers.h b/libguile/finalizers.h
index 44bafb22e..a92a74be1 100644
--- a/libguile/finalizers.h
+++ b/libguile/finalizers.h
@@ -1,7 +1,7 @@
 #ifndef SCM_FINALIZERS_H
 #define SCM_FINALIZERS_H
 
-/* Copyright 2012, 2013, 2014, 2018
+/* Copyright 2012, 2013, 2014, 2018, 2025
      Free Software Foundation, Inc.
 
    This file is part of Guile.
@@ -42,6 +42,9 @@ SCM_INTERNAL void scm_i_finalizer_pre_fork (void);
    thread. */
 SCM_INTERNAL void scm_i_register_async_gc_callback (void (*callback) (void));
 
+/* Return true if THREAD is the finalizer thread.  */
+SCM_INTERNAL int scm_i_is_finalizer_thread (struct scm_thread *thread);
+
 SCM_API int scm_set_automatic_finalization_enabled (int enabled_p);
 SCM_API int scm_run_finalizers (void);
 
diff --git a/libguile/threads.c b/libguile/threads.c
index 77e99da74..6b4510d53 100644
--- a/libguile/threads.c
+++ b/libguile/threads.c
@@ -1,4 +1,4 @@
-/* Copyright 1995-1998,2000-2014,2018-2019,2023-2024
+/* Copyright 1995-1998,2000-2014,2018-2019,2023-2025
      Free Software Foundation, Inc.
 
    This file is part of Guile.
@@ -47,6 +47,7 @@
 #include "dynwind.h"
 #include "eval.h"
 #include "extensions.h"
+#include "finalizers.h"
 #include "fluids.h"
 #include "gc-inline.h"
 #include "gc.h"
@@ -1691,7 +1692,9 @@ SCM_DEFINE (scm_all_threads, "all-threads", 0, 0, 0,
 
   for (t = all_threads; t && n > 0; t = t->next_thread)
     {
-      if (!t->exited && !scm_i_is_signal_delivery_thread (t))
+      if (!t->exited
+          && !scm_i_is_signal_delivery_thread (t)
+          && !scm_i_is_finalizer_thread (t))
 	{
 	  SCM_SETCAR (*l, t->handle);
 	  l = SCM_CDRLOC (*l);

base-commit: f6359a4715d023761454f1bf945633ce4cca98fc
-- 
2.48.1

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

From: Olivier Dion <odion <at> efficios.com>
To: Ludovic Courtès <ludo <at> gnu.org>, 76589 <at> debbugs.gnu.org
Cc: Simon Josefsson <simon <at> josefsson.org>,
 Ludovic Courtès <ludo <at> gnu.org>
Subject: Re: bug#76589: [PATCH] Exclude the finalizer thread from the
 ‘all-threads’ result.
Date: Wed, 26 Feb 2025 10:43:05 -0500

I think that this change is fine, but it could be better when it comes
to testing for multi-threaded environment.

First, a Guile internal thread is leaked in the list of threads, which
users are not expecting.  Perhaps there's code out there that is
handling this fine but would break by removing the internal thread.
Should the internal thread not be leaked anymore?  I think that this
question is orthogonal to the problem that this patch aims to solve,
that is not emitting a false warning for `environ' and `primitive-fork'.

The problem could be solved by keeping an atomic counter of _user
created threads_.  Increment it when a new thread is created, decrement
it when a thread is joined.  Read the counter in a predicate
`multi-threaded?'.  This begs the question whether a multi-threaded
environment can become a single-threaded environment again.  If not,
then keeping a single boolean and storing true in it whenever a user
thread is created is enough.

Thanks,
Olivier

On Wed, 26 Feb 2025, Ludovic Courtès <ludo <at> gnu.org> wrote:
> Fixes <https://bugs.gnu.org/76343>.
>
> Fixes a bug whereby “echo '(environ)' | guile” would wrongfully trigger
> the multiple-thread warning.
>
> * libguile/finalizers.c (finalizer_thread): New variable.
> (finalization_thread_proc): Set it.
> (scm_i_is_finalizer_thread): New function.
> (run_finalization_thread): Clear FINALIZER_THREAD.
> * libguile/finalizers.h (scm_i_is_finalizer_thread): New declaration.
> * libguile/threads.c (scm_all_threads): Use it.
> * NEWS: Update.
>
> Reported-by: Simon Josefsson <simon <at> josefsson.org>
> ---
>  NEWS                  |  8 +++++++-
>  libguile/finalizers.c | 21 ++++++++++++++++++---
>  libguile/finalizers.h |  5 ++++-
>  libguile/threads.c    |  7 +++++--
>  4 files changed, 34 insertions(+), 7 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index 3328a03cf..2a59874d4 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -1,5 +1,5 @@
>  Guile NEWS --- history of user-visible changes.
> -Copyright (C) 1996-2024 Free Software Foundation, Inc.
> +Copyright (C) 1996-2025 Free Software Foundation, Inc.
>  See the end for copying conditions.
>  
>  Please send Guile bug reports to bug-guile <at> gnu.org.
> @@ -69,6 +69,12 @@ every line in a file.
>  ** Immutable stringbufs are now 8-byte aligned on all systems
>     Previously they could end up with an alignment that violated the type
>     tag for their type (e.g. ending up tagged as immediates SCM_IMP()).
> +** 'all-threads' no longer includes the finalizer thread
> +   (<https://bugs.gnu.org/76343>)
> +   Previously 'all-threads' would include the finalizer thread.  This,
> +   in turn, would trigger warnings from 'primitive-fork' and 'environ'
> +   suggesting they are being called in a multi-threaded context, when in
> +   fact user code did not create any thread.
>  
>  
>  Changes in 3.0.10 (since 3.0.9)
> diff --git a/libguile/finalizers.c b/libguile/finalizers.c
> index 1370755bf..47495b595 100644
> --- a/libguile/finalizers.c
> +++ b/libguile/finalizers.c
> @@ -1,4 +1,4 @@
> -/* Copyright 2012-2014,2018-2020,2022
> +/* Copyright 2012-2014,2018-2020,2022,2025
>       Free Software Foundation, Inc.
>  
>     This file is part of Guile.
> @@ -37,6 +37,7 @@
>  #include "gsubr.h"
>  #include "init.h"
>  #include "threads.h"
> +#include "atomics-internal.h"
>  
>  #include "finalizers.h"
>  
> @@ -217,10 +218,14 @@ read_finalization_pipe_data (void *data)
>  
>    return NULL;
>  }
> -  
> +
> +static scm_i_pthread_t finalizer_thread;
> +
>  static void*
>  finalization_thread_proc (void *unused)
>  {
> +  scm_atomic_set_pointer ((void **) &finalizer_thread,
> +                          (void *) pthread_self ());
>    while (1)
>      {
>        struct finalization_pipe_data data;
> @@ -255,10 +260,20 @@ finalization_thread_proc (void *unused)
>      }
>  }
>  
> +int
> +scm_i_is_finalizer_thread (struct scm_thread *t)
> +{
> +  scm_i_pthread_t us =
> +    (scm_i_pthread_t) scm_atomic_ref_pointer ((void **) &finalizer_thread);
> +  return pthread_equal (t->pthread, us);
> +}
> +
>  static void*
>  run_finalization_thread (void *arg)
>  {
> -  return scm_with_guile (finalization_thread_proc, arg);
> +  void *res = scm_with_guile (finalization_thread_proc, arg);
> +  scm_atomic_set_pointer ((void **) &finalizer_thread, NULL);
> +  return res;
>  }
>  
>  static void
> diff --git a/libguile/finalizers.h b/libguile/finalizers.h
> index 44bafb22e..a92a74be1 100644
> --- a/libguile/finalizers.h
> +++ b/libguile/finalizers.h
> @@ -1,7 +1,7 @@
>  #ifndef SCM_FINALIZERS_H
>  #define SCM_FINALIZERS_H
>  
> -/* Copyright 2012, 2013, 2014, 2018
> +/* Copyright 2012, 2013, 2014, 2018, 2025
>       Free Software Foundation, Inc.
>  
>     This file is part of Guile.
> @@ -42,6 +42,9 @@ SCM_INTERNAL void scm_i_finalizer_pre_fork (void);
>     thread. */
>  SCM_INTERNAL void scm_i_register_async_gc_callback (void (*callback) (void));
>  
> +/* Return true if THREAD is the finalizer thread.  */
> +SCM_INTERNAL int scm_i_is_finalizer_thread (struct scm_thread *thread);
> +
>  SCM_API int scm_set_automatic_finalization_enabled (int enabled_p);
>  SCM_API int scm_run_finalizers (void);
>  
> diff --git a/libguile/threads.c b/libguile/threads.c
> index 77e99da74..6b4510d53 100644
> --- a/libguile/threads.c
> +++ b/libguile/threads.c
> @@ -1,4 +1,4 @@
> -/* Copyright 1995-1998,2000-2014,2018-2019,2023-2024
> +/* Copyright 1995-1998,2000-2014,2018-2019,2023-2025
>       Free Software Foundation, Inc.
>  
>     This file is part of Guile.
> @@ -47,6 +47,7 @@
>  #include "dynwind.h"
>  #include "eval.h"
>  #include "extensions.h"
> +#include "finalizers.h"
>  #include "fluids.h"
>  #include "gc-inline.h"
>  #include "gc.h"
> @@ -1691,7 +1692,9 @@ SCM_DEFINE (scm_all_threads, "all-threads", 0, 0, 0,
>  
>    for (t = all_threads; t && n > 0; t = t->next_thread)
>      {
> -      if (!t->exited && !scm_i_is_signal_delivery_thread (t))
> +      if (!t->exited
> +          && !scm_i_is_signal_delivery_thread (t)
> +          && !scm_i_is_finalizer_thread (t))
>  	{
>  	  SCM_SETCAR (*l, t->handle);
>  	  l = SCM_CDRLOC (*l);
>
> base-commit: f6359a4715d023761454f1bf945633ce4cca98fc
> -- 
> 2.48.1
>
>
>
-- 
Olivier Dion
EfficiOS Inc.
https://www.efficios.com

Previous Next

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