Fix PeerDigest lifetime management (#1857)

This change fixes how cbdata is used for managing CachePeer::digest
lifetime. Prior to these changes, cbdata was (ab)used as a reference
counting mechanism: CachePeer::digest object could outlive CachePeer
(effectively its creator), necessitating complex "is it time to delete
this digest?" cleanup logic. Now, CachePeer is an exclusive digest owner
that no longer locks/unlocks its digest field; it just creates/deletes
the object. Digest fetching code no longer needs to cleanup the digest.

"CachePeer::digest is always valid" invariant simplifies digest fetching
code and, hopefully, reduces the probability of bugs similar to Bug 5329
fixed in minimal commit 4657405 (that promised this refactoring).

diff --git a/src/CachePeer.cc b/src/CachePeer.cc
index 98a750cb418041c0ebeadf79be2ab8f553b8e0e1..11d77164045889547a0b36c87c2f9e89c27e1d27 100644 (file)
--- a/src/CachePeer.cc
+++ b/src/CachePeer.cc
@@ -41,9 +41,7 @@ CachePeer::~CachePeer()
     aclDestroyAccessList(&access);
 
 #if USE_CACHE_DIGESTS
-    void *digestTmp = nullptr;
-    if (cbdataReferenceValidDone(digest, &digestTmp))
-        peerDigestNotePeerGone(static_cast<PeerDigest *>(digestTmp));
+    delete digest;
     xfree(digest_url);
 #endif
 

diff --git a/src/PeerDigest.h b/src/PeerDigest.h
index ed5ed57c2742ec2115e1a0933cdc6639dcbb16b6..2d760e5696421b2d2b5ce2ea997b3847a7fb232e 100644 (file)
--- a/src/PeerDigest.h
+++ b/src/PeerDigest.h
@@ -49,7 +49,7 @@ public:
     DigestFetchState(PeerDigest *,HttpRequest *);
     ~DigestFetchState();
 
-    PeerDigest *pd;
+    CbcPointer<PeerDigest> pd;
     StoreEntry *entry;
     StoreEntry *old_entry;
     store_client *sc;
@@ -79,6 +79,10 @@ public:
     PeerDigest(CachePeer *);
     ~PeerDigest();
 
+    /// reacts to digest transfer completion
+    /// \prec DigestFetchState stats were finalized (by calling peerDigestFetchSetStats())
+    void noteFetchFinished(const DigestFetchState &, const char *outcomeDescription, bool sawError);
+
     CbcPointer<CachePeer> peer; ///< pointer back to peer structure, argh
     CacheDigest *cd = nullptr;            /**< actual digest structure */
     const SBuf host; ///< copy of peer->host

diff --git a/src/cache_cf.cc b/src/cache_cf.cc
index dd7eff5dbdbeeef4acb733b2b1679211f918424a..10e15d270e6c6915ab2b1f427fe09ce075370af3 100644 (file)
--- a/src/cache_cf.cc
+++ b/src/cache_cf.cc
@@ -2392,10 +2392,8 @@ parse_peer(CachePeers **peers)
         p->connect_fail_limit = 10;
 
 #if USE_CACHE_DIGESTS
-    if (!p->options.no_digest) {
-        const auto pd = new PeerDigest(p);
-        p->digest = cbdataReference(pd); // CachePeer destructor unlocks
-    }
+    if (!p->options.no_digest)
+        p->digest = new PeerDigest(p);
 #endif
 
     if (p->secure.encryptTransport)

diff --git a/src/peer_digest.cc b/src/peer_digest.cc
index 7d290cc9013e2885e36866c8581d068e661178db..0f375ac4c7968072ee26ee4a10f227a0d50be13b 100644 (file)
--- a/src/peer_digest.cc
+++ b/src/peer_digest.cc
@@ -44,9 +44,7 @@ int peerDigestSwapInMask(void *, char *, ssize_t);
 static int peerDigestFetchedEnough(DigestFetchState * fetch, char *buf, ssize_t size, const char *step_name);
 static void peerDigestFetchStop(DigestFetchState * fetch, char *buf, const char *reason);
 static void peerDigestFetchAbort(DigestFetchState * fetch, char *buf, const char *reason);
-static void peerDigestReqFinish(DigestFetchState * fetch, char *buf, int, int, int, const char *reason, int err);
-static void peerDigestPDFinish(DigestFetchState * fetch, int pcb_valid, int err);
-static void peerDigestFetchFinish(DigestFetchState * fetch, int err);
+static void peerDigestReqFinish(DigestFetchState *, char *buf, const char *reason, int err);
 static void peerDigestFetchSetStats(DigestFetchState * fetch);
 static int peerDigestSetCBlock(PeerDigest * pd, const char *buf);
 static int peerDigestUseful(const PeerDigest * pd);
@@ -77,7 +75,7 @@ CBDATA_CLASS_INIT(PeerDigest);
 CBDATA_CLASS_INIT(DigestFetchState);
 
 DigestFetchState::DigestFetchState(PeerDigest *aPd, HttpRequest *req) :
-    pd(cbdataReference(aPd)),
+    pd(aPd),
     entry(nullptr),
     old_entry(nullptr),
     sc(nullptr),
@@ -104,6 +102,14 @@ DigestFetchState::DigestFetchState(PeerDigest *aPd, HttpRequest *req) :
 
 DigestFetchState::~DigestFetchState()
 {
+    if (old_entry) {
+        debugs(72, 3, "deleting old entry");
+        storeUnregister(old_sc, old_entry, this);
+        old_entry->releaseRequest();
+        old_entry->unlock("DigestFetchState destructed old");
+        old_entry = nullptr;
+    }
+
     /* unlock everything */
     storeUnregister(sc, entry, this);
 
@@ -111,8 +117,6 @@ DigestFetchState::~DigestFetchState()
     entry = nullptr;
 
     HTTPMSGUNLOCK(request);
-
-    assert(pd == nullptr);
 }
 
 PeerDigest::~PeerDigest()
@@ -168,21 +172,6 @@ peerDigestSetCheck(PeerDigest * pd, time_t delay)
     debugs(72, 3, "peerDigestSetCheck: will check peer " << pd->host << " in " << delay << " secs");
 }
 
