bug#40194: Weak sets keep growing

Hi Olivier,

Olivier Dion <[email protected]> skribis:

> From 2adaf872aeb3b84be9cf8af06f958ea4b731f66c Mon Sep 17 00:00:00 2001
> From: Olivier Dion <[email protected]>
> Date: Tue, 17 Feb 2026 11:37:35 -0500
> Subject: [PATCH] Fix weak sets not shrinking after GC.
>=20
> The `scm_i_register_async_gc_callback' mechanism used finalizers to
> trigger post-GC callbacks for vacuuming weak sets and tables.  This
> approach stopped working reliably, causing weak sets to grow
> indefinitely even after GC.
>=20
> Replace the finalizer-based callback mechanism with direct integration
> into `scm_after_gc_c_hook'.  A new hook callback,
> `scm_i_notify_finalizer_thread', is registered with `scm_after_gc_c_hook'.
> When the hook runs after GC, this callback either notifies the finalizer
> thread (if running) to invoke finalizers, or runs them synchronously and
> spawns the finalizer thread if allowed.  The vacuum functions for weak
> sets and tables are called from `scm_run_finalizers'.
>=20
> When `(gc)' is called explicitly from Scheme, finalizers are run
> immediately.  An atomic flag prevents double-vacuuming when the GC hook
> fires afterward.
>=20
> Fixes <https://issues.guix.gnu.org/40194>.
>=20
> * libguile/atomics-internal.h (scm_atomic_swap_pointer): New function.
> * libguile/finalizers.c: Include weak-set.h and weak-table.h.
> (queue_finalizer_async): Remove.
> (async_gc_finalizer): Remove.
> (scm_i_register_async_gc_callback): Remove.
> (allow_finalizer_thread): New static variable.
> (scm_i_finalizer_pre_fork): Use `allow_finalizer_thread' instead of
> `GC_set_finalizer_notifier'.
> (scm_i_notify_finalizer_thread): New function.
> (scm_set_automatic_finalization_enabled): Use `scm_after_gc_c_hook'
> instead of `GC_set_finalizer_notifier'.
> (vacuum_off): New static variable.
> (scm_run_finalizers): Call `scm_i_vacuum_weak_sets' and
> `scm_i_vacuum_weak_tables' using atomic flag to prevent double-vacuuming.
> (scm_i_run_finalizers_and_turnoff_vacuums): New function.
> (scm_init_finalizers): Use `scm_after_gc_c_hook'.
> (scm_init_finalizer_thread): Simplify to just set `allow_finalizer_thread=
'.
> * libguile/finalizers.h (scm_i_register_async_gc_callback): Remove.
> (scm_i_run_finalizers_and_turnoff_vacuums): New declaration.
> * libguile/gc.c: Include finalizers.h.
> (scm_gc): Call `scm_i_run_finalizers_and_turnoff_vacuums' instead of
> `GC_invoke_finalizers'.
> (scm_init_gc): Disable `GC_set_finalizer_notifier'.
> * libguile/weak-set.c (scm_i_vacuum_weak_sets): Rename from
> `vacuum_all_weak_sets' and make non-static.
> (scm_init_weak_set): Remove `scm_i_register_async_gc_callback' call.
> * libguile/weak-set.h (scm_i_vacuum_weak_sets): New declaration.
> * libguile/weak-table.c (scm_i_vacuum_weak_tables): Rename from
> `vacuum_all_weak_tables' and make non-static.
> (scm_init_weak_table): Remove `scm_i_register_async_gc_callback' call.
> * libguile/weak-table.h (scm_i_vacuum_weak_tables): New declaration.
>=20
> Signed-off-by: Olivier Dion <[email protected]>

I totally lost track of the issue bug I=E2=80=99m glad you=E2=80=99re worki=
ng on it.

I only had a quick look but the fix and its explanation make sense to
me.  Do you have ideas though as to why and when the previous approach
=E2=80=9Cstopped working reliably=E2=80=9D as you wrote?

Do you have tests showing that this patch does indeed fix the problem?

This is a quite sensitive piece of code so I=E2=80=99m worried about possib=
le
issues we did not anticipate. :-)

Thanks,
Ludo=E2=80=99.