Author: Henrik Nordstrom <henrik@henriknordstrom.net>

Remove support for deferred state in stateful helpers

the deferred state were previously used for the challenge reuse mode
in NTLM, but is since long unused as it was both incompatible with
NTLMv2 and also not very stable for authentication.

diff --git a/src/enums.h b/src/enums.h
index 674807913203fa619d0108f248f81932cd2495fe..d43cd53f6247f76c13bf0d9b7165bd5f64c48aaf 100755 (executable)
--- a/src/enums.h
+++ b/src/enums.h
@@ -374,18 +374,9 @@ typedef enum {
 typedef enum {
     S_HELPER_UNKNOWN,
     S_HELPER_RESERVE,
-    S_HELPER_RELEASE,
-    S_HELPER_DEFER
+    S_HELPER_RELEASE
 } stateful_helper_callback_t;
 
-/* stateful helper reservation info */
-typedef enum {
-    S_HELPER_FREE,             /* available for requests */
-    S_HELPER_RESERVED,         /* in a reserved state - no active request, but state data in the helper shouldn't be disturbed */
-    S_HELPER_DEFERRED          /* available for requests, and at least one more will come from a previous caller with the server pointer */
-} stateful_helper_reserve_t;
-
-
 #if SQUID_SNMP
 enum {
     SNMP_C_VIEW,

diff --git a/src/helper.cc b/src/helper.cc
index aaeeb1d60c712bfb5fabfbb1d3240b6b74588d2a..959edaf6edc88816d019b30c0e93889516f713d9 100644 (file)
--- a/src/helper.cc
+++ b/src/helper.cc
@@ -56,12 +56,10 @@ static void helperDispatch(helper_server * srv, helper_request * r);
 static void helperStatefulDispatch(helper_stateful_server * srv, helper_stateful_request * r);
 static void helperKickQueue(helper * hlp);
 static void helperStatefulKickQueue(statefulhelper * hlp);
+static void helperStatefulServerDone(helper_stateful_server * srv);
 static void helperRequestFree(helper_request * r);
 static void helperStatefulRequestFree(helper_stateful_request * r);
 static void StatefulEnqueue(statefulhelper * hlp, helper_stateful_request * r);
-static helper_stateful_request *StatefulServerDequeue(helper_stateful_server * srv);
-static void StatefulServerEnqueue(helper_stateful_server * srv, helper_stateful_request * r);
-static void helperStatefulServerKickQueue(helper_stateful_server * srv);
 static bool helperStartStats(StoreEntry *sentry, void *hlp, const char *label);
 
 
@@ -249,10 +247,7 @@ helperStatefulOpenServers(statefulhelper * hlp)
         helper_stateful_server *srv = cbdataAlloc(helper_stateful_server);
         srv->hIpc = hIpc;
         srv->pid = pid;
-        srv->flags.reserved = S_HELPER_FREE;
-        srv->deferred_requests = 0;
-        srv->stats.deferbyfunc = 0;
-        srv->stats.deferbycb = 0;
+        srv->flags.reserved = 0;
         srv->stats.submits = 0;
         srv->stats.releases = 0;
         srv->index = k;
@@ -319,8 +314,7 @@ helperSubmit(helper * hlp, const char *buf, HLPCB * callback, void *data)
     debugs(84, 9, "helperSubmit: " << buf);
 }
 
-/* lastserver = "server last used as part of a deferred or reserved
- * request sequence"
+/* lastserver = "server last used as part of a reserved request sequence"
  */
 void
 helperStatefulSubmit(statefulhelper * hlp, const char *buf, HLPSCB * callback, void *data, helper_stateful_server * lastserver)
@@ -346,28 +340,12 @@ helperStatefulSubmit(statefulhelper * hlp, const char *buf, HLPSCB * callback, v
 
     if ((buf != NULL) && lastserver) {
         debugs(84, 5, "StatefulSubmit with lastserver " << lastserver);
-        /* the queue doesn't count for this assert because queued requests
-         * have already gone through here and been tested.
-         * It's legal to have deferred_requests == 0 and queue entries 
-         * and status of S_HELPEER_DEFERRED.
-         * BUT:  It's not legal to submit a new request w/lastserver in
-         * that state.
-         */
-        assert(!(lastserver->deferred_requests == 0 &&
-                 lastserver->flags.reserved == S_HELPER_DEFERRED));
 
-        if (lastserver->flags.reserved != S_HELPER_RESERVED) {
-            lastserver->stats.submits++;
-            lastserver->deferred_requests--;
-        }
+        assert(lastserver->flags.reserved);
+        assert(!(lastserver->request));
 
-        if (!(lastserver->request)) {
-            debugs(84, 5, "StatefulSubmit dispatching");
-            helperStatefulDispatch(lastserver, r);
-        } else {
-            debugs(84, 5, "StatefulSubmit queuing");
-            StatefulServerEnqueue(lastserver, r);
-        }
+        debugs(84, 5, "StatefulSubmit dispatching");
+        helperStatefulDispatch(lastserver, r);
     } else {
        helper_stateful_server *srv;
         if ((srv = StatefulGetFirstAvailable(hlp))) {
@@ -379,150 +357,26 @@ helperStatefulSubmit(statefulhelper * hlp, const char *buf, HLPSCB * callback, v
     debugs(84, 9, "helperStatefulSubmit: placeholder: '" << r->placeholder << "', buf '" << buf << "'.");
 }
 
-/*
- * helperStatefulDefer
- *
- * find and add a deferred request to a helper
- */
-helper_stateful_server *
-helperStatefulDefer(statefulhelper * hlp)
-{
-    if (hlp == NULL) {
-        debugs(84, 3, "helperStatefulDefer: hlp == NULL");
-        return NULL;
-    }
-
-    debugs(84, 5, "helperStatefulDefer: Running servers " << hlp->n_running);
-
-    if (hlp->n_running == 0) {
-        debugs(84, 1, "helperStatefulDefer: No running servers!. ");
-        return NULL;
-    }
-
-    helper_stateful_server *rv = StatefulGetFirstAvailable(hlp);
-
-    if (rv == NULL) {
-        /*
-         * all currently busy; loop through servers and find server
-         * with the shortest queue
-         */
-
-        for (dlink_node *n = hlp->servers.head; n != NULL; n = n->next) {
-            helper_stateful_server *srv = (helper_stateful_server *)n->data;
-
-            if (srv->flags.reserved == S_HELPER_RESERVED)
-                continue;
-
-            if (!srv->flags.shutdown)
-                continue;
-
-            if ((hlp->IsAvailable != NULL) && (srv->data != NULL) &&
-                    !(hlp->IsAvailable(srv->data)))
-                continue;
-
-            if ((rv != NULL) && (rv->deferred_requests < srv->deferred_requests))
-                continue;
-
-            rv = srv;
-        }
-    }
-
-    if (rv == NULL) {
-        debugs(84, 1, "helperStatefulDefer: None available.");
-        return NULL;
-    }
-
-    /* consistency check:
-     * when the deferred count is 0,
-     *   submits + releases == deferbyfunc + deferbycb
-     * Or in english, when there are no deferred requests, the amount
-     * we have submitted to the queue or cancelled must equal the amount
-     * we have said we wanted to be able to submit or cancel
-     */
-    if (rv->deferred_requests == 0)
-        assert(rv->stats.submits + rv->stats.releases ==
-               rv->stats.deferbyfunc + rv->stats.deferbycb);
-
-    rv->flags.reserved = S_HELPER_DEFERRED;
-
-    rv->deferred_requests++;
-
-    rv->stats.deferbyfunc++;
-
-    return rv;
-}
-
-void
-helperStatefulReset(helper_stateful_server * srv)
-/* puts this helper back in the queue. the calling app is required to
- * manage the state in the helper.
- */
-{
-    statefulhelper *hlp = srv->parent;
-    helper_stateful_request *r = srv->request;
-
-    if (r != NULL) {
-        /* reset attempt DURING an outstaning request */
-        debugs(84, 1, "helperStatefulReset: RESET During request " << hlp->id_name << " ");
-        srv->flags.busy = 0;
-        srv->roffset = 0;
-        helperStatefulRequestFree(r);
-        srv->request = NULL;
-    }
-
-    srv->flags.busy = 0;
-
-    if (srv->queue.head) {
-        srv->flags.reserved = S_HELPER_DEFERRED;
-    } else {
-        srv->flags.reserved = S_HELPER_FREE;
-
-        if ((srv->parent->OnEmptyQueue != NULL) && (srv->data))
-            srv->parent->OnEmptyQueue(srv->data);
-    }
-
-    helperStatefulServerKickQueue(srv);
-}
-
 /*
  * DPW 2007-05-08
  *
  * helperStatefulReleaseServer tells the helper that whoever was
  * using it no longer needs its services.
- *
- * If the state is S_HELPER_DEFERRED, decrease the deferred count.
- * If the count goes to zero, then it can become S_HELPER_FREE.
- *
- * If the state is S_HELPER_RESERVED, then it should always
- * become S_HELPER_FREE.
  */
 void
 helperStatefulReleaseServer(helper_stateful_server * srv)
 {
     debugs(84, 3, HERE << "srv-" << srv->index << " flags.reserved = " << srv->flags.reserved);
-    if (srv->flags.reserved == S_HELPER_FREE)
+    if (!srv->flags.reserved)
         return;
 
     srv->stats.releases++;
+    srv->flags.reserved = 0;
 
-    if (srv->flags.reserved == S_HELPER_DEFERRED) {
-        assert(srv->deferred_requests);
-        srv->deferred_requests--;
-       if (srv->deferred_requests) {
-           debugs(0,0,HERE << "helperStatefulReleaseServer srv->deferred_requests=" << srv->deferred_requests);
-           return;
-       }
-       if (srv->queue.head) {
-           debugs(0,0,HERE << "helperStatefulReleaseServer srv->queue.head not NULL");
-           return;
-       }
-    }
-
-    srv->flags.reserved = S_HELPER_FREE;
     if (srv->parent->OnEmptyQueue != NULL && srv->data)
         srv->parent->OnEmptyQueue(srv->data);
 
-    helperStatefulServerKickQueue(srv);
+    helperStatefulServerDone(srv);
 }
 
 void *
@@ -604,12 +458,11 @@ helperStatefulStats(StoreEntry * sentry, statefulhelper * hlp, const char *label
     storeAppendPrintf(sentry, "avg service time: %d msec\n",
                       hlp->stats.avg_svc_time);
     storeAppendPrintf(sentry, "\n");
-    storeAppendPrintf(sentry, "%7s\t%7s\t%7s\t%11s\t%20s\t%s\t%7s\t%7s\t%7s\n",
+    storeAppendPrintf(sentry, "%7s\t%7s\t%7s\t%11s\t%6s\t%7s\t%7s\t%7s\n",
                       "#",
                       "FD",
                       "PID",
                       "# Requests",
-                      "# Deferred Requests",
                       "Flags",
                       "Time",
                       "Offset",
@@ -617,17 +470,15 @@ helperStatefulStats(StoreEntry * sentry, statefulhelper * hlp, const char *label
 
     for (dlink_node *link = hlp->servers.head; link; link = link->next) {
         helper_stateful_server *srv = (helper_stateful_server *)link->data;
-        double tt = 0.001 * tvSubMsec(srv->dispatch_time,
-                               srv->flags.busy ? current_time : srv->answer_time);
-        storeAppendPrintf(sentry, "%7d\t%7d\t%7d\t%11d\t%20d\t%c%c%c%c%c\t%7.3f\t%7d\t%s\n",
+        double tt = 0.001 * tvSubMsec(srv->dispatch_time, srv->flags.busy ? current_time : srv->answer_time);
+        storeAppendPrintf(sentry, "%7d\t%7d\t%7d\t%11d\t%c%c%c%c%c\t%7.3f\t%7d\t%s\n",
                           srv->index + 1,
                           srv->rfd,
                           srv->pid,
                           srv->stats.uses,
-                          (int) srv->deferred_requests,
                           srv->flags.busy ? 'B' : ' ',
                           srv->flags.closing ? 'C' : ' ',
-                          srv->flags.reserved == S_HELPER_RESERVED ? 'R' : (srv->flags.reserved == S_HELPER_DEFERRED ? 'D' : ' '),
+                          srv->flags.reserved ? 'R' : ' ',
                           srv->flags.shutdown ? 'S' : ' ',
                           srv->request ? (srv->request->placeholder ? 'P' : ' ') : ' ',
                                   tt < 0.0 ? 0.0 : tt,
@@ -638,7 +489,7 @@ helperStatefulStats(StoreEntry * sentry, statefulhelper * hlp, const char *label
     storeAppendPrintf(sentry, "\nFlags key:\n\n");
     storeAppendPrintf(sentry, "   B = BUSY\n");
     storeAppendPrintf(sentry, "   C = CLOSING\n");
-    storeAppendPrintf(sentry, "   R = RESERVED or DEFERRED\n");
+    storeAppendPrintf(sentry, "   R = RESERVED\n");
     storeAppendPrintf(sentry, "   S = SHUTDOWN PENDING\n");
     storeAppendPrintf(sentry, "   P = PLACEHOLDER\n");
 }
@@ -746,14 +597,14 @@ helperStatefulShutdown(statefulhelper * hlp)
             continue;
         }
 
-        if (srv->flags.reserved != S_HELPER_FREE) {
-            debugs(84, 3, "helperStatefulShutdown: " << hlp->id_name << " #" << srv->index + 1 << " is RESERVED.");
-            continue;
-        }
-
-        if (srv->deferred_requests) {
-            debugs(84, 3, "helperStatefulShutdown: " << hlp->id_name << " #" << srv->index + 1 << " has DEFERRED requests.");
-            continue;
+        if (srv->flags.reserved) {
+            if (shutting_down) {
+                debugs(84, 3, "helperStatefulShutdown: " << hlp->id_name << " #" << srv->index + 1 << " is RESERVED. Closing anyway.");
+            }
+            else {
+                debugs(84, 3, "helperStatefulShutdown: " << hlp->id_name << " #" << srv->index + 1 << " is RESERVED. Not Shutting Down Yet.");
+                continue;
+            }
         }
 
         srv->flags.closing = 1;
@@ -1156,44 +1007,22 @@ helperStatefulHandleRead(int fd, char *buf, size_t len, comm_err_t flag, int xer
 
             case S_HELPER_RELEASE:     /* helper finished with */
 
-                if (!srv->deferred_requests && !srv->queue.head) {
-                    srv->flags.reserved = S_HELPER_FREE;
+                srv->flags.reserved = 0;
 
-                    if ((srv->parent->OnEmptyQueue != NULL) && (srv->data))
-                        srv->parent->OnEmptyQueue(srv->data);
+                if ((srv->parent->OnEmptyQueue != NULL) && (srv->data))
+                    srv->parent->OnEmptyQueue(srv->data);
 
-                    debugs(84, 5, "StatefulHandleRead: releasing " << hlp->id_name << " #" << srv->index + 1);
-                } else {
-                    srv->flags.reserved = S_HELPER_DEFERRED;
-                    debugs(84, 5, "StatefulHandleRead: outstanding deferred requests on " <<
-                           hlp->id_name << " #" << srv->index + 1 <<
-                           ". reserving for deferred requests.");
-                }
+                debugs(84, 5, "StatefulHandleRead: releasing " << hlp->id_name << " #" << srv->index + 1);
 
                 break;
 
             case S_HELPER_RESERVE:     /* 'pin' this helper for the caller */
 
-                if (!srv->queue.head) {
-                    assert(srv->deferred_requests == 0);
-                    srv->flags.reserved = S_HELPER_RESERVED;
-                    debugs(84, 5, "StatefulHandleRead: reserving " << hlp->id_name << " #" << srv->index + 1);
-                } else {
-                    fatal("StatefulHandleRead: Callback routine attempted to reserve a stateful helper with deferred requests. This can lead to deadlock.\n");
-                }
+               srv->flags.reserved = 1;
+               debugs(84, 5, "StatefulHandleRead: reserving " << hlp->id_name << " #" << srv->index + 1);
 
                 break;
 
-            case S_HELPER_DEFER:
-                /* the helper is still needed, but can
-                 * be used for other requests in the meantime.
-                 */
-                srv->flags.reserved = S_HELPER_DEFERRED;
-                srv->deferred_requests++;
-                srv->stats.deferbycb++;
-                debugs(84, 5, "StatefulHandleRead: reserving " << hlp->id_name << " #" << srv->index + 1 << " for deferred requests.");
-                break;
-
             default:
                 fatal("helperStatefulHandleRead: unknown stateful helper callback result.\n");
             }
@@ -1213,7 +1042,7 @@ helperStatefulHandleRead(int fd, char *buf, size_t len, comm_err_t flag, int xer
                        tvSubMsec(srv->dispatch_time, current_time),
                        hlp->stats.replies, REDIRECT_AV_FACTOR);
 
-        helperStatefulServerKickQueue(srv);
+        helperStatefulServerDone(srv);
     }
 
     if (srv->rfd != -1)
@@ -1278,31 +1107,6 @@ StatefulEnqueue(statefulhelper * hlp, helper_stateful_request * r)
 
 }
 
-static void
-StatefulServerEnqueue(helper_stateful_server * srv, helper_stateful_request * r)
-{
-    dlink_node *link = (dlink_node *)memAllocate(MEM_DLINK_NODE);
-    dlinkAddTail(r, link, &srv->queue);
-    /* TODO: warning if the queue on this server is more than X
-     * We don't check the queue size at the moment, because
-     * requests hitting here are deferrable 
-     */
-    /*    hlp->stats.queue_size++;
-     * if (hlp->stats.queue_size < hlp->n_running)
-     * return;
-     * if (squid_curtime - hlp->last_queue_warn < 600)
-     * return;
-     * if (shutting_down || reconfiguring)
-     * return;
-     * hlp->last_queue_warn = squid_curtime;
-     * debugs(84, 0, "WARNING: All " << hlp->id_name << " processes are busy.");
-     * debugs(84, 0, "WARNING: " << hlp->stats.queue_size << " pending requests queued");
-     * if (hlp->stats.queue_size > hlp->n_running * 2)
-     * fatalf("Too many queued %s requests", hlp->id_name);
-     * debugs(84, 1, "Consider increasing the number of " << hlp->id_name << " processes in your config file." );  */
-}
-
-
 static helper_request *
 Dequeue(helper * hlp)
 {
@@ -1319,21 +1123,6 @@ Dequeue(helper * hlp)
     return r;
 }
 
-static helper_stateful_request *
-StatefulServerDequeue(helper_stateful_server * srv)
-{
-    dlink_node *link;
-    helper_stateful_request *r = NULL;
-
-    if ((link = srv->queue.head)) {
-        r = (helper_stateful_request *)link->data;
-        dlinkDelete(link, &srv->queue);
-        memFree(link, MEM_DLINK_NODE);
-    }
-
-    return r;
-}
-
 static helper_stateful_request *
 StatefulDequeue(statefulhelper * hlp)
 {
@@ -1407,7 +1196,7 @@ StatefulGetFirstAvailable(statefulhelper * hlp)
         if (srv->flags.busy)
             continue;
 
-        if (srv->flags.reserved == S_HELPER_RESERVED)
+        if (srv->flags.reserved)
             continue;
 
         if (srv->flags.shutdown)
@@ -1527,7 +1316,7 @@ helperStatefulDispatch(helper_stateful_server * srv, helper_stateful_request * r
 
     if (r->placeholder == 1) {
         /* a callback is needed before this request can _use_ a helper. */
-        /* we don't care about releasing/deferring this helper. The request NEVER
+        /* we don't care about releasing this helper. The request NEVER
          * gets to the helper. So we throw away the return code */
         r->callback(r->data, srv, NULL);
         /* throw away the placeholder */
@@ -1536,7 +1325,7 @@ helperStatefulDispatch(helper_stateful_server * srv, helper_stateful_request * r
          * request to the helper which is why we test for the request*/
 
         if (srv->request == NULL)
-            helperStatefulServerKickQueue(srv);
+            helperStatefulServerDone(srv);
 
         return;
     }
@@ -1579,18 +1368,11 @@ helperStatefulKickQueue(statefulhelper * hlp)
 }
 
 static void
-helperStatefulServerKickQueue(helper_stateful_server * srv)
+helperStatefulServerDone(helper_stateful_server * srv)
 {
-    helper_stateful_request *r;
-
-    if ((r = StatefulServerDequeue(srv))) {
-        helperStatefulDispatch(srv, r);
-        return;
-    }
-
     if (!srv->flags.shutdown) {
         helperStatefulKickQueue(srv->parent);
-    } else if (!srv->flags.closing && srv->flags.reserved == S_HELPER_FREE && !srv->flags.busy) {
+    } else if (!srv->flags.closing && !srv->flags.reserved && !srv->flags.busy) {
         int wfd = srv->wfd;
         srv->wfd = -1;
         if (srv->rfd == wfd)

diff --git a/src/helper.h b/src/helper.h
index f5ed077a11c47243a917d8002579aad978c4475f..8de49dd7165038203a24d694bcfbd8c16065dd73 100644 (file)
--- a/src/helper.h
+++ b/src/helper.h
@@ -171,37 +171,24 @@ struct _helper_stateful_server
     struct timeval answer_time;
 
     dlink_node link;
-    dlink_list queue;
     statefulhelper *parent;
     helper_stateful_request *request;
 
     struct _helper_stateful_flags
     {
-
-unsigned int busy:
-        1;
-
-unsigned int closing:
-        1;
-
-unsigned int shutdown:
-        1;
-        stateful_helper_reserve_t reserved;
-    }
-
-    flags;
+        unsigned int busy:1;
+        unsigned int closing:1;
+        unsigned int shutdown:1;
+        unsigned int reserved:1;
+    } flags;
 
     struct
     {
         int uses;
         int submits;
         int releases;
-        int deferbyfunc;
-        int deferbycb;
-    }
+    } stats;
 
-    stats;
-    int deferred_requests;     /* current number of deferred requests */
     void *data;                        /* State data used by the calling routines */
     void *hIpc;
 };
@@ -227,7 +214,7 @@ public:
     MEMPROXY_CLASS(helper_stateful_request);
     char *buf;
     HLPSCB *callback;
-    int placeholder;           /* if 1, this is a dummy request waiting for a stateful helper to become available for deferred requests.*/
+    int placeholder;           /* if 1, this is a dummy request waiting for a stateful helper to become available */
     void *data;
 };
 
@@ -246,10 +233,8 @@ SQUIDCEXTERN helper *helperCreate(const char *);
 SQUIDCEXTERN statefulhelper *helperStatefulCreate(const char *);
 SQUIDCEXTERN void helperFree(helper *);
 SQUIDCEXTERN void helperStatefulFree(statefulhelper *);
-SQUIDCEXTERN void helperStatefulReset(helper_stateful_server * srv);
 SQUIDCEXTERN void helperStatefulReleaseServer(helper_stateful_server * srv);
 SQUIDCEXTERN void *helperStatefulServerGetData(helper_stateful_server * srv);
-SQUIDCEXTERN helper_stateful_server *helperStatefulDefer(statefulhelper *);