-/*
- * called when peer is about to disappear or have already disappeared
- */
-void
-peerDigestNotePeerGone(PeerDigest * pd)
-{
-    if (pd->flags.requested) {
-        debugs(72, 2, "peerDigest: peer " << pd->host << " gone, will destroy after fetch.");
-        /* do nothing now, the fetching chain will notice and take action */
-    } else {
-        debugs(72, 2, "peerDigest: peer " << pd->host << " is gone, destroying now.");
-        delete pd;
-    }
-}
-
 /* callback for eventAdd() (with peer digest locked)
  * request new digest if our copy is too old or if we lack one;
  * schedule next check otherwise */
@@ -196,10 +185,7 @@ peerDigestCheck(void *data)
 
     pd->times.next_check = 0;   /* unknown */
 
-    if (pd->peer.set() && !pd->peer.valid()) {
-        peerDigestNotePeerGone(pd);
-        return;
-    }
+    Assure(pd->peer.valid());
 
     debugs(72, 3, "cache_peer " << RawPointer(pd->peer).orNil());
     debugs(72, 3, "peerDigestCheck: time: " << squid_curtime <<
@@ -349,7 +335,11 @@ peerDigestHandleReply(void *data, StoreIOBuffer receivedData)
         return;
     }
 
-    assert(fetch->pd);
+    if (!fetch->pd) {
+        peerDigestFetchAbort(fetch, fetch->buf, "digest disappeared while loading digest reply from Store");
+        return;
+    }
+
     /* The existing code assumes that the received pointer is
      * where we asked the data to be put
      */
