[squid-dev] [PATCH] helpers queue update

[Amos Jeffries]

This is a hopefully minor update to the helper lookup queueing.

It removes the only use of MEM_DLINK_NODE for custom link-list
implementation and replaces it all with a std::queue.

Also, de-duplicates the *Dequeue() functions by merging them into helper
class as a single nextRequest() getter method.

This has had some basic testing and seems to work so far. Though I have
added one TODO where I think we have some potential for a memory leak
and/or hung transactions if helpers are constantly dying but not also
killing Squid.

Amos
-------------- next part --------------
=== modified file 'src/helper.cc'
--- src/helper.cc	2016-01-01 00:12:18 +0000
+++ src/helper.cc	2016-04-22 20:07:50 +0000
@@ -54,8 +54,6 @@
 static void helperServerFree(helper_server *srv);
 static void helperStatefulServerFree(helper_stateful_server *srv);
 static void Enqueue(helper * hlp, Helper::Request *);
-static Helper::Request *Dequeue(helper * hlp);
-static Helper::Request *StatefulDequeue(statefulhelper * hlp);
 static helper_server *GetFirstAvailable(helper * hlp);
 static helper_stateful_server *StatefulGetFirstAvailable(statefulhelper * hlp);
 static void helperDispatch(helper_server * srv, Helper::Request * r);
@@ -667,7 +665,8 @@
 {
     /* note, don't free id_name, it probably points to static memory */
 
-    if (queue.head)
+    // TODO: if the queue is not empty it will leak Helper::Request's
+    if (!queue.empty())
         debugs(84, DBG_CRITICAL, "WARNING: freeing " << id_name << " helper with " << stats.queue_size << " requests queued");
 }
 
@@ -1102,8 +1101,7 @@
 static void
 Enqueue(helper * hlp, Helper::Request * r)
 {
-    dlink_node *link = (dlink_node *)memAllocate(MEM_DLINK_NODE);
-    dlinkAddTail(r, link, &hlp->queue);
+    hlp->queue.push(r);
     ++ hlp->stats.queue_size;
 
     /* do this first so idle=N has a chance to grow the child pool before it hits critical. */
@@ -1132,8 +1130,7 @@
 static void
 StatefulEnqueue(statefulhelper * hlp, Helper::Request * r)
 {
-    dlink_node *link = (dlink_node *)memAllocate(MEM_DLINK_NODE);
-    dlinkAddTail(r, link, &hlp->queue);
+    hlp->queue.push(r);
     ++ hlp->stats.queue_size;
 
     /* do this first so idle=N has a chance to grow the child pool before it hits critical. */
@@ -1159,35 +1156,15 @@
     debugs(84, DBG_CRITICAL, "WARNING: Consider increasing the number of " << hlp->id_name << " processes in your config file.");
 }
 
-static Helper::Request *
-Dequeue(helper * hlp)
-{
-    dlink_node *link;
-    Helper::Request *r = NULL;
-
-    if ((link = hlp->queue.head)) {
-        r = (Helper::Request *)link->data;
-        dlinkDelete(link, &hlp->queue);
-        memFree(link, MEM_DLINK_NODE);
-        -- hlp->stats.queue_size;
-    }
-
-    return r;
-}
-
-static Helper::Request *
-StatefulDequeue(statefulhelper * hlp)
-{
-    dlink_node *link;
-    Helper::Request *r = NULL;
-
-    if ((link = hlp->queue.head)) {
-        r = (Helper::Request *)link->data;
-        dlinkDelete(link, &hlp->queue);
-        memFree(link, MEM_DLINK_NODE);
-        -- hlp->stats.queue_size;
-    }
-
+Helper::Request *
+helper::nextRequest()
+{
+    if (queue.empty())
+        return nullptr;
+
+    auto *r = queue.front();
+    queue.pop();
+    --stats.queue_size;
     return r;
 }
 
@@ -1394,7 +1371,7 @@
     Helper::Request *r;
     helper_server *srv;
 
-    while ((srv = GetFirstAvailable(hlp)) && (r = Dequeue(hlp)))
+    while ((srv = GetFirstAvailable(hlp)) && (r = hlp->nextRequest()))
         helperDispatch(srv, r);
 }
 
@@ -1404,7 +1381,7 @@
     Helper::Request *r;
     helper_stateful_server *srv;
 
-    while ((srv = StatefulGetFirstAvailable(hlp)) && (r = StatefulDequeue(hlp)))
+    while ((srv = StatefulGetFirstAvailable(hlp)) && (r = hlp->nextRequest()))
         helperStatefulDispatch(srv, r);
 }
 

=== modified file 'src/helper.h'
--- src/helper.h	2016-02-23 08:51:22 +0000
+++ src/helper.h	2016-04-23 03:08:48 +0000
@@ -23,6 +23,7 @@
 
 #include <list>
 #include <map>
+#include <queue>
 
 class Packable;
 class wordlist;
@@ -62,9 +63,12 @@
     }
     ~helper();
 
-    ///< whether at least one more request can be successfully submitted
+    /// whether at least one more request can be successfully submitted
     bool queueFull() const;
 
+    /// \returns next request in the queue, or nil.
+    Helper::Request *nextRequest();
+
     ///< If not full, submit request. Otherwise, either kill Squid or return false.
     bool trySubmit(const char *buf, HLPCB * callback, void *data);
 
@@ -78,7 +82,7 @@
 public:
     wordlist *cmdline;
     dlink_list servers;
-    dlink_list queue;
+    std::queue<Helper::Request *> queue;
     const char *id_name;
     Helper::ChildConfig childs;    ///< Configuration settings for number running.
     int ipc_type;

=== modified file 'src/mem/forward.h'
--- src/mem/forward.h	2016-04-22 19:16:04 +0000
+++ src/mem/forward.h	2016-04-22 19:55:34 +0000
@@ -46,7 +46,6 @@
     MEM_64K_BUF,
     MEM_ACL_DENY_INFO_LIST,
     MEM_ACL_NAME_LIST,
-    MEM_DLINK_NODE,
     MEM_DREAD_CTRL,
     MEM_DWRITE_Q,
     MEM_MD5_DIGEST,

=== modified file 'src/mem/old_api.cc'
--- src/mem/old_api.cc	2016-04-22 19:16:04 +0000
+++ src/mem/old_api.cc	2016-04-22 19:55:37 +0000
@@ -444,7 +444,6 @@
     memDataInit(MEM_ACL_DENY_INFO_LIST, "AclDenyInfoList",
                 sizeof(AclDenyInfoList), 0);
     memDataInit(MEM_ACL_NAME_LIST, "acl_name_list", sizeof(AclNameList), 0);
-    memDataInit(MEM_DLINK_NODE, "dlink_node", sizeof(dlink_node), 10);
     memDataInit(MEM_DREAD_CTRL, "dread_ctrl", sizeof(dread_ctrl), 0);
     memDataInit(MEM_DWRITE_Q, "dwrite_q", sizeof(dwrite_q), 0);
     memDataInit(MEM_NETDBENTRY, "netdbEntry", sizeof(netdbEntry), 0);