Bug #1332: mem_node leak

There was a fairly significant mem_node leak because (1) nodes were
being removed from the 'nodes' list without also being deleted, and
(2) there was no storeIOWrite callback to decrement the use count.

This patch changes the mem_node use count to a boolean write_pending
flag, adds a memNodeWriteComplete() callback function, and removes
and deletes nodes at the same time.  mem_hdr::unlink() was changed
to return boolean to prevent loops in freeDataUpto().

diff --git a/src/mem_node.cc b/src/mem_node.cc
index 788309a8c405ea138c74610b5f68baa117f5d5e8..47baf4fe5f559d8d9665c905910a0edf89fc7841 100644 (file)
--- a/src/mem_node.cc
+++ b/src/mem_node.cc
@@ -1,6 +1,6 @@
 
 /*
- * $Id: mem_node.cc,v 1.6 2004/08/30 05:12:31 robertc Exp $
+ * $Id: mem_node.cc,v 1.7 2005/09/14 18:23:21 wessels Exp $
  *
  * DEBUG: section 19    Store Memory Primitives
  * AUTHOR: Robert Collins
@@ -36,7 +36,35 @@
 #include "squid.h"
 #include "mem_node.h"
 
+static int makeMemNodeDataOffset();
+
 unsigned long mem_node::store_mem_size;
+static int _mem_node_data_offset = makeMemNodeDataOffset();
+
+/*
+ * Calculate the offset between the start of a mem_node and
+ * its 'data' member
+ */
+static int
+makeMemNodeDataOffset()
+{
+    mem_node *p = 0L;
+    return int(&p->data);
+}
+
+/*
+ * This is the callback when storeIOWrite() is done.  We need to
+ * clear the write_pending flag for the mem_node.  First we have
+ * to calculate the start of the mem_node based on the character
+ * buffer that we wrote.  ick.
+ */
+void
+memNodeWriteComplete(void* d)
+{
+    mem_node* n = (mem_node*)((char*)d - _mem_node_data_offset);
+    assert(n->write_pending);
+    n->write_pending = 0;
+}
 
 mem_node::mem_node(off_t offset):nodeBuffer(0,offset,data)
 {}

diff --git a/src/mem_node.h b/src/mem_node.h
index 86f8f31d5e21c8a8d67c1d1ca111da8fc3d5ec92..3ef21ab63f09f075f7a6b1f099b114c1a9eff380 100644 (file)
--- a/src/mem_node.h
+++ b/src/mem_node.h
@@ -1,6 +1,6 @@
 
 /*
- * $Id: mem_node.h,v 1.8 2005/04/01 21:11:28 serassio Exp $
+ * $Id: mem_node.h,v 1.9 2005/09/14 18:23:21 wessels Exp $
  *
  *
  * SQUID Web Proxy Cache          http://www.squid-cache.org/
@@ -58,7 +58,9 @@ public:
     StoreIOBuffer nodeBuffer;
     /* Private */
     char data[SM_PAGE_SIZE];
-    int uses;
+
+unsigned int write_pending:
+    1;
 };
 
 MEMPROXY_CLASS_INLINE(mem_node)
@@ -70,4 +72,6 @@ operator << (std::ostream &os, mem_node &aNode)
     return os;
 }
 
+void memNodeWriteComplete(void *);
+
 #endif /* SQUID_MEM_NODE_H */

diff --git a/src/stmem.cc b/src/stmem.cc
index dc5c8ed2377c31be62315f5b701aadca5ab64c2e..be352e4bf3f2cded9610b8c0245e98f82f587ed6 100644 (file)
--- a/src/stmem.cc
+++ b/src/stmem.cc
@@ -1,6 +1,6 @@
 
 /*
- * $Id: stmem.cc,v 1.88 2005/04/01 21:11:28 serassio Exp $
+ * $Id: stmem.cc,v 1.89 2005/09/14 18:23:21 wessels Exp $
  *
  * DEBUG: section 19    Store Memory Primitives
  * AUTHOR: Harvest Derived
@@ -40,10 +40,18 @@
 #include "MemObject.h"
 #include "Generic.h"
 
+/*
+ * NodeGet() is called to get the data buffer to pass to storeIOWrite().
+ * By setting the write_pending flag here we are assuming that there
+ * will be no other users of NodeGet().  The storeIOWrite() callback
+ * is memNodeWriteComplete(), which, for whatever reason, lives in
+ * mem_node.cc.
+ */
 char *
 mem_hdr::NodeGet(mem_node * aNode)
 {
-    aNode->uses++;
+    assert(!aNode->write_pending);
+    aNode->write_pending = 1;
     return aNode->data;
 }
 
