Fix hashtable memory leak and kvstore overhead_hashtable_lut assert (#1750)

When releasing a kvstore and its hashtables, if it is still rehashed and
table 0 has no data, it may still have allocated buckets that were not
released. This lead to allocated hashtable buckets being leaked. This
looked like missed statistics but it was actually a memory leak.

fix: #1657

---------

Signed-off-by: artikell <[email protected]>
Co-authored-by: Viktor Söderqvist <[email protected]>

Author: artikell

src/hashtable.c

@@ -1032,7 +1032,7 @@ void hashtableEmpty(hashtable *ht, void(callback)(hashtable *)) {
         if (ht->bucket_exp[table_index] < 0) {
             continue;
         }
-        if (ht->used[table_index] > 0) {
+        if (ht->used[table_index] > 0 || ht->child_buckets[table_index] > 0) {
             for (size_t idx = 0; idx < numBuckets(ht->bucket_exp[table_index]); idx++) {
                 if (callback && (idx & 65535) == 0) callback(ht);
                 bucket *b = &ht->tables[table_index][idx];
@@ -1058,6 +1058,7 @@ void hashtableEmpty(hashtable *ht, void(callback)(hashtable *)) {
                 } while (b != NULL);
             }
         }
+
         zfree(ht->tables[table_index]);
         if (ht->type->trackMemUsage) {
             ht->type->trackMemUsage(ht, -sizeof(bucket) * numBuckets(ht->bucket_exp[table_index]));

src/kvstore.c

@@ -319,8 +319,7 @@ void kvstoreRelease(kvstore *kvs) {
         if (metadata->rehashing_node) metadata->rehashing_node = NULL;
         hashtableRelease(ht);
     }
-    /* TODO: The assert below causes a flaky unit test. Find out why. */
-    /* assert(kvs->overhead_hashtable_lut == 0); */
+    assert(kvs->overhead_hashtable_lut == 0);
     zfree(kvs->hashtables);
 
     listRelease(kvs->rehashing);

src/unit/test_kvstore.c

@@ -5,6 +5,11 @@ uint64_t hashTestCallback(const void *key) {
     return hashtableGenHashFunction((char *)key, strlen((char *)key));
 }
 
+uint64_t hashConflictTestCallback(const void *key) {
+    UNUSED(key);
+    return 0;
+}
+
 int cmpTestCallback(const void *k1, const void *k2) {
     return strcmp(k1, k2);
 }
@@ -23,6 +28,16 @@ hashtableType KvstoreHashtableTestType = {
     .getMetadataSize = kvstoreHashtableMetadataSize,
 };
 
+hashtableType KvstoreConflictHashtableTestType = {
+    .hashFunction = hashConflictTestCallback,
+    .keyCompare = cmpTestCallback,
+    .entryDestructor = freeTestCallback,
+    .rehashingStarted = kvstoreHashtableRehashingStarted,
+    .rehashingCompleted = kvstoreHashtableRehashingCompleted,
+    .trackMemUsage = kvstoreHashtableTrackMemUsage,
+    .getMetadataSize = kvstoreHashtableMetadataSize,
+};
+
 char *stringFromInt(int value) {
     char buf[32];
     int len;
@@ -70,31 +85,43 @@ int test_kvstoreIteratorRemoveAllKeysNoDeleteEmptyHashtable(int argc, char **arg
     UNUSED(argv);
     UNUSED(flags);
 
-    int i;
-    void *key;
-    kvstoreIterator *kvs_it;
-
-    int didx = 0;
-    int curr_slot = 0;
-    kvstore *kvs1 = kvstoreCreate(&KvstoreHashtableTestType, 0, KVSTORE_ALLOCATE_HASHTABLES_ON_DEMAND);
-
-    for (i = 0; i < 16; i++) {
-        TEST_ASSERT(kvstoreHashtableAdd(kvs1, didx, stringFromInt(i)));
-    }
-
-    kvs_it = kvstoreIteratorInit(kvs1, HASHTABLE_ITER_SAFE);
-    while (kvstoreIteratorNext(kvs_it, &key)) {
-        curr_slot = kvstoreIteratorGetCurrentHashtableIndex(kvs_it);
-        TEST_ASSERT(kvstoreHashtableDelete(kvs1, curr_slot, key));
+    hashtableType *type[] = {
+        &KvstoreHashtableTestType,
+        &KvstoreConflictHashtableTestType,
+        NULL,
+    };
+
+    for (int t = 0; type[t] != NULL; t++) {
+        hashtableType *testType = type[t];
+        TEST_PRINT_INFO("Testing %d hashtableType\n", t);
+
+        int i;
+        void *key;
+        kvstoreIterator *kvs_it;
+
+        int didx = 0;
+        int curr_slot = 0;
+        kvstore *kvs1 = kvstoreCreate(testType, 0, KVSTORE_ALLOCATE_HASHTABLES_ON_DEMAND);
+
+        for (i = 0; i < 16; i++) {
+            TEST_ASSERT(kvstoreHashtableAdd(kvs1, didx, stringFromInt(i)));
+        }
+
+        kvs_it = kvstoreIteratorInit(kvs1, HASHTABLE_ITER_SAFE);
+        while (kvstoreIteratorNext(kvs_it, &key)) {
+            curr_slot = kvstoreIteratorGetCurrentHashtableIndex(kvs_it);
+            TEST_ASSERT(kvstoreHashtableDelete(kvs1, curr_slot, key));
+        }
+        kvstoreIteratorRelease(kvs_it);
+
+        hashtable *ht = kvstoreGetHashtable(kvs1, didx);
+        TEST_ASSERT(ht != NULL);
+        TEST_ASSERT(kvstoreHashtableSize(kvs1, didx) == 0);
+        TEST_ASSERT(kvstoreSize(kvs1) == 0);
+
+        kvstoreRelease(kvs1);
     }
-    kvstoreIteratorRelease(kvs_it);
-
-    hashtable *ht = kvstoreGetHashtable(kvs1, didx);
-    TEST_ASSERT(ht != NULL);
-    TEST_ASSERT(kvstoreHashtableSize(kvs1, didx) == 0);
-    TEST_ASSERT(kvstoreSize(kvs1) == 0);
 
-    kvstoreRelease(kvs1);
     return 0;
 }