From b40fcea2c2f0f12d0ad73a911a44fe0d7d6b58cb Mon Sep 17 00:00:00 2001
From: Alexander Rosenberg <zanderpkg@pm.me>
Date: Sun, 7 Sep 2025 15:51:36 -0700
Subject: [PATCH] Make sure that destructors are called when objects are
 garbage collected

---
 src/refcount.c       | 65 ++++++++++++++++++++++++++++++++++++++++++--
 test/test_refcount.c |  7 ++++-
 2 files changed, 69 insertions(+), 3 deletions(-)

diff --git a/src/refcount.c b/src/refcount.c
index db604d6..212744f 100644
--- a/src/refcount.c
+++ b/src/refcount.c
@@ -588,6 +588,60 @@ static bool free_roots_foreach(void *obj, void *ignored, void *user_data) {
     return false;
 }
 
+/**
+ * @struct ContexAndFlag
+ * #RefcountContext and a boolean flag.
+ */
+struct ContextAndFlag {
+    const RefcountContext *ctx; //!< The context.
+    bool flag; //!< The flag.
+};
+
+/**
+ * Call the destructors for an object. Used from #call_destructors_for_gc.
+ * @param obj The object
+ * @param ignored Ignored
+ * @param ctx_raw A #ContexAndFlag. The flag is set to the same value as the
+ * return value.
+ * @return True if the object's reference count increased above 0
+ */
+static bool call_destructors_for_gc_callback(void *obj, void *ignored,
+                                             void *ctx_and_flag_raw) {
+    struct ContextAndFlag *ctx_and_flag = ctx_and_flag_raw;
+    const RefcountContext *ctx = ctx_and_flag->ctx;
+    uint64_t old_ref_count = ENTRY->impl.counted.ref_count;
+    ENTRY->impl.counted.ref_count = 0;
+    call_object_destructors(ctx, obj);
+    ctx_and_flag->flag = ENTRY->impl.counted.ref_count;
+    if (!ENTRY->impl.counted.ref_count) {
+        // the object can still be saved by another object in the reference loop
+        // having its refcount incremented. Therefore, we restore the original
+        // reference count.
+        ENTRY->impl.counted.ref_count = old_ref_count;
+    }
+    return ctx_and_flag->flag;
+}
+
+/**
+ * Call destructors for a hash table where the keys are objects. Stop if any
+ * object had it's reference count increase past 0.
+ *
+ * > Calling this will set the reference count of all objects in counts to 0!
+ *
+ * @param ctx The #RefcountContext
+ * @param counts The table for which to call destructors
+ * @return True if an object had its reference count increase above 0.
+ */
+static bool call_destructors_for_gc(const RefcountContext *ctx,
+                                    HTTable *counts) {
+    struct ContextAndFlag ctx_and_flag = {
+        .ctx = ctx,
+        .flag = false,
+    };
+    ht_foreach(counts, call_destructors_for_gc_callback, &ctx_and_flag);
+    return ctx_and_flag.flag;
+}
+
 /**
  * Check the root pointed to by the double pointer root_ptr. After the call,
  * root_ptr is set to the next root to be checked.
@@ -634,18 +688,25 @@ static ptrdiff_t check_gc_root(RefcountContext *ctx, RefcountList **root_ptr) {
             obj_held_refs(ctx, obj, &queue);
         }
     }
-    if (seen_objects == clear_objects) {
+    ptrdiff_t freed_count = 0;
+    if (seen_objects == clear_objects
+        && !call_destructors_for_gc(ctx, counts)) {
+        // all objects still have a refcount of zero, even after calling
+        // destructors, proceed with freeing them
         struct ContextAndRootPtr data = {
             .ctx = ctx, .root_ptr = root_ptr, .did_update = false};
         ht_foreach(counts, free_roots_foreach, &data);
         if (!data.did_update) {
             *root_ptr = (*root_ptr)->next;
         }
+        freed_count = seen_objects;
     } else {
+        // either something still had a reference, or it was re-refed by its
+        // destructor. Either way, don't free anything
         *root_ptr = (*root_ptr)->next;
     }
     ht_free(counts);
-    return seen_objects == clear_objects ? seen_objects : 0;
+    return freed_count;
 }
 
 /**
diff --git a/test/test_refcount.c b/test/test_refcount.c
index 4b3ce86..fbd4b42 100644
--- a/test/test_refcount.c
+++ b/test/test_refcount.c
@@ -114,6 +114,9 @@ int main(int argc, const char **argv) {
     refcount_context_ref(c, a);
     refcount_context_unref(c, a);
     assert(refcount_context_num_refs(c, a) == 1);
+    int key;
+    A *a_with_destructor = a;
+    assert(refcount_context_add_destructor(c, a, &key, reref_destructor, c));
 
     a = NULL;
     for (char i = 'a'; i <= 'z'; ++i) {
@@ -124,6 +127,9 @@ int main(int argc, const char **argv) {
     assert(refcount_context_num_refs(c, a) == 0);
     refcount_context_unref(c, a);
 
+    assert(refcount_context_garbage_collect(c) == 0);
+    assert(refcount_context_num_refs(c, a_with_destructor) == 1);
+    assert(refcount_context_remove_destructor(c, a_with_destructor, &key));
     assert(refcount_context_garbage_collect(c) == 26);
 
     a = make_a(c, 0, "a");
@@ -162,7 +168,6 @@ int main(int argc, const char **argv) {
 
     a = make_a(c, 10, "test destructor");
     assert(refcount_context_num_refs(c, a) == 0);
-    int key;
     assert(refcount_context_add_destructor(c, a, &key, reref_destructor, c));
     assert(refcount_context_num_refs(c, a) == 0);
     assert(!refcount_context_unref(c, a));