@@ -79,16 +87,17 @@ mem_hdr::freeContent()
     inmem_hi = 0;
 }
 
-void
+bool
 mem_hdr::unlink(mem_node *aNode)
 {
-    nodes.remove (aNode, NodeCompare);
-
-    if (!aNode->uses)
-        delete aNode;
-    else
-        aNode->uses--;
+    if (aNode->write_pending) {
+        debug(0,0)("cannot unlink mem_node %p while write_pending\n", aNode);
+        return false;
+    }
 
+    nodes.remove (aNode, NodeCompare);
+    delete aNode;
+    return true;
 }
 
 int
@@ -98,10 +107,15 @@ mem_hdr::freeDataUpto(int target_offset)
 
     SplayNode<mem_node*> const * theStart = nodes.start();
 
-    while (theStart && theStart != nodes.finish() &&
-            theStart->data->end() <= (size_t) target_offset ) {
-        unlink(theStart->data);
-        theStart = nodes.start();
+    while ((theStart = nodes.start())) {
+        if (theStart == nodes.finish())
+            break;
+
+        if (theStart->data->end() > (size_t) target_offset )
+            break;
+
+        if (!unlink(theStart->data))
+            break;
     }
 
     assert (lowestOffset () <= target_offset);

diff --git a/src/stmem.h b/src/stmem.h
index 04a8875d0eb8d0733c60e7fedd4237eff60a189a..3624ae283e1d92e23816c35ccc28b30230ef8b4a 100644 (file)
--- a/src/stmem.h
+++ b/src/stmem.h
@@ -1,6 +1,6 @@
 
 /*
- * $Id: stmem.h,v 1.8 2005/04/01 21:11:28 serassio Exp $
+ * $Id: stmem.h,v 1.9 2005/09/14 18:23:21 wessels Exp $
  *
  *
  * SQUID Web Proxy Cache          http://www.squid-cache.org/
@@ -74,7 +74,7 @@ public:
 
 private:
     void debugDump() const;
-    void unlink(mem_node *aNode);
+    bool unlink(mem_node *aNode);
     void makeAppendSpace();
     int appendToNode(mem_node *aNode, const char *data, int maxLength);
     void appendNode (mem_node *aNode);

diff --git a/src/store_swapout.cc b/src/store_swapout.cc
index 1137e2317e6bd0b7eede1f5d91c5f5d0f6eb7a28..d52cc66bfb48887c8bca1872138984eb1f9048c1 100644 (file)
--- a/src/store_swapout.cc
+++ b/src/store_swapout.cc
@@ -1,6 +1,6 @@
 
 /*
- * $Id: store_swapout.cc,v 1.100 2005/04/01 21:11:28 serassio Exp $
+ * $Id: store_swapout.cc,v 1.101 2005/09/14 18:23:21 wessels Exp $
  *
  * DEBUG: section 20    Storage Manager Swapout Functions
  * AUTHOR: Duane Wessels
@@ -148,7 +148,7 @@ doPages(StoreEntry *anEntry)
 
         mem->swapout.queue_offset += swap_buf_len;
 
-        storeIOWrite(mem->swapout.sio, mem->data_hdr.NodeGet(mem->swapout.memnode), swap_buf_len, -1, NULL);
+        storeIOWrite(mem->swapout.sio, mem->data_hdr.NodeGet(mem->swapout.memnode), swap_buf_len, -1, memNodeWriteComplete);
 
         /* the storeWrite() call might generate an error */
         if (anEntry->swap_status != SWAPOUT_WRITING)