From 39fe14b23a8168e233600625a45034a7b33c912a Mon Sep 17 00:00:00 2001 From: Eduard Bagdasaryan Date: Thu, 1 Jun 2017 17:34:40 -0600 Subject: [PATCH] Collapse internal revalidation requests (SMP-unaware caches), again. The security fix in v5 r14979 had a negative effect on collapsed forwarding. All "private" entries were considered automatically non-shareable among collapsed clients. However this is not true: there are many situations when collapsed forwarding should work despite of "private" entry status: 304/5xx responses are good examples of that. This patch fixes that by means of a new StoreEntry::shareableWhenPrivate flag. The suggested fix is not complete: To cover all possible situations, we need to decide whether StoreEntry::shareableWhenPrivate is true or not for all contexts where StoreEntry::setPrivateKey() is used. This patch fixes only few important cases inside http.cc, making CF (as well collapsed revalidation) work for some [non-cacheable] response status codes, including 3xx, 5xx and some others. The original support for internal revalidation requests collapsing was in trink r14755 and referred to Squid bugs 2833, 4311, and 4471. --- src/MemStore.cc | 2 +- src/Store.h | 27 +++- src/client_side_reply.cc | 6 +- src/fs/rock/RockSwapDir.cc | 2 +- src/fs/ufs/UFSSwapDir.cc | 2 +- src/http.cc | 247 ++++++++++++++++--------------- src/http.h | 23 ++- src/store.cc | 52 +++++-- src/tests/stub_store.cc | 8 +- src/tests/testStoreController.cc | 2 +- src/tests/testStoreHashIndex.cc | 2 +- 11 files changed, 221 insertions(+), 152 deletions(-) diff --git a/src/MemStore.cc b/src/MemStore.cc index fed280c389..ad65db20c9 100644 --- a/src/MemStore.cc +++ b/src/MemStore.cc @@ -474,7 +474,7 @@ MemStore::anchorEntry(StoreEntry &e, const sfileno index, const Ipc::StoreMapAnc e.ping_status = PING_NONE; EBIT_CLR(e.flags, RELEASE_REQUEST); - EBIT_CLR(e.flags, KEY_PRIVATE); + e.clearPrivate(); EBIT_SET(e.flags, ENTRY_VALIDATED); MemObject::MemCache &mc = e.mem_obj->memCache; diff --git a/src/Store.h b/src/Store.h index c5ffeb6370..cb31b5c33d 100644 --- a/src/Store.h +++ b/src/Store.h @@ -83,15 +83,19 @@ public: void abort(); void makePublic(const KeyScope keyScope = ksDefault); - void makePrivate(); + void makePrivate(const bool shareable); + /// A low-level method just resetting "private key" flags. + /// To avoid key inconsistency please use forcePublicKey() + /// or similar instead. + void clearPrivate(); void setPublicKey(const KeyScope keyScope = ksDefault); /// Resets existing public key to a public key with default scope, /// releasing the old default-scope entry (if any). /// Does nothing if the existing public key already has default scope. void clearPublicKeyScope(); - void setPrivateKey(); + void setPrivateKey(const bool shareable); void expireNow(); - void releaseRequest(); + void releaseRequest(const bool shareable = false); void negativeCache(); void cacheNegatively(); /** \todo argh, why both? */ void invokeHandlers(); @@ -212,7 +216,13 @@ public: /// update last reference timestamp and related Store metadata void touch(); - virtual void release(); + virtual void release(const bool shareable = false); + + /// May the caller commit to treating this [previously locked] + /// entry as a cache hit? + bool mayStartHitting() const { + return !EBIT_TEST(flags, KEY_PRIVATE) || shareableWhenPrivate; + } #if USE_ADAPTATION /// call back producer when more buffer space is available @@ -240,6 +250,13 @@ private: unsigned short lock_count; /* Assume < 65536! */ + /// Nobody can find/lock KEY_PRIVATE entries, but some transactions + /// (e.g., collapsed requests) find/lock a public entry before it becomes + /// private. May such transactions start using the now-private entry + /// they previously locked? This member should not affect transactions + /// that already started reading from the entry. + bool shareableWhenPrivate; + #if USE_ADAPTATION /// producer callback registered with deferProducer AsyncCall::Pointer deferredProducer; @@ -247,6 +264,8 @@ private: bool validLength() const; bool hasOneOfEtags(const String &reqETags, const bool allowWeakMatch) const; + + friend std::ostream &operator <<(std::ostream &os, const StoreEntry &e); }; std::ostream &operator <<(std::ostream &os, const StoreEntry &e); diff --git a/src/client_side_reply.cc b/src/client_side_reply.cc index 395b366950..a367b5b674 100644 --- a/src/client_side_reply.cc +++ b/src/client_side_reply.cc @@ -415,8 +415,8 @@ clientReplyContext::handleIMSReply(StoreIOBuffer result) if (result.flags.error && !EBIT_TEST(http->storeEntry()->flags, ENTRY_ABORTED)) return; - if (collapsedRevalidation == crSlave && EBIT_TEST(http->storeEntry()->flags, KEY_PRIVATE)) { - debugs(88, 3, "CF slave hit private " << *http->storeEntry() << ". MISS"); + if (collapsedRevalidation == crSlave && !http->storeEntry()->mayStartHitting()) { + debugs(88, 3, "CF slave hit private non-shareable " << *http->storeEntry() << ". MISS"); // restore context to meet processMiss() expectations restoreState(); http->logType = LOG_TCP_MISS; @@ -548,7 +548,7 @@ clientReplyContext::cacheHit(StoreIOBuffer result) // The previously identified hit suddenly became unsharable! // This is common for collapsed forwarding slaves but might also // happen to regular hits because we are called asynchronously. - if (EBIT_TEST(e->flags, KEY_PRIVATE)) { + if (!e->mayStartHitting()) { debugs(88, 3, "unsharable " << *e << ". MISS"); http->logType = LOG_TCP_MISS; processMiss(); diff --git a/src/fs/rock/RockSwapDir.cc b/src/fs/rock/RockSwapDir.cc index a9bab4eb6c..52a1c211fb 100644 --- a/src/fs/rock/RockSwapDir.cc +++ b/src/fs/rock/RockSwapDir.cc @@ -137,7 +137,7 @@ Rock::SwapDir::anchorEntry(StoreEntry &e, const sfileno filen, const Ipc::StoreM e.ping_status = PING_NONE; EBIT_CLR(e.flags, RELEASE_REQUEST); - EBIT_CLR(e.flags, KEY_PRIVATE); + e.clearPrivate(); EBIT_SET(e.flags, ENTRY_VALIDATED); e.swap_dirn = index; diff --git a/src/fs/ufs/UFSSwapDir.cc b/src/fs/ufs/UFSSwapDir.cc index 48170703cb..cb19b41b87 100644 --- a/src/fs/ufs/UFSSwapDir.cc +++ b/src/fs/ufs/UFSSwapDir.cc @@ -816,7 +816,7 @@ Fs::Ufs::UFSSwapDir::addDiskRestore(const cache_key * key, e->refcount = refcount; e->flags = newFlags; EBIT_CLR(e->flags, RELEASE_REQUEST); - EBIT_CLR(e->flags, KEY_PRIVATE); + e->clearPrivate(); e->ping_status = PING_NONE; EBIT_CLR(e->flags, ENTRY_VALIDATED); mapBitSet(e->swap_filen); diff --git a/src/http.cc b/src/http.cc index c19971a323..3e685d0e52 100644 --- a/src/http.cc +++ b/src/http.cc @@ -288,7 +288,9 @@ HttpStateData::processSurrogateControl(HttpReply *reply) (Config.onoff.surrogate_is_remote && sctusable->noStoreRemote())) { surrogateNoStore = true; - entry->makePrivate(); + // Be conservative for now and make it non-shareable because + // there is no enough information here to make the decision. + entry->makePrivate(false); } /* The HttpHeader logic cannot tell if the header it's parsing is a reply to an @@ -313,8 +315,8 @@ HttpStateData::processSurrogateControl(HttpReply *reply) } } -int -HttpStateData::cacheableReply() +HttpStateData::ReuseDecision::Answers +HttpStateData::reusableReply(HttpStateData::ReuseDecision &decision) { HttpReply const *rep = finalReply(); HttpHeader const *hdr = &rep->header; @@ -335,24 +337,19 @@ HttpStateData::cacheableReply() #define REFRESH_OVERRIDE(flag) 0 #endif - if (EBIT_TEST(entry->flags, RELEASE_REQUEST)) { - debugs(22, 3, "NO because " << *entry << " has been released."); - return 0; - } + if (EBIT_TEST(entry->flags, RELEASE_REQUEST)) + return decision.make(ReuseDecision::reuseNot, "the entry has been released"); // RFC 7234 section 4: a cache MUST use the most recent response // (as determined by the Date header field) - if (sawDateGoBack) { - debugs(22, 3, "NO because " << *entry << " has an older date header."); - return 0; - } + // TODO: whether such responses could be shareable? + if (sawDateGoBack) + return decision.make(ReuseDecision::reuseNot, "the response has an older date header"); // Check for Surrogate/1.0 protocol conditions // NP: reverse-proxy traffic our parent server has instructed us never to cache - if (surrogateNoStore) { - debugs(22, 3, HERE << "NO because Surrogate-Control:no-store"); - return 0; - } + if (surrogateNoStore) + return decision.make(ReuseDecision::reuseNot, "Surrogate-Control:no-store"); // RFC 2616: HTTP/1.1 Cache-Control conditions if (!ignoreCacheControl) { @@ -362,10 +359,9 @@ HttpStateData::cacheableReply() // RFC 2616 section 14.9.2 - MUST NOT cache any response with request CC:no-store if (request && request->cache_control && request->cache_control->hasNoStore() && - !REFRESH_OVERRIDE(ignore_no_store)) { - debugs(22, 3, HERE << "NO because client request Cache-Control:no-store"); - return 0; - } + !REFRESH_OVERRIDE(ignore_no_store)) + return decision.make(ReuseDecision::reuseNot, + "client request Cache-Control:no-store"); // NP: request CC:no-cache only means cache READ is forbidden. STORE is permitted. if (rep->cache_control && rep->cache_control->hasNoCacheWithParameters()) { @@ -374,8 +370,8 @@ HttpStateData::cacheableReply() * successfully (ie, must revalidate AND these headers are prohibited on stale replies). * That is a bit tricky for squid right now so we avoid caching entirely. */ - debugs(22, 3, HERE << "NO because server reply Cache-Control:no-cache has parameters"); - return 0; + return decision.make(ReuseDecision::reuseNot, + "server reply Cache-Control:no-cache has parameters"); } // NP: request CC:private is undefined. We ignore. @@ -383,10 +379,9 @@ HttpStateData::cacheableReply() // RFC 2616 section 14.9.2 - MUST NOT cache any response with CC:no-store if (rep->cache_control && rep->cache_control->hasNoStore() && - !REFRESH_OVERRIDE(ignore_no_store)) { - debugs(22, 3, HERE << "NO because server reply Cache-Control:no-store"); - return 0; - } + !REFRESH_OVERRIDE(ignore_no_store)) + return decision.make(ReuseDecision::reuseNot, + "server reply Cache-Control:no-store"); // RFC 2616 section 14.9.1 - MUST NOT cache any response with CC:private in a shared cache like Squid. // CC:private overrides CC:public when both are present in a response. @@ -399,23 +394,21 @@ HttpStateData::cacheableReply() * successfully (ie, must revalidate AND these headers are prohibited on stale replies). * That is a bit tricky for squid right now so we avoid caching entirely. */ - debugs(22, 3, HERE << "NO because server reply Cache-Control:private"); - return 0; + return decision.make(ReuseDecision::reuseNot, + "server reply Cache-Control:private"); } } // RFC 2068, sec 14.9.4 - MUST NOT cache any response with Authentication UNLESS certain CC controls are present // allow HTTP violations to IGNORE those controls (ie re-block caching Auth) if (request && (request->flags.auth || request->flags.authSent)) { - if (!rep->cache_control) { - debugs(22, 3, HERE << "NO because Authenticated and server reply missing Cache-Control"); - return 0; - } + if (!rep->cache_control) + return decision.make(ReuseDecision::reuseNot, + "authenticated and server reply missing Cache-Control"); - if (ignoreCacheControl) { - debugs(22, 3, HERE << "NO because Authenticated and ignoring Cache-Control"); - return 0; - } + if (ignoreCacheControl) + return decision.make(ReuseDecision::reuseNot, + "authenticated and ignoring Cache-Control"); bool mayStore = false; // HTTPbis pt6 section 3.2: a response CC:public is present @@ -444,10 +437,8 @@ HttpStateData::cacheableReply() mayStore = true; } - if (!mayStore) { - debugs(22, 3, HERE << "NO because Authenticated transaction"); - return 0; - } + if (!mayStore) + return decision.make(ReuseDecision::reuseNot, "authenticated transaction"); // NP: response CC:no-cache is equivalent to CC:must-revalidate,max-age=0. We MAY cache, and do so. // NP: other request CC flags are limiters on HIT/MISS/REFRESH. We don't care about here. @@ -458,12 +449,26 @@ HttpStateData::cacheableReply() * probably should not be cachable */ if ((v = hdr->getStr(Http::HdrType::CONTENT_TYPE))) - if (!strncasecmp(v, "multipart/x-mixed-replace", 25)) { - debugs(22, 3, HERE << "NO because Content-Type:multipart/x-mixed-replace"); - return 0; - } + if (!strncasecmp(v, "multipart/x-mixed-replace", 25)) + return decision.make(ReuseDecision::reuseNot, "Content-Type:multipart/x-mixed-replace"); + + // TODO: if possible, provide more specific message for each status code + static const char *shareableError = "shareable error status code"; + static const char *nonShareableError = "non-shareable error status code"; + ReuseDecision::Answers statusAnswer = ReuseDecision::reuseNot; + const char *statusReason = nonShareableError; switch (rep->sline.status()) { + + /* There are several situations when a non-cacheable response may be + * still shareable (e.g., among collapsed clients). We assume that these + * are 3xx and 5xx responses, indicating server problems and some of + * 4xx responses, common for all clients with a given cache key (e.g., + * 404 Not Found or 414 URI Too Long). On the other hand, we should not + * share non-cacheable client-specific errors, such as 400 Bad Request + * or 406 Not Acceptable. + */ + /* Responses that are cacheable */ case Http::scOkay: @@ -481,111 +486,87 @@ HttpStateData::cacheableReply() * unless we know how to refresh it. */ - if (!refreshIsCachable(entry) && !REFRESH_OVERRIDE(store_stale)) { - debugs(22, 3, "NO because refreshIsCachable() returned non-cacheable.."); - return 0; - } else { - debugs(22, 3, HERE << "YES because HTTP status " << rep->sline.status()); - return 1; - } - /* NOTREACHED */ + if (refreshIsCachable(entry) || REFRESH_OVERRIDE(store_stale)) + decision.make(ReuseDecision::cachePositively, "refresh check returned cacheable"); + else + decision.make(ReuseDecision::doNotCacheButShare, "refresh check returned non-cacheable"); break; /* Responses that only are cacheable if the server says so */ case Http::scFound: case Http::scTemporaryRedirect: - if (rep->date <= 0) { - debugs(22, 3, HERE << "NO because HTTP status " << rep->sline.status() << " and Date missing/invalid"); - return 0; - } - if (rep->expires > rep->date) { - debugs(22, 3, HERE << "YES because HTTP status " << rep->sline.status() << " and Expires > Date"); - return 1; - } else { - debugs(22, 3, HERE << "NO because HTTP status " << rep->sline.status() << " and Expires <= Date"); - return 0; - } - /* NOTREACHED */ + if (rep->date <= 0) + decision.make(ReuseDecision::doNotCacheButShare, "Date is missing/invalid"); + if (rep->expires > rep->date) + decision.make(ReuseDecision::cachePositively, "Expires > Date"); + else + decision.make(ReuseDecision::doNotCacheButShare, "Expires <= Date"); break; - /* Errors can be negatively cached */ - + /* Some responses can be negatively cached */ case Http::scNoContent: - case Http::scUseProxy: - - case Http::scBadRequest: - case Http::scForbidden: - case Http::scNotFound: - case Http::scMethodNotAllowed: - case Http::scUriTooLong: - case Http::scInternalServerError: - case Http::scNotImplemented: - case Http::scBadGateway: - case Http::scServiceUnavailable: - case Http::scGatewayTimeout: case Http::scMisdirectedRequest: - debugs(22, 3, "MAYBE because HTTP status " << rep->sline.status()); - return -1; + statusAnswer = ReuseDecision::doNotCacheButShare; + statusReason = shareableError; + case Http::scBadRequest: - /* NOTREACHED */ +#if USE_HTTP_VIOLATIONS + if (Config.negativeTtl > 0) + decision.make(ReuseDecision::cacheNegatively, "Config.negativeTtl > 0"); + else +#endif + decision.make(statusAnswer, statusReason); break; - /* Some responses can never be cached */ - - case Http::scPartialContent: /* Not yet supported */ - + /* these responses can never be cached, some + of them can be shared though */ case Http::scSeeOther: - case Http::scNotModified: - case Http::scUnauthorized: - case Http::scProxyAuthenticationRequired: - - case Http::scInvalidHeader: /* Squid header parsing error */ - - case Http::scHeaderTooLarge: - case Http::scPaymentRequired: + case Http::scInsufficientStorage: + // TODO: use more specific reason for non-error status codes + decision.make(ReuseDecision::doNotCacheButShare, shareableError); + break; + + case Http::scPartialContent: /* Not yet supported. TODO: make shareable for suitable ranges */ case Http::scNotAcceptable: - case Http::scRequestTimeout: - case Http::scConflict: + case Http::scRequestTimeout: // TODO: is this shareable? + case Http::scConflict: // TODO: is this shareable? case Http::scLengthRequired: case Http::scPreconditionFailed: case Http::scPayloadTooLarge: case Http::scUnsupportedMediaType: case Http::scUnprocessableEntity: - case Http::scLocked: + case Http::scLocked: // TODO: is this shareable? case Http::scFailedDependency: - case Http::scInsufficientStorage: case Http::scRequestedRangeNotSatisfied: case Http::scExpectationFailed: - - debugs(22, 3, HERE << "NO because HTTP status " << rep->sline.status()); - return 0; + case Http::scInvalidHeader: /* Squid header parsing error */ + case Http::scHeaderTooLarge: + decision.make(ReuseDecision::reuseNot, nonShareableError); + break; default: /* RFC 2616 section 6.1.1: an unrecognized response MUST NOT be cached. */ - debugs (11, 3, HERE << "NO because unknown HTTP status code " << rep->sline.status()); - return 0; - - /* NOTREACHED */ + decision.make(ReuseDecision::reuseNot, "unknown status code"); break; } - /* NOTREACHED */ + return decision.answer; } /// assemble a variant key (vary-mark) from the given Vary header and HTTP request @@ -921,11 +902,12 @@ HttpStateData::haveParsedReplyHeaders() Ctx ctx = ctx_enter(entry->mem_obj->urlXXX()); HttpReply *rep = finalReply(); + const Http::StatusCode statusCode = rep->sline.status(); entry->timestampsSet(); /* Check if object is cacheable or not based on reply code */ - debugs(11, 3, "HTTP CODE: " << rep->sline.status()); + debugs(11, 3, "HTTP CODE: " << statusCode); if (const StoreEntry *oldEntry = findPreviouslyCachedEntry(entry)) sawDateGoBack = rep->olderThan(oldEntry->getReply()); @@ -942,7 +924,9 @@ HttpStateData::haveParsedReplyHeaders() const SBuf vary(httpMakeVaryMark(request.getRaw(), rep)); if (vary.isEmpty()) { - entry->makePrivate(); + // TODO: check whether such responses are shareable. + // Do not share for now. + entry->makePrivate(false); if (!fwd->reforwardableStatus(rep->sline.status())) EBIT_CLR(entry->flags, ENTRY_FWD_HDR_WAIT); varyFailure = true; @@ -965,30 +949,31 @@ HttpStateData::haveParsedReplyHeaders() if (!fwd->reforwardableStatus(rep->sline.status())) EBIT_CLR(entry->flags, ENTRY_FWD_HDR_WAIT); - switch (cacheableReply()) { + ReuseDecision decision(entry, statusCode); - case 1: - entry->makePublic(); + switch (reusableReply(decision)) { + + case ReuseDecision::reuseNot: + entry->makePrivate(false); break; - case 0: - entry->makePrivate(); + case ReuseDecision::cachePositively: + entry->makePublic(); break; - case -1: + case ReuseDecision::cacheNegatively: + entry->cacheNegatively(); + break; -#if USE_HTTP_VIOLATIONS - if (Config.negativeTtl > 0) - entry->cacheNegatively(); - else -#endif - entry->makePrivate(); + case ReuseDecision::doNotCacheButShare: + entry->makePrivate(true); break; default: assert(0); break; } + debugs(11, 3, "decided: " << decision); } if (!ignoreCacheControl) { @@ -2457,3 +2442,29 @@ HttpStateData::abortAll(const char *reason) mustStop(reason); } +HttpStateData::ReuseDecision::ReuseDecision(const StoreEntry *e, const Http::StatusCode code) + : answer(HttpStateData::ReuseDecision::reuseNot), reason(nullptr), entry(e), statusCode(code) {} + +HttpStateData::ReuseDecision::Answers +HttpStateData::ReuseDecision::make(const HttpStateData::ReuseDecision::Answers ans, const char *why) +{ + answer = ans; + reason = why; + return answer; +} + +std::ostream &operator <<(std::ostream &os, const HttpStateData::ReuseDecision &d) +{ + static const char *ReuseMessages[] = { + "do not cache and do not share", // reuseNot + "cache positively and share", // cachePositively + "cache negatively and share", // cacheNegatively + "do not cache but share" // doNotCacheButShare + }; + + assert(d.answer >= HttpStateData::ReuseDecision::reuseNot && + d.answer <= HttpStateData::ReuseDecision::doNotCacheButShare); + return os << ReuseMessages[d.answer] << " because " << d.reason << + "; HTTP status " << d.statusCode << " " << *(d.entry); +} + diff --git a/src/http.h b/src/http.h index 5c85b98777..42e5d443cb 100644 --- a/src/http.h +++ b/src/http.h @@ -23,6 +23,23 @@ class HttpStateData : public Client CBDATA_CLASS(HttpStateData); public: + + /// assists in making and relaying entry caching/sharing decision + class ReuseDecision + { + public: + enum Answers { reuseNot = 0, cachePositively, cacheNegatively, doNotCacheButShare }; + + ReuseDecision(const StoreEntry *e, const Http::StatusCode code); + /// stores the corresponding decision + Answers make(const Answers ans, const char *why); + + Answers answer; ///< the decision id + const char *reason; ///< the decision reason + const StoreEntry *entry; ///< entry for debugging + const Http::StatusCode statusCode; ///< HTTP status for debugging + }; + HttpStateData(FwdState *); ~HttpStateData(); @@ -40,8 +57,8 @@ public: void readReply(const CommIoCbParams &io); virtual void maybeReadVirginBody(); // read response data from the network - // Determine whether the response is a cacheable representation - int cacheableReply(); + // Checks whether the response is cacheable/shareable. + ReuseDecision::Answers reusableReply(ReuseDecision &decision); CachePeer *_peer; /* CachePeer request made to */ int eof; /* reached end-of-object? */ @@ -135,6 +152,8 @@ private: bool sawDateGoBack; }; +std::ostream &operator <<(std::ostream &os, const HttpStateData::ReuseDecision &d); + int httpCachable(const HttpRequestMethod&); void httpStart(FwdState *); SBuf httpMakeVaryMark(HttpRequest * request, HttpReply const * reply); diff --git a/src/store.cc b/src/store.cc index 0804d8a972..9e4190fb95 100644 --- a/src/store.cc +++ b/src/store.cc @@ -147,11 +147,18 @@ StoreEntry::makePublic(const KeyScope scope) } void -StoreEntry::makePrivate() +StoreEntry::makePrivate(const bool shareable) { /* This object should never be cached at all */ expireNow(); - releaseRequest(); /* delete object when not used */ + releaseRequest(shareable); /* delete object when not used */ +} + +void +StoreEntry::clearPrivate() +{ + EBIT_CLR(flags, KEY_PRIVATE); + shareableWhenPrivate = false; } void @@ -327,7 +334,8 @@ StoreEntry::StoreEntry() : ping_status(PING_NONE), store_status(STORE_PENDING), swap_status(SWAPOUT_NONE), - lock_count(0) + lock_count(0), + shareableWhenPrivate(false) { debugs(20, 5, "StoreEntry constructed, this=" << this); } @@ -463,14 +471,14 @@ StoreEntry::setReleaseFlag() } void -StoreEntry::releaseRequest() +StoreEntry::releaseRequest(const bool shareable) { if (EBIT_TEST(flags, RELEASE_REQUEST)) return; setReleaseFlag(); // makes validToSend() false, preventing future hits - setPrivateKey(); + setPrivateKey(shareable); } int @@ -582,10 +590,14 @@ getKeyCounter(void) * concept'. */ void -StoreEntry::setPrivateKey() +StoreEntry::setPrivateKey(const bool shareable) { - if (key && EBIT_TEST(flags, KEY_PRIVATE)) - return; /* is already private */ + if (key && EBIT_TEST(flags, KEY_PRIVATE)) { + // The entry is already private, but it may be still shareable. + if (!shareable) + shareableWhenPrivate = false; + return; + } if (key) { setReleaseFlag(); // will markForUnlink(); all caches/workers will know @@ -603,6 +615,7 @@ StoreEntry::setPrivateKey() assert(hash_lookup(store_table, newkey) == NULL); EBIT_SET(flags, KEY_PRIVATE); + shareableWhenPrivate = shareable; hashInsert(newkey); } @@ -659,14 +672,17 @@ StoreEntry::forcePublicKey(const cache_key *newkey) if (StoreEntry *e2 = (StoreEntry *)hash_lookup(store_table, newkey)) { assert(e2 != this); debugs(20, 3, "Making old " << *e2 << " private."); - e2->setPrivateKey(); - e2->release(); + + // TODO: check whether there is any sense in keeping old entry + // shareable here. Leaving it non-shareable for now. + e2->setPrivateKey(false); + e2->release(false); } if (key) hashDelete(); - EBIT_CLR(flags, KEY_PRIVATE); + clearPrivate(); hashInsert(newkey); @@ -788,7 +804,7 @@ storeCreateEntry(const char *url, const char *logUrl, const RequestFlags &flags, e->lock("storeCreateEntry"); if (neighbors_do_private_keys || !flags.hierarchical) - e->setPrivateKey(); + e->setPrivateKey(false); else e->setPublicKey(); @@ -1232,7 +1248,7 @@ Store::Maintain(void *) /* release an object from a cache */ void -StoreEntry::release() +StoreEntry::release(const bool shareable) { PROF_start(storeRelease); debugs(20, 3, "releasing " << *this << ' ' << getMD5Text()); @@ -1242,7 +1258,7 @@ StoreEntry::release() if (locked()) { expireNow(); debugs(20, 3, "storeRelease: Only setting RELEASE_REQUEST bit"); - releaseRequest(); + releaseRequest(shareable); PROF_stop(storeRelease); return; } @@ -1252,7 +1268,7 @@ StoreEntry::release() Store::Root().memoryUnlink(*this); - setPrivateKey(); + setPrivateKey(shareable); // lock the entry until rebuilding is done lock("storeLateRelease"); @@ -2100,7 +2116,11 @@ std::ostream &operator <<(std::ostream &os, const StoreEntry &e) if (EBIT_TEST(e.flags, RELEASE_REQUEST)) os << 'X'; if (EBIT_TEST(e.flags, REFRESH_REQUEST)) os << 'F'; if (EBIT_TEST(e.flags, ENTRY_REVALIDATE_STALE)) os << 'E'; - if (EBIT_TEST(e.flags, ENTRY_DISPATCHED)) os << 'D'; + if (EBIT_TEST(e.flags, KEY_PRIVATE)) { + os << 'I'; + if (e.shareableWhenPrivate) + os << 'H'; + } if (EBIT_TEST(e.flags, KEY_PRIVATE)) os << 'I'; if (EBIT_TEST(e.flags, ENTRY_FWD_HDR_WAIT)) os << 'W'; if (EBIT_TEST(e.flags, ENTRY_NEGCACHED)) os << 'N'; diff --git a/src/tests/stub_store.cc b/src/tests/stub_store.cc index a3adf29859..5c6c38f128 100644 --- a/src/tests/stub_store.cc +++ b/src/tests/stub_store.cc @@ -38,11 +38,11 @@ bool StoreEntry::mayStartSwapOut() STUB_RETVAL(false) void StoreEntry::trimMemory(const bool preserveSwappable) STUB void StoreEntry::abort() STUB void StoreEntry::makePublic(const KeyScope scope) STUB -void StoreEntry::makePrivate() STUB +void StoreEntry::makePrivate(const bool shareable) STUB void StoreEntry::setPublicKey(const KeyScope scope) STUB -void StoreEntry::setPrivateKey() STUB +void StoreEntry::setPrivateKey(const bool shareable) STUB void StoreEntry::expireNow() STUB -void StoreEntry::releaseRequest() STUB +void StoreEntry::releaseRequest(const bool shareable) STUB void StoreEntry::negativeCache() STUB void StoreEntry::cacheNegatively() STUB void StoreEntry::purgeMem() STUB @@ -93,7 +93,7 @@ int64_t StoreEntry::objectLen() const STUB_RETVAL(0) int64_t StoreEntry::contentLen() const STUB_RETVAL(0) void StoreEntry::lock(const char *) STUB void StoreEntry::touch() STUB -void StoreEntry::release() STUB +void StoreEntry::release(const bool shareable) STUB void StoreEntry::append(char const *, int) STUB void StoreEntry::vappendf(const char *, va_list) STUB diff --git a/src/tests/testStoreController.cc b/src/tests/testStoreController.cc index fbbada6b26..116a50d409 100644 --- a/src/tests/testStoreController.cc +++ b/src/tests/testStoreController.cc @@ -114,7 +114,7 @@ addedEntry(Store::Disk *aStore, e->lastModified(squid_curtime); e->refcount = 1; EBIT_CLR(e->flags, RELEASE_REQUEST); - EBIT_CLR(e->flags, KEY_PRIVATE); + e->clearPrivate(); e->ping_status = PING_NONE; EBIT_CLR(e->flags, ENTRY_VALIDATED); e->hashInsert((const cache_key *)name.termedBuf()); /* do it after we clear KEY_PRIVATE */ diff --git a/src/tests/testStoreHashIndex.cc b/src/tests/testStoreHashIndex.cc index 25ae8e06b1..96ea8bdad9 100644 --- a/src/tests/testStoreHashIndex.cc +++ b/src/tests/testStoreHashIndex.cc @@ -92,7 +92,7 @@ addedEntry(Store::Disk *aStore, e->lastModified(squid_curtime); e->refcount = 1; EBIT_CLR(e->flags, RELEASE_REQUEST); - EBIT_CLR(e->flags, KEY_PRIVATE); + e->clearPrivate(); e->ping_status = PING_NONE; EBIT_CLR(e->flags, ENTRY_VALIDATED); e->hashInsert((const cache_key *)name.termedBuf()); /* do it after we clear KEY_PRIVATE */ -- 2.39.2