From: Eduard Bagdasaryan Date: Mon, 8 Jul 2024 21:58:29 +0000 (+0000) Subject: Fix PeerDigest lifetime management (#1857) X-Git-Tag: SQUID_7_0_1~93 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=f036532;p=thirdparty%2Fsquid.git 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 98a750cb41..11d7716404 100644 --- 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(digestTmp)); + delete digest; xfree(digest_url); #endif diff --git a/src/PeerDigest.h b/src/PeerDigest.h index ed5ed57c27..2d760e5696 100644 --- a/src/PeerDigest.h +++ b/src/PeerDigest.h @@ -49,7 +49,7 @@ public: DigestFetchState(PeerDigest *,HttpRequest *); ~DigestFetchState(); - PeerDigest *pd; + CbcPointer 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 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 dd7eff5dbd..10e15d270e 100644 --- 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 7d290cc901..0f375ac4c7 100644 --- 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(""); // 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