------------------------------------------------------------ revno: 14169 revision-id: squid3@treenet.co.nz-20170614213720-3qmiohlx4zr2jnqq parent: squid3@treenet.co.nz-20170601134753-6u64sl2rzmbfs67l fixes bug: http://bugs.squid-cache.org/show_bug.cgi?id=2833 author: Eduard Bagdasaryan committer: Amos Jeffries branch nick: 3.5 timestamp: Thu 2017-06-15 09:37:20 +1200 message: Bug 2833 pt2: 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. ------------------------------------------------------------ # Bazaar merge directive format 2 (Bazaar 0.90) # revision_id: squid3@treenet.co.nz-20170614213720-3qmiohlx4zr2jnqq # target_branch: http://bzr.squid-cache.org/bzr/squid3/3.5 # testament_sha1: 9e248e2e9d2f1defe1070eb808177df978fb4146 # timestamp: 2017-06-14 21:51:05 +0000 # source_branch: http://bzr.squid-cache.org/bzr/squid3/3.5 # base_revision_id: squid3@treenet.co.nz-20170601134753-\ # 6u64sl2rzmbfs67l # # Begin patch === modified file 'src/HttpHdrCc.cc' --- src/HttpHdrCc.cc 2017-01-01 00:16:45 +0000 +++ src/HttpHdrCc.cc 2017-06-14 21:37:20 +0000 @@ -262,8 +262,8 @@ case CC_PUBLIC: break; case CC_PRIVATE: - if (Private().size()) - packerPrintf(p, "=\"" SQUIDSTRINGPH "\"", SQUIDSTRINGPRINT(Private())); + if (private_.size()) + packerPrintf(p, "=\"" SQUIDSTRINGPH "\"", SQUIDSTRINGPRINT(private_)); break; case CC_NO_CACHE: === modified file 'src/MemStore.cc' --- src/MemStore.cc 2017-01-01 00:16:45 +0000 +++ src/MemStore.cc 2017-06-14 21:37:20 +0000 @@ -299,7 +299,7 @@ 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; === modified file 'src/Store.h' --- src/Store.h 2017-01-01 00:16:45 +0000 +++ src/Store.h 2017-06-14 21:37:20 +0000 @@ -95,15 +95,19 @@ void abort(); void unlink(); 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(); @@ -230,7 +234,13 @@ /// 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 @@ -252,6 +262,13 @@ 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; @@ -259,6 +276,8 @@ 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); === modified file 'src/client_side_reply.cc' --- src/client_side_reply.cc 2017-05-29 13:15:55 +0000 +++ src/client_side_reply.cc 2017-06-14 21:37:20 +0000 @@ -396,8 +396,8 @@ 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; @@ -530,7 +530,7 @@ // 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(); === modified file 'src/fs/rock/RockSwapDir.cc' --- src/fs/rock/RockSwapDir.cc 2017-01-01 00:16:45 +0000 +++ src/fs/rock/RockSwapDir.cc 2017-06-14 21:37:20 +0000 @@ -149,7 +149,7 @@ 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; === modified file 'src/fs/ufs/UFSSwapDir.cc' --- src/fs/ufs/UFSSwapDir.cc 2017-01-01 00:16:45 +0000 +++ src/fs/ufs/UFSSwapDir.cc 2017-06-14 21:37:20 +0000 @@ -809,7 +809,7 @@ 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); === modified file 'src/http.cc' --- src/http.cc 2017-01-01 00:16:45 +0000 +++ src/http.cc 2017-06-14 21:37:20 +0000 @@ -290,7 +290,9 @@ (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 @@ -315,12 +317,13 @@ } } -int -HttpStateData::cacheableReply() +HttpStateData::ReuseDecision::Answers +HttpStateData::reusableReply(HttpStateData::ReuseDecision &decision) { HttpReply const *rep = finalReply(); HttpHeader const *hdr = &rep->header; const char *v; + #if USE_HTTP_VIOLATIONS const RefreshPattern *R = NULL; @@ -337,24 +340,19 @@ #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) { @@ -363,11 +361,10 @@ // for now we are not reliably doing that so we waste CPU re-checking request CC // RFC 2616 section 14.9.2 - MUST NOT cache any response with request CC:no-store - if (request && request->cache_control && request->cache_control->noStore() && - !REFRESH_OVERRIDE(ignore_no_store)) { - debugs(22, 3, HERE << "NO because client request Cache-Control:no-store"); - return 0; - } + if (request && request->cache_control && request->cache_control->hasNoStore() && + !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->hasNoCache() && rep->cache_control->noCache().size() > 0) { @@ -376,19 +373,18 @@ * 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. // NP: other request CC flags are limiters on HIT/MISS. We don't care about here. // RFC 2616 section 14.9.2 - MUST NOT cache any response with CC:no-store - if (rep->cache_control && rep->cache_control->noStore() && - !REFRESH_OVERRIDE(ignore_no_store)) { - debugs(22, 3, HERE << "NO because server reply Cache-Control:no-store"); - return 0; - } + if (rep->cache_control && rep->cache_control->hasNoStore() && + !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. @@ -401,27 +397,25 @@ * 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) && !REFRESH_OVERRIDE(ignore_auth)) { - 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 - if (rep->cache_control->Public()) { + if (rep->cache_control->hasPublic()) { debugs(22, 3, HERE << "Authenticated but server reply Cache-Control:public"); mayStore = true; @@ -441,15 +435,13 @@ #endif // HTTPbis pt6 section 3.2: a response CC:s-maxage is present - } else if (rep->cache_control->sMaxAge()) { + } else if (rep->cache_control->hasSMaxAge()) { debugs(22, 3, HERE << "Authenticated but server reply Cache-Control:s-maxage"); 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. @@ -460,12 +452,26 @@ * probably should not be cachable */ if ((v = hdr->getStr(HDR_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: @@ -482,112 +488,90 @@ * Don't cache objects that need to be refreshed on next request, * unless we know how to refresh it. */ + 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"); - 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 */ 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"); + else if (rep->expires > rep->date) + decision.make(ReuseDecision::cachePositively, "Expires > Date"); + else + decision.make(ReuseDecision::doNotCacheButShare, "Expires <= Date"); break; - /* Errors can be negatively cached */ - + /* These responses can be negatively cached. Most can also be shared. */ 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; - - /* NOTREACHED */ + statusAnswer = ReuseDecision::doNotCacheButShare; + statusReason = shareableError; + // fall through to the actual decision making below + + case Http::scBadRequest: // no sharing; perhaps the server did not like something specific to this request + +#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 @@ -898,11 +882,12 @@ 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()); @@ -919,7 +904,9 @@ const SBuf vary(httpMakeVaryMark(request, 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; @@ -942,30 +929,31 @@ if (!fwd->reforwardableStatus(rep->sline.status())) EBIT_CLR(entry->flags, ENTRY_FWD_HDR_WAIT); - switch (cacheableReply()) { - - case 1: + ReuseDecision decision(entry, statusCode); + + switch (reusableReply(decision)) { + + case ReuseDecision::reuseNot: + entry->makePrivate(false); + break; + + case ReuseDecision::cachePositively: entry->makePublic(); break; - case 0: - entry->makePrivate(); + case ReuseDecision::cacheNegatively: + entry->cacheNegatively(); break; - case -1: - -#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) { @@ -2429,3 +2417,29 @@ 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); +} + === modified file 'src/http.h' --- src/http.h 2017-01-01 00:16:45 +0000 +++ src/http.h 2017-06-14 21:37:20 +0000 @@ -22,6 +22,23 @@ { 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(); @@ -39,8 +56,8 @@ 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? */ @@ -119,6 +136,8 @@ CBDATA_CLASS2(HttpStateData); }; +std::ostream &operator <<(std::ostream &os, const HttpStateData::ReuseDecision &d); + int httpCachable(const HttpRequestMethod&); void httpStart(FwdState *); SBuf httpMakeVaryMark(HttpRequest * request, HttpReply const * reply); === modified file 'src/store.cc' --- src/store.cc 2017-01-01 00:16:45 +0000 +++ src/store.cc 2017-06-14 21:37:20 +0000 @@ -171,11 +171,18 @@ } 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 @@ -365,7 +372,8 @@ 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); } @@ -504,14 +512,14 @@ } 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 @@ -623,12 +631,16 @@ * concept'. */ void -StoreEntry::setPrivateKey() +StoreEntry::setPrivateKey(const bool shareable) { const cache_key *newkey; - 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 @@ -649,6 +661,7 @@ assert(hash_lookup(store_table, newkey) == NULL); EBIT_SET(flags, KEY_PRIVATE); + shareableWhenPrivate = shareable; hashInsert(newkey); } @@ -705,14 +718,17 @@ 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); @@ -830,7 +846,7 @@ e->lock("storeCreateEntry"); if (neighbors_do_private_keys || !flags.hierarchical) - e->setPrivateKey(); + e->setPrivateKey(false); else e->setPublicKey(); @@ -1264,7 +1280,7 @@ /* release an object from a cache */ void -StoreEntry::release() +StoreEntry::release(const bool shareable) { PROF_start(storeRelease); debugs(20, 3, "releasing " << *this << ' ' << getMD5Text()); @@ -1274,7 +1290,7 @@ if (locked()) { expireNow(); debugs(20, 3, "storeRelease: Only setting RELEASE_REQUEST bit"); - releaseRequest(); + releaseRequest(shareable); PROF_stop(storeRelease); return; } @@ -1282,7 +1298,7 @@ Store::Root().memoryUnlink(*this); if (StoreController::store_dirs_rebuilding && swap_filen > -1) { - setPrivateKey(); + setPrivateKey(shareable); if (swap_filen > -1) { // lock the entry until rebuilding is done @@ -2181,7 +2197,11 @@ 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 (EBIT_TEST(e.flags, KEY_PRIVATE)) { + os << 'I'; + if (e.shareableWhenPrivate) + os << 'H'; + } if (EBIT_TEST(e.flags, ENTRY_FWD_HDR_WAIT)) os << 'W'; if (EBIT_TEST(e.flags, ENTRY_NEGCACHED)) os << 'N'; if (EBIT_TEST(e.flags, ENTRY_VALIDATED)) os << 'V'; === modified file 'src/tests/stub_store.cc' --- src/tests/stub_store.cc 2017-01-01 00:16:45 +0000 +++ src/tests/stub_store.cc 2017-06-14 21:37:20 +0000 @@ -43,11 +43,11 @@ void StoreEntry::abort() STUB void StoreEntry::unlink() STUB void StoreEntry::makePublic(const KeyScope keyScope) STUB -void StoreEntry::makePrivate() STUB +void StoreEntry::makePrivate(const bool shareable) STUB void StoreEntry::setPublicKey(const KeyScope keyScope) 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 @@ -99,7 +99,7 @@ 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 NullStoreEntry *NullStoreEntry::getInstance() STUB_RETVAL(NULL) const char *NullStoreEntry::getMD5Text() const STUB_RETVAL(NULL) === modified file 'src/tests/testStoreController.cc' --- src/tests/testStoreController.cc 2017-01-01 00:16:45 +0000 +++ src/tests/testStoreController.cc 2017-06-14 21:37:20 +0000 @@ -116,7 +116,7 @@ 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 */ === modified file 'src/tests/testStoreHashIndex.cc' --- src/tests/testStoreHashIndex.cc 2017-01-01 00:16:45 +0000 +++ src/tests/testStoreHashIndex.cc 2017-06-14 21:37:20 +0000 @@ -97,7 +97,7 @@ 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 */