[Spread-users] Addressing the skiplist leak.

[Theo Schlossnagle]

Hi all,

Below is a diff against HEAD to fix the skiplist index leak...  It 
should be noted that modifying sl_remove_all to free the sl->index 
isn't a workable solution.  sl_remove_all is used legitimately in the 
groups.c file to clear the contents of a list for reuse -- so dropping 
the sl->index would be disastrous (it would cause the secondary index 
to be dropped and queries to return invalid answers).

So, this was a quick patch and I don't have a set of machines handy to 
test it on -- it needs a cluster to exercise most of the code-paths in 
the groups.c file.  Ideally, the dispose function should call: 
sl_destruct( &obj->MembersList, dispose ) on objects of type GROUP.  
However, I don't really know if that sort of custom disposal is 
possible in the memory system.

So, below is an attempt to carefully dispose of the skiplists in groups 
that have been created immediately each place they are disposed of.  
This has more room for human error than the first approach and must be 
minded for future disposals of GROUP objects.

Somone who knows the memory system better than I is welcome to migrate 
the sl_destruct of MembersList into the "dispose" operation on GROUP 
objects to make this intuitive.

And... as I haven't tested this except on one machine -- so I haven't 
tested it at all really.  It should be put through some testing before 
it is used anywhere important.

// Theo Schlossnagle
// Principal Engineer -- http://www.omniti.com/~jesus/
// Postal Engine -- http://www.postalengine.com/
// Ecelerity: fastest MTA on Earth


Index: groups.c
===================================================================
RCS file: /storage/cvsroot/spread/daemon/groups.c,v
retrieving revision 1.14
diff -u -r1.14 groups.c
--- groups.c    13 Feb 2004 16:12:14 -0000      1.14
+++ groups.c    22 Feb 2004 03:05:20 -0000
@@ -341,6 +341,7 @@
                              if( grp->num_members == 0 )
                             {
                                 /* discard this empty group */
+                                sl_destruct ( &grp->MembersList, 
dispose);
                                  sl_remove (  &GroupsList, grp->name, 
dispose);
                                 Num_groups--;
                                 GlobalStatus.num_groups = Num_groups;
@@ -417,6 +418,7 @@
                             if( grp->num_members == 0 )
                             {
                                 /* discard this empty group */
+                                sl_destruct ( &grp->MembersList, 
dispose);
                                  sl_remove (  &GroupsList, grp->name, 
dispose);
                                 Num_groups--;
                                 GlobalStatus.num_groups = Num_groups;
@@ -658,6 +660,7 @@
                         if( grp->num_members == 0 )
                         {
                                 /* discard this empty group */
+                                sl_destruct ( &grp->MembersList, 
dispose);
                                 sl_remove ( &GroupsList, grp->name, 
dispose);
                                 Num_groups--;
                                 GlobalStatus.num_groups = Num_groups;
@@ -1117,6 +1120,7 @@
                 if( grp->num_members == 0 )
                 {
                         /* discard this empty group */
+                        sl_destruct ( &grp->MembersList, dispose );
                         sl_remove( &GroupsList, grp->name, dispose );
                         Num_groups--;
                         GlobalStatus.num_groups = Num_groups;
@@ -1286,6 +1290,7 @@
                         grp->num_members--;
                         if( grp->num_members == 0 )
                         {
+                                sl_destruct ( &grp->MembersList, 
dispose );
                                  sl_remove( &GroupsList, grp->name, 
dispose );
                                 Num_groups--;
                                 GlobalStatus.num_groups = Num_groups;
@@ -1611,6 +1616,7 @@
                   orig_grp->num_members = orig_grp->MembersList.size;

                         /* free this Work group */
+                  sl_destruct(&currentgroup->MembersList, dispose);
                   sl_remove(indices[i]->groups, currentgroup, dispose);
              }

@@ -1692,7 +1698,7 @@
                 }
         }

-       sl_remove_all( &work, dispose );
+       sl_destruct( &work, dispose );

         G_print();
  }
Index: skiplist.c
===================================================================
RCS file: /storage/cvsroot/spread/daemon/skiplist.c,v
retrieving revision 1.3
diff -u -r1.3 skiplist.c
--- skiplist.c  28 Jan 2003 20:15:51 -0000      1.3
+++ skiplist.c  22 Feb 2004 03:05:21 -0000
@@ -555,3 +555,15 @@
    sl->height = 0;
    sl->size = 0;
  }
+void sli_destruct_free(Skiplist *sl, FreeFunc myfree) {
+  sl_remove_all(sl, NULL);
+  free(sl);
+}
+void sl_destruct(Skiplist *sl, FreeFunc myfree) {
+  if(sl->index) {
+    sl_remove_all(sl->index, (FreeFunc)sli_destruct_free);
+    free(sl->index);
+  }
+  sl_remove_all(sl, myfree);
+}
+
Index: skiplist.h
===================================================================
RCS file: /storage/cvsroot/spread/daemon/skiplist.h,v
retrieving revision 1.2
diff -u -r1.2 skiplist.h
--- skiplist.h  22 Sep 2002 02:56:52 -0000      1.2
+++ skiplist.h  22 Feb 2004 03:05:21 -0000
@@ -110,10 +110,13 @@
  int sl_remove_compare(Skiplist *sl, void *data, FreeFunc myfree,
                       SkiplistComparator comp);

-/* removes all nodes in a skiplist, you can free the skiplist itself
-   without memory leaks after calling this function */
+/* removes all nodes in a skiplist, the list can still be used */
  /* sl is the skiplist from which you are removing */
  void sl_remove_all(Skiplist *sl, FreeFunc myfree);

+/* removes all nodes in a skiplist, you can free the skiplist itself
+   without memory leaks after calling this function.  After calling
+   this function, the list cannot be safely used, it must be freed */
+void sl_destruct(Skiplist *sl, FreeFunc myfree);

  #endif