From 7976fed3725088add7a2776633efdcf4eaa20077 Mon Sep 17 00:00:00 2001 From: Eduard Bagdasaryan Date: Mon, 26 Oct 2020 16:21:34 +0000 Subject: [PATCH] Removed StoreClient::created() and improved PURGE code quality (#734) The StoreClient::created() callback method was probably added in hope to make Store lookups asynchronous, but that functionality has not been implemented, leaving convoluted and dangerous synchronous created() call chains behind. Moreover, properly supporting asynchronous Store lookups in modern code is likely to require a very different API. Removal of created() allowed to greatly simplify PURGE processing, eliminating some tricky state, such as `purging` and `lookingforstore`. Also removed all Store::getPublic*() methods as no longer used. --- src/ICP.h | 10 +- src/Store.h | 3 - src/StoreClient.h | 12 +- src/client_side.cc | 6 +- src/client_side_reply.cc | 233 ++++++++++--------------------------- src/client_side_reply.h | 23 ++-- src/client_side_request.cc | 20 +--- src/client_side_request.h | 3 +- src/esi/Esi.cc | 4 +- src/htcp.cc | 22 ++-- src/icp_v2.cc | 89 +++++++------- src/icp_v3.cc | 33 ++---- src/ip/Address.h | 4 + src/mime.cc | 16 +-- src/redirect.cc | 5 +- src/servers/Http1Server.cc | 4 +- src/store.cc | 21 ---- src/store_client.cc | 2 +- src/tests/stub_icp.cc | 4 +- src/tests/stub_store.cc | 3 - src/urn.cc | 36 +++--- 21 files changed, 181 insertions(+), 372 deletions(-) diff --git a/src/ICP.h b/src/ICP.h index e4bfa7505b..9ee8255b9f 100644 --- a/src/ICP.h +++ b/src/ICP.h @@ -65,22 +65,24 @@ public: ICPState(icp_common_t &aHeader, HttpRequest *aRequest); virtual ~ICPState(); + /// whether the cache contains the requested entry + bool isHit() const; + icp_common_t header; HttpRequest *request; int fd; Ip::Address from; char *url; + mutable AccessLogEntryPointer al; protected: /* StoreClient API */ - virtual LogTags *loggingTags() override; + virtual LogTags *loggingTags() const override; virtual void fillChecklist(ACLFilledChecklist &) const override; /// either confirms and starts processing a cache hit or returns false - bool confirmAndPrepHit(const StoreEntry &); - - mutable AccessLogEntryPointer al; + bool confirmAndPrepHit(const StoreEntry &) const; }; extern Comm::ConnectionPointer icpIncomingConn; diff --git a/src/Store.h b/src/Store.h index 7bf30d479b..0fa0a1b02b 100644 --- a/src/Store.h +++ b/src/Store.h @@ -239,9 +239,6 @@ public: public: static size_t inUseCount(); - static void getPublicByRequestMethod(StoreClient * aClient, HttpRequest * request, const HttpRequestMethod& method); - static void getPublicByRequest(StoreClient * aClient, HttpRequest * request); - static void getPublic(StoreClient * aClient, const char *uri, const HttpRequestMethod& method); void *operator new(size_t byteCount); void operator delete(void *address); diff --git a/src/StoreClient.h b/src/StoreClient.h index 0d240d8b45..a43304cd8e 100644 --- a/src/StoreClient.h +++ b/src/StoreClient.h @@ -20,21 +20,15 @@ class StoreEntry; class ACLFilledChecklist; class LogTags; -/// A StoreEntry::getPublic*() caller. +/// a storeGetPublic*() caller class StoreClient { public: virtual ~StoreClient () {} - // TODO: Remove? Probably added to make lookups asynchronous, but they are - // still blocking. A lot more is needed to support async callbacks. - /// Handle a StoreEntry::getPublic*() result. - /// A nil entry indicates a cache miss. - virtual void created(StoreEntry *) = 0; - /// \return LogTags (if the class logs transactions) or nil (otherwise) - virtual LogTags *loggingTags() = 0; + virtual LogTags *loggingTags() const = 0; protected: /// configure the ACL checklist with the current transaction state @@ -43,7 +37,7 @@ protected: /// \returns whether the caller must collapse on the given entry /// Before returning true, updates common collapsing-related stats. /// See also: StoreEntry::hittingRequiresCollapsing(). - bool startCollapsingOn(const StoreEntry &, const bool doingRevalidation); + bool startCollapsingOn(const StoreEntry &, const bool doingRevalidation) const; // These methods only interpret Squid configuration. Their allowances are // provisional -- other factors may prevent collapsed forwarding. The first diff --git a/src/client_side.cc b/src/client_side.cc index 63d95b0b5e..c05fa0794a 100644 --- a/src/client_side.cc +++ b/src/client_side.cc @@ -1674,7 +1674,7 @@ clientProcessRequest(ConnStateData *conn, const Http1::RequestParserPointer &hp, clientReplyContext *repContext = dynamic_cast(node->data.getRaw()); assert (repContext); repContext->setReplyToError(ERR_UNSUP_REQ, Http::scNotImplemented, request->method, NULL, - conn->clientConnection->remote, request.getRaw(), NULL, NULL); + conn, request.getRaw(), nullptr, nullptr); assert(context->http->out.offset == 0); context->pullData(); clientProcessRequestFinished(conn, request); @@ -1689,7 +1689,7 @@ clientProcessRequest(ConnStateData *conn, const Http1::RequestParserPointer &hp, conn->quitAfterError(request.getRaw()); repContext->setReplyToError(ERR_INVALID_REQ, Http::scLengthRequired, request->method, NULL, - conn->clientConnection->remote, request.getRaw(), NULL, NULL); + conn, request.getRaw(), nullptr, nullptr); assert(context->http->out.offset == 0); context->pullData(); clientProcessRequestFinished(conn, request); @@ -1725,7 +1725,7 @@ clientProcessRequest(ConnStateData *conn, const Http1::RequestParserPointer &hp, conn->quitAfterError(request.getRaw()); repContext->setReplyToError(ERR_TOO_BIG, Http::scPayloadTooLarge, Http::METHOD_NONE, NULL, - conn->clientConnection->remote, http->request, NULL, NULL); + conn, http->request, nullptr, nullptr); assert(context->http->out.offset == 0); context->pullData(); clientProcessRequestFinished(conn, request); diff --git a/src/client_side_reply.cc b/src/client_side_reply.cc index 86d6a27aae..ba1737778e 100644 --- a/src/client_side_reply.cc +++ b/src/client_side_reply.cc @@ -53,7 +53,7 @@ CBDATA_CLASS_INIT(clientReplyContext); /* Local functions */ extern "C" CSS clientReplyStatus; -ErrorState *clientBuildError(err_type, Http::StatusCode, char const *, Ip::Address &, HttpRequest *, const AccessLogEntry::Pointer &); +ErrorState *clientBuildError(err_type, Http::StatusCode, char const *, const ConnStateData *, HttpRequest *, const AccessLogEntry::Pointer &); /* privates */ @@ -74,7 +74,6 @@ clientReplyContext::~clientReplyContext() clientReplyContext::clientReplyContext(ClientHttpRequest *clientContext) : purgeStatus(Http::scNone), - lookingforstore(0), http(cbdataReference(clientContext)), headers_sz(0), sc(NULL), @@ -100,7 +99,7 @@ clientReplyContext::clientReplyContext(ClientHttpRequest *clientContext) : void clientReplyContext::setReplyToError( err_type err, Http::StatusCode status, const HttpRequestMethod& method, char const *uri, - Ip::Address &addr, HttpRequest * failedrequest, const char *unparsedrequest, + const ConnStateData *conn, HttpRequest *failedrequest, const char *unparsedrequest, #if USE_AUTH Auth::UserRequest::Pointer auth_user_request #else @@ -108,7 +107,7 @@ clientReplyContext::setReplyToError( #endif ) { - auto errstate = clientBuildError(err, status, uri, addr, failedrequest, http->al); + auto errstate = clientBuildError(err, status, uri, conn, failedrequest, http->al); if (unparsedrequest) errstate->request_hdrs = xstrdup(unparsedrequest); @@ -760,13 +759,10 @@ clientReplyContext::processMiss() return; } - Comm::ConnectionPointer conn = http->getConn() != nullptr ? http->getConn()->clientConnection : nullptr; /// Deny loops if (r->flags.loopDetected) { http->al->http.code = Http::scForbidden; - Ip::Address tmp_noaddr; - tmp_noaddr.setNoAddr(); - err = clientBuildError(ERR_ACCESS_DENIED, Http::scForbidden, nullptr, conn ? conn->remote : tmp_noaddr, http->request, http->al); + err = clientBuildError(ERR_ACCESS_DENIED, Http::scForbidden, nullptr, http->getConn(), http->request, http->al); createStoreEntry(r->method, RequestFlags()); errorAppendEntry(http->storeEntry(), err); triggerInitialStoreRead(); @@ -788,6 +784,7 @@ clientReplyContext::processMiss() assert(r->clientConnectionManager == http->getConn()); + Comm::ConnectionPointer conn = http->getConn() != nullptr ? http->getConn()->clientConnection : nullptr; /** Start forwarding to get the new object from network */ FwdState::Start(conn, http->storeEntry(), r, http->al); } @@ -804,11 +801,8 @@ clientReplyContext::processOnlyIfCachedMiss() { debugs(88, 4, http->request->method << ' ' << http->uri); http->al->http.code = Http::scGatewayTimeout; - Ip::Address tmp_noaddr; - tmp_noaddr.setNoAddr(); ErrorState *err = clientBuildError(ERR_ONLY_IF_CACHED_MISS, Http::scGatewayTimeout, NULL, - http->getConn() ? http->getConn()->clientConnection->remote : tmp_noaddr, - http->request, http->al); + http->getConn(), http->request, http->al); removeClientStoreReference(&sc, http); startError(err); } @@ -886,18 +880,6 @@ clientReplyContext::blockedHit() const } } -void -clientReplyContext::purgeRequestFindObjectToPurge() -{ - /* Try to find a base entry */ - http->flags.purging = true; - lookingforstore = 1; - - // TODO: can we use purgeAllCached() here instead of doing the - // getPublicByRequestMethod() dance? - StoreEntry::getPublicByRequestMethod(this, http->request, Http::METHOD_GET); -} - // Purges all entries with a given url // TODO: move to SideAgent parent, when we have one /* @@ -927,25 +909,8 @@ clientReplyContext::purgeAllCached() purgeEntriesByUrl(http->request, url.c_str()); } -void -clientReplyContext::created(StoreEntry *newEntry) -{ - detailStoreLookup(newEntry ? "match" : "mismatch"); - - if (lookingforstore == 1) - purgeFoundGet(newEntry); - else if (lookingforstore == 2) - purgeFoundHead(newEntry); - else if (lookingforstore == 3) - purgeDoPurgeGet(newEntry); - else if (lookingforstore == 4) - purgeDoPurgeHead(newEntry); - else if (lookingforstore == 5) - identifyFoundObject(newEntry); -} - LogTags * -clientReplyContext::loggingTags() +clientReplyContext::loggingTags() const { // XXX: clientReplyContext code assumes that http cbdata is always valid. // TODO: Either add cbdataReferenceValid(http) checks in all the relevant @@ -953,65 +918,6 @@ clientReplyContext::loggingTags() return &http->logType; } -void -clientReplyContext::purgeFoundGet(StoreEntry *newEntry) -{ - if (!newEntry) { - lookingforstore = 2; - StoreEntry::getPublicByRequestMethod(this, http->request, Http::METHOD_HEAD); - } else - purgeFoundObject (newEntry); -} - -void -clientReplyContext::purgeFoundHead(StoreEntry *newEntry) -{ - if (!newEntry) - purgeDoMissPurge(); - else - purgeFoundObject (newEntry); -} - -void -clientReplyContext::purgeFoundObject(StoreEntry *entry) -{ - assert (entry); - - if (EBIT_TEST(entry->flags, ENTRY_SPECIAL)) { - http->logType.update(LOG_TCP_DENIED); - Ip::Address tmp_noaddr; - tmp_noaddr.setNoAddr(); // TODO: make a global const - ErrorState *err = clientBuildError(ERR_ACCESS_DENIED, Http::scForbidden, NULL, - http->getConn() ? http->getConn()->clientConnection->remote : tmp_noaddr, - http->request, http->al); - startError(err); - return; // XXX: leaking unused entry if some store does not keep it - } - - StoreIOBuffer localTempBuffer; - /* Swap in the metadata */ - http->storeEntry(entry); - - http->storeEntry()->lock("clientReplyContext::purgeFoundObject"); - http->storeEntry()->ensureMemObject(storeId(), http->log_uri, - http->request->method); - - sc = storeClientListAdd(http->storeEntry(), this); - - http->logType.update(LOG_TCP_HIT); - - reqofs = 0; - - localTempBuffer.offset = http->out.offset; - - localTempBuffer.length = next()->readBuffer.length; - - localTempBuffer.data = next()->readBuffer.data; - - storeClientCopy(sc, http->storeEntry(), - localTempBuffer, CacheHit, this); -} - void clientReplyContext::purgeRequest() { @@ -1020,10 +926,8 @@ clientReplyContext::purgeRequest() if (!Config2.onoff.enable_purge) { http->logType.update(LOG_TCP_DENIED); - Ip::Address tmp_noaddr; - tmp_noaddr.setNoAddr(); ErrorState *err = clientBuildError(ERR_ACCESS_DENIED, Http::scForbidden, NULL, - http->getConn() ? http->getConn()->clientConnection->remote : tmp_noaddr, http->request, http->al); + http->getConn(), http->request, http->al); startError(err); return; } @@ -1031,47 +935,34 @@ clientReplyContext::purgeRequest() /* Release both IP cache */ ipcacheInvalidate(http->request->url.host()); - if (!http->flags.purging) - purgeRequestFindObjectToPurge(); - else - purgeDoMissPurge(); -} - -void -clientReplyContext::purgeDoMissPurge() -{ - http->logType.update(LOG_TCP_MISS); - lookingforstore = 3; - StoreEntry::getPublicByRequestMethod(this,http->request, Http::METHOD_GET); + // TODO: can we use purgeAllCached() here instead? + purgeDoPurge(); } void -clientReplyContext::purgeDoPurgeGet(StoreEntry *newEntry) -{ - if (newEntry) { - /* Release the cached URI */ - debugs(88, 4, "clientPurgeRequest: GET '" << newEntry->url() << "'" ); -#if USE_HTCP - neighborsHtcpClear(newEntry, http->request, HttpRequestMethod(Http::METHOD_GET), HTCP_CLR_PURGE); -#endif - newEntry->release(true); - purgeStatus = Http::scOkay; +clientReplyContext::purgeDoPurge() +{ + auto firstFound = false; + if (const auto entry = storeGetPublicByRequestMethod(http->request, Http::METHOD_GET)) { + // special entries are only METHOD_GET entries without variance + if (EBIT_TEST(entry->flags, ENTRY_SPECIAL)) { + http->logType.update(LOG_TCP_DENIED); + const auto err = clientBuildError(ERR_ACCESS_DENIED, Http::scForbidden, nullptr, + http->getConn(), http->request, http->al); + startError(err); + entry->abandon(__FUNCTION__); + return; + } + firstFound = true; + if (!purgeEntry(*entry, Http::METHOD_GET)) + return; } - lookingforstore = 4; - StoreEntry::getPublicByRequestMethod(this, http->request, Http::METHOD_HEAD); -} + detailStoreLookup(storeLookupString(firstFound)); -void -clientReplyContext::purgeDoPurgeHead(StoreEntry *newEntry) -{ - if (newEntry) { - debugs(88, 4, "HEAD " << newEntry->url()); -#if USE_HTCP - neighborsHtcpClear(newEntry, http->request, HttpRequestMethod(Http::METHOD_HEAD), HTCP_CLR_PURGE); -#endif - newEntry->release(true); - purgeStatus = Http::scOkay; + if (const auto entry = storeGetPublicByRequestMethod(http->request, Http::METHOD_HEAD)) { + if (!purgeEntry(*entry, Http::METHOD_HEAD)) + return; } /* And for Vary, release the base URI if none of the headers was included in the request */ @@ -1079,26 +970,15 @@ clientReplyContext::purgeDoPurgeHead(StoreEntry *newEntry) && http->request->vary_headers.find('=') != SBuf::npos) { // XXX: performance regression, c_str() reallocates SBuf tmp(http->request->effectiveRequestUri()); - StoreEntry *entry = storeGetPublic(tmp.c_str(), Http::METHOD_GET); - if (entry) { - debugs(88, 4, "Vary GET " << entry->url()); -#if USE_HTCP - neighborsHtcpClear(entry, http->request, HttpRequestMethod(Http::METHOD_GET), HTCP_CLR_PURGE); -#endif - entry->release(true); - purgeStatus = Http::scOkay; + if (const auto entry = storeGetPublic(tmp.c_str(), Http::METHOD_GET)) { + if (!purgeEntry(*entry, Http::METHOD_GET, "Vary ")) + return; } - entry = storeGetPublic(tmp.c_str(), Http::METHOD_HEAD); - - if (entry) { - debugs(88, 4, "Vary HEAD " << entry->url()); -#if USE_HTCP - neighborsHtcpClear(entry, http->request, HttpRequestMethod(Http::METHOD_HEAD), HTCP_CLR_PURGE); -#endif - entry->release(true); - purgeStatus = Http::scOkay; + if (const auto entry = storeGetPublic(tmp.c_str(), Http::METHOD_HEAD)) { + if (!purgeEntry(*entry, Http::METHOD_HEAD, "Vary ")) + return; } } @@ -1122,6 +1002,18 @@ clientReplyContext::purgeDoPurgeHead(StoreEntry *newEntry) http->storeEntry()->complete(); } +bool +clientReplyContext::purgeEntry(StoreEntry &entry, const Http::MethodType methodType, const char *descriptionPrefix) +{ + debugs(88, 4, descriptionPrefix << Http::MethodStr(methodType) << " '" << entry.url() << "'" ); +#if USE_HTCP + neighborsHtcpClear(&entry, http->request, HttpRequestMethod(methodType), HTCP_CLR_PURGE); +#endif + entry.release(true); + purgeStatus = Http::scOkay; + return true; +} + void clientReplyContext::traceReply(clientStreamNode * node) { @@ -1689,12 +1581,11 @@ clientReplyContext::identifyStoreObject() // client sent CC:no-cache or some other condition has been // encountered which prevents delivering a public/cached object. if (!r->flags.noCache || r->flags.internal) { - lookingforstore = 5; - StoreEntry::getPublicByRequest (this, r); + const auto e = storeGetPublicByRequest(r); + identifyFoundObject(e, storeLookupString(bool(e))); } else { // "external" no-cache requests skip Store lookups - detailStoreLookup("no-cache"); - identifyFoundObject(nullptr); + identifyFoundObject(nullptr, "no-cache"); } } @@ -1703,8 +1594,10 @@ clientReplyContext::identifyStoreObject() * to see if we can determine the final status of the request. */ void -clientReplyContext::identifyFoundObject(StoreEntry *newEntry) +clientReplyContext::identifyFoundObject(StoreEntry *newEntry, const char *detail) { + detailStoreLookup(detail); + HttpRequest *r = http->request; http->storeEntry(newEntry); const auto e = http->storeEntry(); @@ -1976,12 +1869,9 @@ clientReplyContext::next() const void clientReplyContext::sendBodyTooLargeError() { - Ip::Address tmp_noaddr; - tmp_noaddr.setNoAddr(); // TODO: make a global const http->logType.update(LOG_TCP_DENIED_REPLY); ErrorState *err = clientBuildError(ERR_TOO_BIG, Http::scForbidden, NULL, - http->getConn() != NULL ? http->getConn()->clientConnection->remote : tmp_noaddr, - http->request, http->al); + http->getConn(), http->request, http->al); removeClientStoreReference(&(sc), http); HTTPMSGUNLOCK(reply); startError(err); @@ -1993,11 +1883,9 @@ void clientReplyContext::sendPreconditionFailedError() { http->logType.update(LOG_TCP_HIT); - Ip::Address tmp_noaddr; - tmp_noaddr.setNoAddr(); ErrorState *const err = clientBuildError(ERR_PRECONDITION_FAILED, Http::scPreconditionFailed, - NULL, http->getConn() ? http->getConn()->clientConnection->remote : tmp_noaddr, http->request, http->al); + nullptr, http->getConn(), http->request, http->al); removeClientStoreReference(&sc, http); HTTPMSGUNLOCK(reply); startError(err); @@ -2106,11 +1994,8 @@ clientReplyContext::processReplyAccessResult(const Acl::Answer &accessAllowed) if (page_id == ERR_NONE) page_id = ERR_ACCESS_DENIED; - Ip::Address tmp_noaddr; - tmp_noaddr.setNoAddr(); err = clientBuildError(page_id, Http::scForbidden, NULL, - http->getConn() != NULL ? http->getConn()->clientConnection->remote : tmp_noaddr, - http->request, http->al); + http->getConn(), http->request, http->al); removeClientStoreReference(&sc, http); @@ -2341,10 +2226,10 @@ clientReplyContext::createStoreEntry(const HttpRequestMethod& m, RequestFlags re ErrorState * clientBuildError(err_type page_id, Http::StatusCode status, char const *url, - Ip::Address &src_addr, HttpRequest * request, const AccessLogEntry::Pointer &al) + const ConnStateData *conn, HttpRequest *request, const AccessLogEntry::Pointer &al) { const auto err = new ErrorState(page_id, status, request, al); - err->src_addr = src_addr; + err->src_addr = conn && conn->clientConnection ? conn->clientConnection->remote : Ip::Address::NoAddr(); if (url) err->url = xstrdup(url); diff --git a/src/client_side_reply.h b/src/client_side_reply.h index b55a18aa36..dd33e62caf 100644 --- a/src/client_side_reply.h +++ b/src/client_side_reply.h @@ -34,23 +34,16 @@ public: void saveState(); void restoreState(); void purgeRequest (); - void purgeRequestFindObjectToPurge(); - void purgeDoMissPurge(); - void purgeFoundGet(StoreEntry *newEntry); - void purgeFoundHead(StoreEntry *newEntry); - void purgeFoundObject(StoreEntry *entry); void sendClientUpstreamResponse(); - void purgeDoPurgeGet(StoreEntry *entry); - void purgeDoPurgeHead(StoreEntry *entry); void doGetMoreData(); void identifyStoreObject(); - void identifyFoundObject(StoreEntry *entry); + void identifyFoundObject(StoreEntry *entry, const char *detail); int storeOKTransferDone() const; int storeNotOKTransferDone() const; /// replaces current response store entry with the given one void setReplyToStoreEntry(StoreEntry *e, const char *reason); /// builds error using clientBuildError() and calls setReplyToError() below - void setReplyToError(err_type, Http::StatusCode, const HttpRequestMethod&, char const *, Ip::Address &, HttpRequest *, const char *, + void setReplyToError(err_type, Http::StatusCode, const HttpRequestMethod&, char const *, const ConnStateData *, HttpRequest *, const char *, #if USE_AUTH Auth::UserRequest::Pointer); #else @@ -72,12 +65,8 @@ public: Http::StatusCode purgeStatus; - /* state variable - replace with class to handle storeentries at some point */ - int lookingforstore; - /* StoreClient API */ - virtual void created (StoreEntry *newEntry); - virtual LogTags *loggingTags(); + virtual LogTags *loggingTags() const; ClientHttpRequest *http; /// Base reply header bytes received from Store. @@ -127,8 +116,14 @@ private: void triggerInitialStoreRead(); void sendClientOldEntry(); void purgeAllCached(); + /// attempts to release the cached entry + /// \returns whether the entry was released + bool purgeEntry(StoreEntry &, const Http::MethodType, const char *descriptionPrefix = ""); + /// releases both cached GET and HEAD entries + void purgeDoPurge(); void forgetHit(); bool blockedHit() const; + const char *storeLookupString(bool found) const { return found ? "match" : "mismatch"; } void detailStoreLookup(const char *detail); void sendBodyTooLargeError(); diff --git a/src/client_side_request.cc b/src/client_side_request.cc index f6c7469307..cde14e5e17 100644 --- a/src/client_side_request.cc +++ b/src/client_side_request.cc @@ -84,7 +84,7 @@ static const char *const crlf = "\r\n"; static void clientFollowXForwardedForCheck(Acl::Answer answer, void *data); #endif /* FOLLOW_X_FORWARDED_FOR */ -ErrorState *clientBuildError(err_type, Http::StatusCode, char const *url, Ip::Address &, HttpRequest *, const AccessLogEntry::Pointer &); +ErrorState *clientBuildError(err_type, Http::StatusCode, char const *url, const ConnStateData *, HttpRequest *, const AccessLogEntry::Pointer &); CBDATA_CLASS_INIT(ClientRequestContext); @@ -569,7 +569,7 @@ ClientRequestContext::hostHeaderVerifyFailed(const char *A, const char *B) assert (repContext); repContext->setReplyToError(ERR_CONFLICT_HOST, Http::scConflict, http->request->method, NULL, - http->getConn()->clientConnection->remote, + http->getConn(), http->request, NULL, #if USE_AUTH @@ -802,13 +802,7 @@ ClientRequestContext::clientAccessCheckDone(const Acl::Answer &answer) page_id = ERR_ACCESS_DENIED; } - Ip::Address tmpnoaddr; - tmpnoaddr.setNoAddr(); - error = clientBuildError(page_id, status, - NULL, - http->getConn() != NULL ? http->getConn()->clientConnection->remote : tmpnoaddr, - http->request, http->al - ); + error = clientBuildError(page_id, status, nullptr, http->getConn(), http->request, http->al); #if USE_AUTH error->auth_user_request = @@ -2176,15 +2170,9 @@ ClientHttpRequest::calloutsError(const err_type error, const int errDetail) // setReplyToError, but it seems unlikely that the errno reflects the // true cause of the error at this point, so I did not pass it. if (calloutContext) { - Ip::Address noAddr; - noAddr.setNoAddr(); ConnStateData * c = getConn(); calloutContext->error = clientBuildError(error, Http::scInternalServerError, - NULL, - c != NULL ? c->clientConnection->remote : noAddr, - request, - al - ); + nullptr, c, request, al); #if USE_AUTH calloutContext->error->auth_user_request = c != NULL && c->getAuth() != NULL ? c->getAuth() : request->auth_user_request; diff --git a/src/client_side_request.h b/src/client_side_request.h index ecffb21cae..8b9a548490 100644 --- a/src/client_side_request.h +++ b/src/client_side_request.h @@ -132,12 +132,11 @@ public: AccessLogEntry::Pointer al; ///< access.log entry struct Flags { - Flags() : accel(false), internal(false), done_copying(false), purging(false) {} + Flags() : accel(false), internal(false), done_copying(false) {} bool accel; bool internal; bool done_copying; - bool purging; } flags; struct Redirect { diff --git a/src/esi/Esi.cc b/src/esi/Esi.cc index a510182d25..25753bf1aa 100644 --- a/src/esi/Esi.cc +++ b/src/esi/Esi.cc @@ -1421,7 +1421,7 @@ ESIContext::freeResources () /* don't touch incoming, it's a pointer into buffered anyway */ } -ErrorState *clientBuildError(err_type, Http::StatusCode, char const *, Ip::Address &, HttpRequest *, const AccessLogEntry::Pointer &); +ErrorState *clientBuildError(err_type, Http::StatusCode, char const *, const ConnStateData *, HttpRequest *, const AccessLogEntry::Pointer &); /* This can ONLY be used before we have sent *any* data to the client */ void @@ -1439,7 +1439,7 @@ ESIContext::fail () flags.error = 1; /* create an error object */ // XXX: with the in-direction on remote IP. does the http->getConn()->clientConnection exist? - const auto err = clientBuildError(errorpage, errorstatus, nullptr, http->getConn()->clientConnection->remote, http->request, http->al); + const auto err = clientBuildError(errorpage, errorstatus, nullptr, http->getConn(), http->request, http->al); err->err_msg = errormessage; errormessage = NULL; rep = err->BuildHttpReply(); diff --git a/src/htcp.cc b/src/htcp.cc index fa04feb204..1b15cebc20 100644 --- a/src/htcp.cc +++ b/src/htcp.cc @@ -139,8 +139,7 @@ public: virtual std::ostream &detailCodeContext(std::ostream &os) const; // override /* StoreClient API */ - void created(StoreEntry *); - virtual LogTags *loggingTags(); + virtual LogTags *loggingTags() const; virtual void fillChecklist(ACLFilledChecklist &) const; public: @@ -969,24 +968,19 @@ htcpSpecifier::checkHit() return; } - StoreEntry::getPublicByRequest(this, checkHitRequest.getRaw()); -} - -void -htcpSpecifier::created(StoreEntry *e) -{ + const auto e = storeGetPublicByRequest(checkHitRequest.getRaw()); StoreEntry *hit = nullptr; if (!e) { - debugs(31, 3, "htcpCheckHit: NO; public object not found"); + debugs(31, 3, "NO; public object not found"); } else if (!e->validToSend()) { - debugs(31, 3, "htcpCheckHit: NO; entry not valid to send" ); + debugs(31, 3, "NO; entry not valid to send" ); } else if (refreshCheckHTCP(e, checkHitRequest.getRaw())) { - debugs(31, 3, "htcpCheckHit: NO; cached response is stale"); + debugs(31, 3, "NO; cached response is stale"); } else if (e->hittingRequiresCollapsing() && !startCollapsingOn(*e, false)) { - debugs(31, 3, "htcpCheckHit: NO; prohibited CF hit: " << *e); + debugs(31, 3, "NO; prohibited CF hit: " << *e); } else { - debugs(31, 3, "htcpCheckHit: YES!?"); + debugs(31, 3, "YES!?"); hit = e; } @@ -998,7 +992,7 @@ htcpSpecifier::created(StoreEntry *e) } LogTags * -htcpSpecifier::loggingTags() +htcpSpecifier::loggingTags() const { // calling htcpSyncAle() here would not change cache.code if (!al) diff --git a/src/icp_v2.cc b/src/icp_v2.cc index e1491c5db0..1c5fd10f46 100644 --- a/src/icp_v2.cc +++ b/src/icp_v2.cc @@ -155,7 +155,20 @@ ICPState::~ICPState() } bool -ICPState::confirmAndPrepHit(const StoreEntry &e) +ICPState::isHit() const +{ + const auto e = storeGetPublic(url, Http::METHOD_GET); + + const auto hit = e && confirmAndPrepHit(*e); + + if (e) + e->abandon(__FUNCTION__); + + return hit; +} + +bool +ICPState::confirmAndPrepHit(const StoreEntry &e) const { if (!e.validToSend()) return false; @@ -170,7 +183,7 @@ ICPState::confirmAndPrepHit(const StoreEntry &e) } LogTags * -ICPState::loggingTags() +ICPState::loggingTags() const { // calling icpSyncAle(LOG_TAG_NONE) here would not change cache.code if (!al) @@ -199,7 +212,6 @@ public: ICPState(aHeader, aRequest),rtt(0),src_rtt(0),flags(0) {} ~ICP2State(); - virtual void created(StoreEntry * newEntry) override; int rtt; int src_rtt; @@ -209,39 +221,6 @@ public: ICP2State::~ICP2State() {} -void -ICP2State::created(StoreEntry *e) -{ - debugs(12, 5, "icpHandleIcpV2: OPCODE " << icp_opcode_str[header.opcode]); - icp_opcode codeToSend; - - if (e && confirmAndPrepHit(*e)) { - codeToSend = ICP_HIT; - } else { -#if USE_ICMP - if (Config.onoff.test_reachability && rtt == 0) { - if ((rtt = netdbHostRtt(request->url.host())) == 0) - netdbPingSite(request->url.host()); - } -#endif /* USE_ICMP */ - - if (icpGetCommonOpcode() != ICP_ERR) - codeToSend = icpGetCommonOpcode(); - else if (Config.onoff.test_reachability && rtt == 0) - codeToSend = ICP_MISS_NOFETCH; - else - codeToSend = ICP_MISS; - } - - icpCreateAndSend(codeToSend, flags, url, header.reqnum, src_rtt, fd, from, al); - - // TODO: StoreClients must either store/lock or abandon found entries. - //if (e) - // e->abandon(); - - delete this; -} - /* End ICP2State */ /// updates ALE (if any) and logs the transaction (if needed) @@ -536,15 +515,35 @@ doV2Query(int fd, Ip::Address &from, char *buf, icp_common_t header) #endif /* USE_ICMP */ /* The peer is allowed to use this cache */ - ICP2State *state = new ICP2State(header, icp_request); - state->fd = fd; - state->from = from; - state->url = xstrdup(url); - state->flags = flags; - state->rtt = rtt; - state->src_rtt = src_rtt; + ICP2State state(header, icp_request); + state.fd = fd; + state.from = from; + state.url = xstrdup(url); + state.flags = flags; + state.rtt = rtt; + state.src_rtt = src_rtt; - StoreEntry::getPublic(state, url, Http::METHOD_GET); + icp_opcode codeToSend; + + if (state.isHit()) { + codeToSend = ICP_HIT; + } else { +#if USE_ICMP + if (Config.onoff.test_reachability && state.rtt == 0) { + if ((state.rtt = netdbHostRtt(state.request->url.host())) == 0) + netdbPingSite(state.request->url.host()); + } +#endif /* USE_ICMP */ + + if (icpGetCommonOpcode() != ICP_ERR) + codeToSend = icpGetCommonOpcode(); + else if (Config.onoff.test_reachability && rtt == 0) + codeToSend = ICP_MISS_NOFETCH; + else + codeToSend = ICP_MISS; + } + + icpCreateAndSend(codeToSend, flags, url, header.reqnum, src_rtt, fd, from, state.al); HTTPMSGUNLOCK(icp_request); } @@ -584,6 +583,8 @@ icpHandleIcpV2(int fd, Ip::Address &from, char *buf, int len) return; } + debugs(12, 5, "OPCODE " << icp_opcode_str[header.getOpCode()] << '=' << uint8_t(header.opcode)); + switch (header.opcode) { case ICP_QUERY: diff --git a/src/icp_v3.cc b/src/icp_v3.cc index 4daeac10ad..8e368b32ba 100644 --- a/src/icp_v3.cc +++ b/src/icp_v3.cc @@ -27,8 +27,7 @@ public: ICP3State(icp_common_t &aHeader, HttpRequest *aRequest) : ICPState(aHeader, aRequest) {} - ~ICP3State(); - void created (StoreEntry *newEntry); + ~ICP3State() = default; }; /// \ingroup ServerProtocolICPInternal3 @@ -49,37 +48,21 @@ doV3Query(int fd, Ip::Address &from, char *buf, icp_common_t header) } /* The peer is allowed to use this cache */ - ICP3State *state = new ICP3State (header, icp_request); - state->fd = fd; - state->from = from; - state->url = xstrdup(url); + ICP3State state(header, icp_request); + state.fd = fd; + state.from = from; + state.url = xstrdup(url); - StoreEntry::getPublic (state, url, Http::METHOD_GET); -} - -ICP3State::~ICP3State() -{} - -void -ICP3State::created(StoreEntry *e) -{ - debugs(12, 5, "icpHandleIcpV3: OPCODE " << icp_opcode_str[header.opcode]); icp_opcode codeToSend; - if (e && confirmAndPrepHit(*e)) { + if (state.isHit()) { codeToSend = ICP_HIT; } else if (icpGetCommonOpcode() == ICP_ERR) codeToSend = ICP_MISS; else codeToSend = icpGetCommonOpcode(); - icpCreateAndSend(codeToSend, 0, url, header.reqnum, 0, fd, from, al); - - // TODO: StoreClients must either store/lock or abandon found entries. - //if (e) - // e->abandon(); - - delete this; + icpCreateAndSend(codeToSend, 0, url, header.reqnum, 0, fd, from, state.al); } /// \ingroup ServerProtocolICPInternal3 @@ -102,6 +85,8 @@ icpHandleIcpV3(int fd, Ip::Address &from, char *buf, int len) return; } + debugs(12, 5, "OPCODE " << icp_opcode_str[header.getOpCode()] << '=' << uint8_t(header.opcode)); + switch (header.opcode) { case ICP_QUERY: diff --git a/src/ip/Address.h b/src/ip/Address.h index 6f7cd042b3..7555e28ff5 100644 --- a/src/ip/Address.h +++ b/src/ip/Address.h @@ -298,6 +298,10 @@ public: */ bool GetHostByName(const char *s); + /// \returns an Address with true isNoAddr() + /// \see isNoAddr() for more details + static const Address &NoAddr() { static const Address noAddr(v6_noaddr); return noAddr; } + public: /* XXX: When C => C++ conversion is done will be fully private. * Legacy Transition Methods. diff --git a/src/mime.cc b/src/mime.cc index 5acdd8f260..45fdaf66f5 100644 --- a/src/mime.cc +++ b/src/mime.cc @@ -45,8 +45,7 @@ public: void load(); /* StoreClient API */ - virtual void created(StoreEntry *); - virtual LogTags *loggingTags() { return nullptr; } // no access logging/ACLs + virtual LogTags *loggingTags() const { return nullptr; } // no access logging/ACLs virtual void fillChecklist(ACLFilledChecklist &) const; private: @@ -356,15 +355,12 @@ MimeIcon::load() if (type == NULL) fatal("Unknown icon format while reading mime.conf\n"); - StoreEntry::getPublic(this, url_, Http::METHOD_GET); -} - -void -MimeIcon::created(StoreEntry *newEntry) -{ - /* if the icon is already in the store, do nothing */ - if (newEntry) + if (const auto e = storeGetPublic(url_, Http::METHOD_GET)) { + // do not overwrite an already stored icon + e->abandon(__FUNCTION__); return; + } + // XXX: if a 204 is cached due to earlier load 'failure' we should try to reload. // default is a 200 object with image data. diff --git a/src/redirect.cc b/src/redirect.cc index 3af54f5816..063d06f160 100644 --- a/src/redirect.cc +++ b/src/redirect.cc @@ -258,12 +258,9 @@ constructHelperQuery(const char *name, helper *hlp, HLPCB *replyHandler, ClientH clientStreamNode *node = (clientStreamNode *)http->client_stream.tail->prev->data; clientReplyContext *repContext = dynamic_cast(node->data.getRaw()); assert (repContext); - Ip::Address tmpnoaddr; - tmpnoaddr.setNoAddr(); repContext->setReplyToError(ERR_GATEWAY_FAILURE, status, http->request->method, NULL, - http->getConn() != NULL && http->getConn()->clientConnection != NULL ? - http->getConn()->clientConnection->remote : tmpnoaddr, + http->getConn(), http->request, NULL, #if USE_AUTH diff --git a/src/servers/Http1Server.cc b/src/servers/Http1Server.cc index c457ba245c..9ac3673024 100644 --- a/src/servers/Http1Server.cc +++ b/src/servers/Http1Server.cc @@ -220,7 +220,7 @@ Http::One::Server::setReplyError(Http::StreamPointer &context, HttpRequest::Poin clientReplyContext *repContext = dynamic_cast(node->data.getRaw()); assert (repContext); - repContext->setReplyToError(requestError, errStatusCode, method, context->http->uri, clientConnection->remote, nullptr, requestErrorBytes, nullptr); + repContext->setReplyToError(requestError, errStatusCode, method, context->http->uri, this, nullptr, requestErrorBytes, nullptr); assert(context->http->out.offset == 0); context->pullData(); @@ -264,7 +264,7 @@ Http::One::Server::processParsedRequest(Http::StreamPointer &context) clientReplyContext *repContext = dynamic_cast(node->data.getRaw()); assert (repContext); repContext->setReplyToError(ERR_INVALID_REQ, Http::scExpectationFailed, request->method, http->uri, - clientConnection->remote, request.getRaw(), NULL, NULL); + this, request.getRaw(), nullptr, nullptr); assert(context->http->out.offset == 0); context->pullData(); clientProcessRequestFinished(this, request); diff --git a/src/store.cc b/src/store.cc index 094c00c1a2..450ae630cd 100644 --- a/src/store.cc +++ b/src/store.cc @@ -517,27 +517,6 @@ StoreEntry::doAbandon(const char *context) Store::Root().handleIdleEntry(*this); // may delete us } -void -StoreEntry::getPublicByRequestMethod (StoreClient *aClient, HttpRequest * request, const HttpRequestMethod& method) -{ - assert (aClient); - aClient->created(storeGetPublicByRequestMethod(request, method)); -} - -void -StoreEntry::getPublicByRequest (StoreClient *aClient, HttpRequest * request) -{ - assert (aClient); - aClient->created(storeGetPublicByRequest(request)); -} - -void -StoreEntry::getPublic (StoreClient *aClient, const char *uri, const HttpRequestMethod& method) -{ - assert (aClient); - aClient->created(storeGetPublic(uri, method)); -} - StoreEntry * storeGetPublic(const char *uri, const HttpRequestMethod& method) { diff --git a/src/store_client.cc b/src/store_client.cc index ae1b07fc59..6e79c4365e 100644 --- a/src/store_client.cc +++ b/src/store_client.cc @@ -63,7 +63,7 @@ StoreClient::onCollapsingPath() const } bool -StoreClient::startCollapsingOn(const StoreEntry &e, const bool doingRevalidation) +StoreClient::startCollapsingOn(const StoreEntry &e, const bool doingRevalidation) const { if (!e.hittingRequiresCollapsing()) return false; // collapsing is impossible due to the entry state diff --git a/src/tests/stub_icp.cc b/src/tests/stub_icp.cc index 6d4538b3d3..939660bd29 100644 --- a/src/tests/stub_icp.cc +++ b/src/tests/stub_icp.cc @@ -21,8 +21,8 @@ icp_common_t *icp_common_t::CreateMessage(icp_opcode opcode, int flags, const ch icp_opcode icp_common_t::getOpCode() const STUB_RETVAL(ICP_INVALID) ICPState::ICPState(icp_common_t &aHeader, HttpRequest *aRequest) STUB ICPState::~ICPState() STUB -bool ICPState::confirmAndPrepHit(const StoreEntry &) STUB_RETVAL(false) -LogTags *ICPState::loggingTags() STUB_RETVAL(nullptr) +bool ICPState::confirmAndPrepHit(const StoreEntry &) const STUB_RETVAL(false) +LogTags *ICPState::loggingTags() const STUB_RETVAL(nullptr) void ICPState::fillChecklist(ACLFilledChecklist&) const STUB Comm::ConnectionPointer icpIncomingConn; diff --git a/src/tests/stub_store.cc b/src/tests/stub_store.cc index bcbebc55cb..a3f87e52ae 100644 --- a/src/tests/stub_store.cc +++ b/src/tests/stub_store.cc @@ -71,9 +71,6 @@ bool StoreEntry::hasIfMatchEtag(const HttpRequest &request) const STUB_RETVAL(fa bool StoreEntry::hasIfNoneMatchEtag(const HttpRequest &request) const STUB_RETVAL(false) Store::Disk &StoreEntry::disk() const STUB_RETREF(Store::Disk) size_t StoreEntry::inUseCount() STUB_RETVAL(0) -void StoreEntry::getPublicByRequestMethod(StoreClient * aClient, HttpRequest * request, const HttpRequestMethod& method) STUB -void StoreEntry::getPublicByRequest(StoreClient * aClient, HttpRequest * request) STUB -void StoreEntry::getPublic(StoreClient * aClient, const char *uri, const HttpRequestMethod& method) STUB void *StoreEntry::operator new(size_t byteCount) { STUB diff --git a/src/urn.cc b/src/urn.cc index 32f3b6688d..69c29b75f4 100644 --- a/src/urn.cc +++ b/src/urn.cc @@ -37,7 +37,6 @@ class UrnState : public StoreClient public: explicit UrnState(const AccessLogEntry::Pointer &anAle): ale(anAle) {} - void created (StoreEntry *newEntry); void start (HttpRequest *, StoreEntry *); void setUriResFromRequest(HttpRequest *); @@ -55,7 +54,7 @@ public: private: /* StoreClient API */ - virtual LogTags *loggingTags() { return ale ? &ale->cache.code : nullptr; } + virtual LogTags *loggingTags() const { return ale ? &ale->cache.code : nullptr; } virtual void fillChecklist(ACLFilledChecklist &) const; char *urlres = nullptr; @@ -175,29 +174,19 @@ UrnState::start(HttpRequest * r, StoreEntry * e) if (urlres_r == NULL) return; - StoreEntry::getPublic (this, urlres, Http::METHOD_GET); -} + auto urlEntry = storeGetPublic(urlres, Http::METHOD_GET); -void -UrnState::fillChecklist(ACLFilledChecklist &checklist) const -{ - checklist.setRequest(request.getRaw()); - checklist.al = ale; -} - -void -UrnState::created(StoreEntry *e) -{ - if (!e || (e->hittingRequiresCollapsing() && !startCollapsingOn(*e, false))) { + if (!urlEntry || (urlEntry->hittingRequiresCollapsing() && !startCollapsingOn(*urlEntry, false))) { urlres_e = storeCreateEntry(urlres, urlres, RequestFlags(), Http::METHOD_GET); sc = storeClientListAdd(urlres_e, this); FwdState::Start(Comm::ConnectionPointer(), urlres_e, urlres_r.getRaw(), ale); - // TODO: StoreClients must either store/lock or abandon found entries. - //if (e) - // e->abandon(); + if (urlEntry) { + urlEntry->abandon(__FUNCTION__); + urlEntry = nullptr; + } } else { - urlres_e = e; - urlres_e->lock("UrnState::created"); + urlres_e = urlEntry; + urlres_e->lock("UrnState::start"); sc = storeClientListAdd(urlres_e, this); } @@ -212,6 +201,13 @@ UrnState::created(StoreEntry *e) this); } +void +UrnState::fillChecklist(ACLFilledChecklist &checklist) const +{ + checklist.setRequest(request.getRaw()); + checklist.al = ale; +} + void urnStart(HttpRequest *r, StoreEntry *e, const AccessLogEntryPointer &ale) { -- 2.47.2