@@ -448,7 +438,7 @@ static int
 peerDigestFetchReply(void *data, char *buf, ssize_t size)
 {
     DigestFetchState *fetch = (DigestFetchState *)data;
-    PeerDigest *pd = fetch->pd;
+    const auto pd = fetch->pd.get();
     assert(pd && buf);
     assert(!fetch->offset);
 
@@ -535,7 +525,7 @@ peerDigestSwapInCBlock(void *data, char *buf, ssize_t size)
         return -1;
 
     if (size >= (ssize_t)StoreDigestCBlockSize) {
-        PeerDigest *pd = fetch->pd;
+        const auto pd = fetch->pd.get();
 
         assert(pd);
         assert(fetch->entry->mem_obj);
@@ -566,9 +556,8 @@ int
 peerDigestSwapInMask(void *data, char *buf, ssize_t size)
 {
     DigestFetchState *fetch = (DigestFetchState *)data;
-    PeerDigest *pd;
-
-    pd = fetch->pd;
+    const auto pd = fetch->pd.get();
+    assert(pd);
     assert(pd->cd && pd->cd->mask);
 
     /*
@@ -602,20 +591,16 @@ peerDigestFetchedEnough(DigestFetchState * fetch, char *buf, ssize_t size, const
     static const SBuf hostUnknown("<unknown>"); // peer host (if any)
     SBuf host = hostUnknown;
 
-    PeerDigest *pd = nullptr;
+    const auto pd = fetch->pd.get();
+    Assure(pd);
     const char *reason = nullptr;  /* reason for completion */
     const char *no_bug = nullptr;  /* successful completion if set */
-    const int pdcb_valid = cbdataReferenceValid(fetch->pd);
-    const int pcb_valid = pdcb_valid && fetch->pd->peer.valid();
 
     /* test possible exiting conditions (the same for most steps!)
      * cases marked with '?!' should not happen */
 
     if (!reason) {
-        if (!pdcb_valid || !(pd = fetch->pd))
-            reason = "peer digest disappeared?!";
-        else
-            host = pd->host;
+        host = pd->host;
     }
 
     debugs(72, 6, step_name << ": peer " << host << ", offset: " <<
@@ -624,9 +609,7 @@ peerDigestFetchedEnough(DigestFetchState * fetch, char *buf, ssize_t size, const
     /* continue checking (with pd and host known and valid) */
 
     if (!reason) {
-        if (!pd->peer)
-            reason = "peer disappeared";
-        else if (size < 0)
+        if (size < 0)
             reason = "swap failure";
         else if (!fetch->entry)
             reason = "swap aborted?!";
@@ -650,11 +633,7 @@ peerDigestFetchedEnough(DigestFetchState * fetch, char *buf, ssize_t size, const
     if (reason) {
         const int level = strstr(reason, "?!") ? 1 : 3;
         debugs(72, level, "" << step_name << ": peer " << host << ", exiting after '" << reason << "'");
-        peerDigestReqFinish(fetch, buf,
-                            1, pdcb_valid, pcb_valid, reason, !no_bug);
-    } else {
-        /* paranoid check */
-        assert(pdcb_valid && pcb_valid);
+        peerDigestReqFinish(fetch, buf, reason, !no_bug);
     }
 
     return reason != nullptr;
@@ -667,64 +646,43 @@ peerDigestFetchStop(DigestFetchState * fetch, char *buf, const char *reason)
 {
     assert(reason);
     debugs(72, 2, "peerDigestFetchStop: peer " << fetch->pd->host << ", reason: " << reason);
-    peerDigestReqFinish(fetch, buf, 1, 1, 1, reason, 0);
+    peerDigestReqFinish(fetch, buf, reason, 0);
 }
 
-/* call this when all callback data is valid but something bad happened */
+/// diff reducer: handle a peer digest fetch failure
 static void
 peerDigestFetchAbort(DigestFetchState * fetch, char *buf, const char *reason)
 {
     assert(reason);
-    debugs(72, 2, "peerDigestFetchAbort: peer " << fetch->pd->host << ", reason: " << reason);
-    peerDigestReqFinish(fetch, buf, 1, 1, 1, reason, 1);
+    peerDigestReqFinish(fetch, buf, reason, 1);
 }
 
 /* complete the digest transfer, update stats, unlock/release everything */
 static void
 peerDigestReqFinish(DigestFetchState * fetch, char * /* buf */,
-                    int fcb_valid, int pdcb_valid, int pcb_valid,
                     const char *reason, int err)
 {
     assert(reason);
 
-    /* must go before peerDigestPDFinish */
-
-    if (pdcb_valid) {
-        fetch->pd->flags.requested = false;
-        fetch->pd->req_result = reason;
-    }
-
-    /* schedule next check if peer is still out there */
-    if (pcb_valid) {
-        PeerDigest *pd = fetch->pd;
-
-        if (err) {
-            pd->times.retry_delay = peerDigestIncDelay(pd);
-            peerDigestSetCheck(pd, pd->times.retry_delay);
-        } else {
-            pd->times.retry_delay = 0;
-            peerDigestSetCheck(pd, peerDigestNewDelay(fetch->entry));
-        }
-    }
+    debugs(72, 2, "peer: " << RawPointer(fetch->pd.valid() ? fetch->pd->peer : nullptr).orNil() << ", reason: " << reason << ", err: " << err);
 
     /* note: order is significant */
-    if (fcb_valid)
-        peerDigestFetchSetStats(fetch);
-
-    if (pdcb_valid)
-        peerDigestPDFinish(fetch, pcb_valid, err);
+    peerDigestFetchSetStats(fetch);
+    if (const auto pd = fetch->pd.get())
+        pd->noteFetchFinished(*fetch, reason, err);
 
-    if (fcb_valid)
-        peerDigestFetchFinish(fetch, err);
+    delete fetch;
 }
 
-/* destroys digest if peer disappeared
- * must be called only when fetch and pd cbdata are valid */
-static void
-peerDigestPDFinish(DigestFetchState * fetch, int pcb_valid, int err)
+void
+PeerDigest::noteFetchFinished(const DigestFetchState &finishedFetch, const char * const outcomeDescription, const bool sawError)
 {
-    PeerDigest *pd = fetch->pd;
-    const auto host = pd->host;
+    const auto pd = this; // TODO: remove this diff reducer
+    const auto fetch = &finishedFetch; // TODO: remove this diff reducer
+
+    pd->flags.requested = false;
+    pd->req_result = outcomeDescription;
+
     pd->times.received = squid_curtime;
     pd->times.req_delay = fetch->resp_time;
     pd->stats.sent.kbytes += fetch->sent.bytes;
@@ -732,20 +690,19 @@ peerDigestPDFinish(DigestFetchState * fetch, int pcb_valid, int err)
     pd->stats.sent.msgs += fetch->sent.msg;
     pd->stats.recv.msgs += fetch->recv.msg;
 
-    if (err) {
-        debugs(72, DBG_IMPORTANT, "" << (pcb_valid ? "temporary " : "" ) << "disabling (" << pd->req_result << ") digest from " << host);
+    if (sawError) {
+        debugs(72, DBG_IMPORTANT, "disabling (" << outcomeDescription << ") digest from " << host);
 
+        pd->times.retry_delay = peerDigestIncDelay(pd);
+        peerDigestSetCheck(pd, pd->times.retry_delay);
         delete pd->cd;
         pd->cd = nullptr;
 
         pd->flags.usable = false;
-
-        if (!pcb_valid)
-            peerDigestNotePeerGone(pd);
     } else {
-        assert(pcb_valid);
-
         pd->flags.usable = true;
+        pd->times.retry_delay = 0;
+        peerDigestSetCheck(pd, peerDigestNewDelay(fetch->entry));
 
         /* XXX: ugly condition, but how? */
 
@@ -754,32 +711,6 @@ peerDigestPDFinish(DigestFetchState * fetch, int pcb_valid, int err)
         else
             debugs(72, 2, "received valid digest from " << host);
     }
-
-    cbdataReferenceDone(fetch->pd);
-}
-
-/* free fetch state structures
- * must be called only when fetch cbdata is valid */
-static void
-peerDigestFetchFinish(DigestFetchState * fetch, int /* err */)
-{
-    assert(fetch->entry && fetch->request);
-
-    if (fetch->old_entry) {
-        debugs(72, 3, "peerDigestFetchFinish: deleting old entry");
-        storeUnregister(fetch->old_sc, fetch->old_entry, fetch);
-        fetch->old_entry->releaseRequest();
-        fetch->old_entry->unlock("peerDigestFetchFinish old");
-        fetch->old_entry = nullptr;
-    }
-
-    /* update global stats */
-    statCounter.cd.kbytes_sent += fetch->sent.bytes;
-    statCounter.cd.kbytes_recv += fetch->recv.bytes;
-    statCounter.cd.msgs_sent += fetch->sent.msg;
-    statCounter.cd.msgs_recv += fetch->recv.msg;
-
-    delete fetch;
 }
 
 /* calculate fetch stats after completion */
@@ -813,6 +744,10 @@ peerDigestFetchSetStats(DigestFetchState * fetch)
            std::showpos << (int) (fetch->entry->lastModified() - squid_curtime) <<
            ")");
 
+    statCounter.cd.kbytes_sent += fetch->sent.bytes;
+    statCounter.cd.kbytes_recv += fetch->recv.bytes;
+    statCounter.cd.msgs_sent += fetch->sent.msg;
+    statCounter.cd.msgs_recv += fetch->recv.msg;
 }
 
 static int