From 66d51f4febd9fb534d065e50423addae265ee966 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Sat, 12 Oct 2019 00:40:35 +0000 Subject: [PATCH] Re-enabled updates of stored headers on HTTP 304 responses (#485) Commit 60ba25f disabled header updates (introduced in commit abf396e) after we discovered critical inconsistencies in related metadata updates. Finding a way to keep metadata consistent proved to be very difficult. The primary challenge is the multitude of often concurrent and semi-dependent activities associated with a single StoreEntry object (e.g., writing an incoming miss response into RAM, caching the response, loading a cache hit into RAM, and sending a response to the client). Concurrent activities (re)set or use overlapping sets of 304-dependent members, including StoreEntry "basics" (e.g. StoreEntry::swap_file_sz and timestamp), "reply" (MemObject::reply_ including its hdr_sz member), and "data" (MemObject::data_hdr). A data member update by one activity affects other activities. Splitting one StoreEntry object into two internally consistent and "constant" StoreEntry objects (one old and one updated) does not work well because there is no mechanism to share StoreEntry "data" storage and invokeHandlers() call streams after the split. To avoid crashes and response corruption due to inconsistent sizes and offsets, all size-related data members must be based on the same entry "version". If we update one such member, we must update all others. Furthermore, due to copying of information into activity-local variables/state, we cannot update anything while an activity is still running. For example, two HTTP clients may use the same StoreEntry to receive data, and one of them may already be in the middle of response sending, using old response offsets/sizes, when a 304 update arrives for the other. With any updates of size-related StoreEntry data members ruled out, the only remaining solution for preserving consistency is to keep all those members constant/stale despite the 304 update! The updated size-related info has to go somewhere else (inside the same StoreEntry object). The updated headers are now stored in a new MemObject::updatedReply field. The updated headers are swapped out, but the old StoreEntry is not (and was not before these changes) associated with the new store entry anchor. After the old StoreEntry is abandoned, new local hits will use the updated anchors. Other worker hits will use the updated anchors immediately, but they will create fresh StoreEntry objects. We update freshness-related data members because the associated instant decisions should not lead to inconsistencies, and using the latest info is preferable. If any counter-examples are found, we may have to split StoreEntry::timestamp/etc. fields into stale and fresh groups. We do not update Vary-related data members. See rationale below[1]. Also removed HttpHeader::update() code that disabled needUpdate() checks for non-CF configurations. The check is expensive but storing the updated response is a lot more expensive so even if a small fraction of checks prevents updates, we may improve performance. Also moved the corresponding code into HttpReply so that most Header::update() callers (that have nothing to do with 304 updates) do not spend time on it. Also adjusted CheckQuickAbortIsReasonable(): The old expectlen setting code did not check for unknown content length, relying on "-1 + hdr_sz" to be negative only when no data has been received. We now use a more direct (but possibly still broken for HTTP/0) test (hdr_sz <= 0) and avoid using unknown content_length in arithmetic expressions. HTTP/0 aside, responses without Content-Length should still be aborted but now with a correct "unknown content length" debug message. MemObject is always constructed with an (empty) base reply. We now also assert that MemObject always has a (possibly empty) base reply after checking that all [indirect] replaceBaseReply() callers either * supply a non-nil reply or * call replaceHttpReply() with a true andStartWriting parameter, going through an existing assert(rep) in StoreEntry::startWriting(). [1] Why exclude Vary response headers from 304 updates? RFC 7234 Section 4.3.4 requires that Squid updates cached Vary. However, reacting to changed variance requires either forgetting all cached entries attached to the old Vary mark entry (bad for caching) or re-keying all those entries using a new variance and a new Vary mark. Re-keying requires both maintaining a list of cached Vary-controlled entries and caching _request_ headers for every such entry! Whether HTTP compliance is worth this complexity is debatable. Perhaps origins should not return 304s to change variance? FWIW, Fastly folks decided that it is _not_ worth it for them; look for the "Side note" in https://www.smashingmagazine.com/2017/11/understanding-vary-header/ --- src/FwdState.cc | 13 +-- src/HttpHeader.cc | 26 +++--- src/HttpHeader.h | 8 +- src/HttpReply.cc | 34 ++++---- src/HttpReply.h | 6 +- src/MemObject.cc | 53 +++++++++--- src/MemObject.h | 47 +++++++++-- src/MemStore.cc | 9 +-- src/Store.h | 28 ++++--- src/StoreMeta.h | 2 +- src/StoreMetaUnpacker.h | 9 --- src/acl/Asn.cc | 2 +- src/client_side.cc | 27 ++++--- src/client_side_reply.cc | 60 +++++++------- src/client_side_reply.h | 3 + src/client_side_request.cc | 4 +- src/client_side_request.h | 8 ++ src/clients/Client.cc | 2 +- src/fs/rock/RockHeaderUpdater.cc | 38 ++++++++- src/http.cc | 8 +- src/http/Stream.cc | 5 +- src/icmp/net_db.cc | 9 +-- src/ipc/StoreMap.cc | 5 +- src/ipc/StoreMap.h | 4 +- src/peer_digest.cc | 26 +++--- src/refresh.cc | 8 +- src/store.cc | 135 +++++++++++++++---------------- src/store/Controller.cc | 34 +++++--- src/store/Controller.h | 4 +- src/store_client.cc | 29 ++++--- src/store_log.cc | 2 +- src/store_swapmeta.cc | 2 +- src/store_swapout.cc | 2 +- src/tests/stub_HttpHeader.cc | 2 +- src/tests/stub_HttpReply.cc | 2 +- src/tests/stub_libstore.cc | 2 +- src/tests/stub_store.cc | 7 +- src/tests/stub_store_swapout.cc | 2 +- src/tests/testRock.cc | 6 +- src/tests/testUfs.cc | 6 +- src/urn.cc | 2 +- 41 files changed, 408 insertions(+), 273 deletions(-) diff --git a/src/FwdState.cc b/src/FwdState.cc index f8954f7185..09053541fe 100644 --- a/src/FwdState.cc +++ b/src/FwdState.cc @@ -511,16 +511,17 @@ FwdState::unregister(int fd) void FwdState::complete() { - debugs(17, 3, HERE << entry->url() << "\n\tstatus " << entry->getReply()->sline.status()); + const auto replyStatus = entry->mem().baseReply().sline.status(); + debugs(17, 3, *entry << " status " << replyStatus << ' ' << entry->url()); #if URL_CHECKSUM_DEBUG entry->mem_obj->checkUrlChecksum(); #endif - logReplyStatus(n_tries, entry->getReply()->sline.status()); + logReplyStatus(n_tries, replyStatus); if (reforward()) { - debugs(17, 3, HERE << "re-forwarding " << entry->getReply()->sline.status() << " " << entry->url()); + debugs(17, 3, "re-forwarding " << replyStatus << " " << entry->url()); if (Comm::IsConnOpen(serverConn)) unregister(serverConn); @@ -531,9 +532,9 @@ FwdState::complete() } else { if (Comm::IsConnOpen(serverConn)) - debugs(17, 3, HERE << "server FD " << serverConnection()->fd << " not re-forwarding status " << entry->getReply()->sline.status()); + debugs(17, 3, "server FD " << serverConnection()->fd << " not re-forwarding status " << replyStatus); else - debugs(17, 3, HERE << "server (FD closed) not re-forwarding status " << entry->getReply()->sline.status()); + debugs(17, 3, "server (FD closed) not re-forwarding status " << replyStatus); entry->complete(); if (!Comm::IsConnOpen(serverConn)) @@ -1200,7 +1201,7 @@ FwdState::reforward() return 0; } - const Http::StatusCode s = e->getReply()->sline.status(); + const auto s = entry->mem().baseReply().sline.status(); debugs(17, 3, HERE << "status " << s); return reforwardableStatus(s); } diff --git a/src/HttpHeader.cc b/src/HttpHeader.cc index d161cbfa29..6965a3412c 100644 --- a/src/HttpHeader.cc +++ b/src/HttpHeader.cc @@ -157,6 +157,7 @@ HttpHeader::HttpHeader(const http_hdr_owner_type anOwner): owner(anOwner), len(0 httpHeaderMaskInit(&mask, 0); } +// XXX: Delete as unused, expensive, and violating copy semantics by skipping Warnings HttpHeader::HttpHeader(const HttpHeader &other): owner(other.owner), len(other.len), conflictingContentLength_(false) { entries.reserve(other.entries.capacity()); @@ -169,6 +170,7 @@ HttpHeader::~HttpHeader() clean(); } +// XXX: Delete as unused, expensive, and violating assignment semantics by skipping Warnings HttpHeader & HttpHeader::operator =(const HttpHeader &other) { @@ -244,10 +246,16 @@ HttpHeader::append(const HttpHeader * src) } } -/// check whether the fresh header has any new/changed updatable fields bool HttpHeader::needUpdate(HttpHeader const *fresh) const { + // our 1xx Warnings must be removed + for (const auto e: entries) { + // TODO: Move into HttpHeaderEntry::is1xxWarning() before official commit. + if (e && e->id == Http::HdrType::WARNING && (e->getInt()/100 == 1)) + return true; + } + for (const auto e: fresh->entries) { if (!e || skipUpdateHeader(e->id)) continue; @@ -275,21 +283,20 @@ HttpHeader::updateWarnings() bool HttpHeader::skipUpdateHeader(const Http::HdrType id) const { - // RFC 7234, section 4.3.4: use fields other from Warning for update - return id == Http::HdrType::WARNING; + return + // RFC 7234, section 4.3.4: use header fields other than Warning + (id == Http::HdrType::WARNING) || + // TODO: Consider updating Vary headers after comparing the magnitude of + // the required changes (and/or cache losses) with compliance gains. + (id == Http::HdrType::VARY); } -bool +void HttpHeader::update(HttpHeader const *fresh) { assert(fresh); assert(this != fresh); - // Optimization: Finding whether a header field changed is expensive - // and probably not worth it except for collapsed revalidation needs. - if (Config.onoff.collapsed_forwarding && !needUpdate(fresh)) - return false; - updateWarnings(); const HttpHeaderEntry *e; @@ -318,7 +325,6 @@ HttpHeader::update(HttpHeader const *fresh) addEntry(e->clone()); } - return true; } bool diff --git a/src/HttpHeader.h b/src/HttpHeader.h index e5c9930a51..8697b649e5 100644 --- a/src/HttpHeader.h +++ b/src/HttpHeader.h @@ -82,7 +82,12 @@ public: /* Interface functions */ void clean(); void append(const HttpHeader * src); - bool update(HttpHeader const *fresh); + /// replaces fields with matching names and adds fresh fields with new names + /// also updates Http::HdrType::WARNINGs, assuming `fresh` is a 304 reply + /// TODO: Refactor most callers to avoid special handling of WARNINGs. + void update(const HttpHeader *fresh); + /// \returns whether calling update(fresh) would change our set of fields + bool needUpdate(const HttpHeader *fresh) const; void compact(); int parse(const char *header_start, size_t len, Http::ContentLengthInterpreter &interpreter); /// Parses headers stored in a buffer. @@ -172,7 +177,6 @@ protected: /// *blk_end points to the first header delimiter character (CR or LF in CR?LF). /// If block starts where it ends, then there are no fields in the header. static bool Isolate(const char **parse_start, size_t l, const char **blk_start, const char **blk_end); - bool needUpdate(const HttpHeader *fresh) const; bool skipUpdateHeader(const Http::HdrType id) const; void updateWarnings(); diff --git a/src/HttpReply.cc b/src/HttpReply.cc index 0cc0c190f8..c97afe196f 100644 --- a/src/HttpReply.cc +++ b/src/HttpReply.cc @@ -124,12 +124,12 @@ HttpReply::MakeConnectionEstablished() { return rep; } -HttpReply * +HttpReplyPointer HttpReply::make304() const { static const Http::HdrType ImsEntries[] = {Http::HdrType::DATE, Http::HdrType::CONTENT_TYPE, Http::HdrType::EXPIRES, Http::HdrType::LAST_MODIFIED, /* eof */ Http::HdrType::OTHER}; - HttpReply *rv = new HttpReply; + const HttpReplyPointer rv(new HttpReply); int t; HttpHeaderEntry *e; @@ -159,9 +159,8 @@ HttpReply::packed304Reply() const /* Not as efficient as skipping the header duplication, * but easier to maintain */ - HttpReply *temp = make304(); + const auto temp = make304(); MemBuf *rv = temp->pack(); - delete temp; return rv; } @@ -260,23 +259,20 @@ HttpReply::validatorsMatch(HttpReply const * otherRep) const return 1; } -bool -HttpReply::updateOnNotModified(HttpReply const * freshRep) +HttpReply::Pointer +HttpReply::recreateOnNotModified(const HttpReply &reply304) const { - assert(freshRep); - - /* update raw headers */ - if (!header.update(&freshRep->header)) - return false; + // If enough 304s do not update, then this expensive checking is cheaper + // than blindly storing reply prefix identical to the already stored one. + if (!header.needUpdate(&reply304.header)) + return nullptr; - /* clean cache */ - hdrCacheClean(); - - header.compact(); - /* init cache */ - hdrCacheInit(); - - return true; + const Pointer cloned = clone(); + cloned->header.update(&reply304.header); + cloned->hdrCacheClean(); + cloned->header.compact(); + cloned->hdrCacheInit(); + return cloned; } /* internal routines */ diff --git a/src/HttpReply.h b/src/HttpReply.h index a0bf2854b6..ace7e7ebe1 100644 --- a/src/HttpReply.h +++ b/src/HttpReply.h @@ -72,7 +72,9 @@ public: virtual bool inheritProperties(const Http::Message *); - bool updateOnNotModified(HttpReply const *other); + /// \returns nil (if no updates are necessary) + /// \returns a new reply combining this reply with 304 updates (otherwise) + Pointer recreateOnNotModified(const HttpReply &reply304) const; /** set commonly used info with one call */ void setHeaders(Http::StatusCode status, @@ -85,7 +87,7 @@ public: static HttpReplyPointer MakeConnectionEstablished(); /** construct a 304 reply and return it */ - HttpReply *make304() const; + HttpReplyPointer make304() const; void redirect(Http::StatusCode, const char *); diff --git a/src/MemObject.cc b/src/MemObject.cc index 37bc26bd13..28b13a0280 100644 --- a/src/MemObject.cc +++ b/src/MemObject.cc @@ -132,6 +132,21 @@ MemObject::~MemObject() ctx_exit(ctx); /* must exit before we free mem->url */ } +HttpReply & +MemObject::adjustableBaseReply() +{ + assert(!updatedReply_); + return *reply_; +} + +void +MemObject::replaceBaseReply(const HttpReplyPointer &r) +{ + assert(r); + reply_ = r; + updatedReply_ = nullptr; +} + void MemObject::write(const StoreIOBuffer &writeBuffer) { @@ -161,6 +176,8 @@ MemObject::dump() const debugs(20, DBG_IMPORTANT, "MemObject->inmem_lo: " << inmem_lo); debugs(20, DBG_IMPORTANT, "MemObject->nclients: " << nclients); debugs(20, DBG_IMPORTANT, "MemObject->reply: " << reply_); + debugs(20, DBG_IMPORTANT, "MemObject->updatedReply: " << updatedReply_); + debugs(20, DBG_IMPORTANT, "MemObject->appliedUpdates: " << appliedUpdates); debugs(20, DBG_IMPORTANT, "MemObject->request: " << request); debugs(20, DBG_IMPORTANT, "MemObject->logUri: " << logUri_); debugs(20, DBG_IMPORTANT, "MemObject->storeId: " << storeId_); @@ -241,18 +258,27 @@ MemObject::size() const int64_t MemObject::expectedReplySize() const { - debugs(20, 7, "object_sz: " << object_sz); - if (object_sz >= 0) // complete() has been called; we know the exact answer + if (object_sz >= 0) { + debugs(20, 7, object_sz << " frozen by complete()"); return object_sz; + } + + const auto hdr_sz = baseReply().hdr_sz; - if (reply_) { - const int64_t clen = reply_->bodySize(method); - debugs(20, 7, "clen: " << clen); - if (clen >= 0 && reply_->hdr_sz > 0) // yuck: Http::Message sets hdr_sz to 0 - return clen + reply_->hdr_sz; + // Cannot predict future length using an empty/unset or HTTP/0 reply. + // For any HTTP/1 reply, hdr_sz is positive -- status-line cannot be empty. + if (hdr_sz <= 0) + return -1; + + const auto clen = baseReply().bodySize(method); + if (clen < 0) { + debugs(20, 7, "unknown; hdr: " << hdr_sz); + return -1; } - return -1; // not enough information to predict + const auto messageSize = clen + hdr_sz; + debugs(20, 7, messageSize << " hdr: " << hdr_sz << " clen: " << clen); + return messageSize; } void @@ -262,8 +288,10 @@ MemObject::reset() data_hdr.freeContent(); inmem_lo = 0; /* Should we check for clients? */ - if (reply_) - reply_->reset(); + assert(reply_); + reply_->reset(); + updatedReply_ = nullptr; + appliedUpdates = false; } int64_t @@ -280,11 +308,12 @@ MemObject::lowestMemReaderOffset() const bool MemObject::readAheadPolicyCanRead() const { - const bool canRead = endOffset() - getReply()->hdr_sz < + const auto savedHttpHeaders = baseReply().hdr_sz; + const bool canRead = endOffset() - savedHttpHeaders < lowestMemReaderOffset() + Config.readAheadGap; if (!canRead) { - debugs(19, 9, "no: " << endOffset() << '-' << getReply()->hdr_sz << + debugs(19, 5, "no: " << endOffset() << '-' << savedHttpHeaders << " < " << lowestMemReaderOffset() << '+' << Config.readAheadGap); } diff --git a/src/MemObject.h b/src/MemObject.h index 3aca9b6ebd..72338dbf93 100644 --- a/src/MemObject.h +++ b/src/MemObject.h @@ -54,11 +54,47 @@ public: void write(const StoreIOBuffer &buf); void unlinkRequest() { request = nullptr; } - const HttpReplyPointer &getReply() const { return reply_; } - void replaceReply(const HttpReplyPointer &r) { reply_ = r; } + + /// HTTP response before 304 (Not Modified) updates + /// starts "empty"; modified via replaceBaseReply() or adjustableBaseReply() + const HttpReply &baseReply() const { return *reply_; } + + /// \returns nil -- if no 304 updates since replaceBaseReply() + /// \returns combination of baseReply() and 304 updates -- after updates + const HttpReplyPointer &updatedReply() const { return updatedReply_; } + + /// \returns the updated-by-304(s) response (if it exists) + /// \returns baseReply() (otherwise) + const HttpReply &freshestReply() const { + if (updatedReply_) + return *updatedReply_; + else + return baseReply(); + } + + /// \returns writable base reply for parsing and other initial modifications + /// Base modifications can only be done when forming/loading the entry. + /// After that, use replaceBaseReply() to reset all of the replies. + HttpReply &adjustableBaseReply(); + + /// (re)sets base reply, usually just replacing the initial/empty object + /// also forgets the updated reply (if any) + void replaceBaseReply(const HttpReplyPointer &r); + + /// (re)sets updated reply; \see updatedReply() + void updateReply(const HttpReply &r) { updatedReply_ = &r; } + + /// reflects past Controller::updateOnNotModified(old, e304) calls: + /// for HTTP 304 entries: whether our entry was used as "e304" + /// for other entries: whether our entry was updated as "old" + bool appliedUpdates = false; + void stat (MemBuf * mb) const; int64_t endOffset () const; - void markEndOfReplyHeaders(); ///< sets reply_->hdr_sz to endOffset() + + /// sets baseReply().hdr_sz (i.e. written reply headers size) to endOffset() + void markEndOfReplyHeaders(); + /// negative if unknown; otherwise, expected object_sz, expected endOffset /// maximum, and stored reply headers+body size (all three are the same) int64_t expectedReplySize() const; @@ -150,8 +186,6 @@ public: }; MemCache memCache; ///< current [shared] memory caching state for the entry - /* Read only - this reply must be preserved by store clients */ - /* The original reply. possibly with updated metadata. */ HttpRequestPointer request; struct timeval start_ping; @@ -177,7 +211,8 @@ public: void kickReads(); private: - HttpReplyPointer reply_; + HttpReplyPointer reply_; ///< \see baseReply() + HttpReplyPointer updatedReply_; ///< \see updatedReply() mutable String storeId_; ///< StoreId for our entry (usually request URI) mutable String logUri_; ///< URI used for logging (usually request URI) diff --git a/src/MemStore.cc b/src/MemStore.cc index ecc7672f45..93a9807dba 100644 --- a/src/MemStore.cc +++ b/src/MemStore.cc @@ -360,10 +360,7 @@ MemStore::updateHeadersOrThrow(Ipc::StoreMapUpdate &update) // our +/- hdr_sz math below does not work if the chains differ [in size] Must(update.stale.anchor->basics.swap_file_sz == update.fresh.anchor->basics.swap_file_sz); - const HttpReply *rawReply = update.entry->getReply(); - Must(rawReply); - const HttpReply &reply = *rawReply; - const uint64_t staleHdrSz = reply.hdr_sz; + const uint64_t staleHdrSz = update.entry->mem().baseReply().hdr_sz; debugs(20, 7, "stale hdr_sz: " << staleHdrSz); /* we will need to copy same-slice payload after the stored headers later */ @@ -374,7 +371,7 @@ MemStore::updateHeadersOrThrow(Ipc::StoreMapUpdate &update) Must(update.stale.anchor); ShmWriter writer(*this, update.entry, update.fresh.fileNo); - reply.packHeadersUsingSlowPacker(writer); + update.entry->mem().freshestReply().packHeadersUsingSlowPacker(writer); const uint64_t freshHdrSz = writer.totalWritten; debugs(20, 7, "fresh hdr_sz: " << freshHdrSz << " diff: " << (freshHdrSz - staleHdrSz)); @@ -546,7 +543,7 @@ MemStore::copyFromShmSlice(StoreEntry &e, const StoreIOBuffer &buf, bool eof) // from store_client::readBody() // parse headers if needed; they might span multiple slices! - HttpReply *rep = (HttpReply *)e.getReply(); + const auto rep = &e.mem().adjustableBaseReply(); if (rep->pstate < Http::Message::psParsed) { // XXX: have to copy because httpMsgParseStep() requires 0-termination MemBuf mb; diff --git a/src/Store.h b/src/Store.h index be67b92554..1596f2773f 100644 --- a/src/Store.h +++ b/src/Store.h @@ -49,7 +49,14 @@ public: StoreEntry(); virtual ~StoreEntry(); - HttpReply const *getReply() const; + MemObject &mem() { assert(mem_obj); return *mem_obj; } + const MemObject &mem() const { assert(mem_obj); return *mem_obj; } + + /// \retval * the address of freshest reply (if mem_obj exists) + /// \retval nullptr when mem_obj does not exist + /// \see MemObject::freshestReply() + const HttpReply *hasFreshestReply() const { return mem_obj ? &mem_obj->freshestReply() : nullptr; } + void write(StoreIOBuffer); /** Check if the Store entry is empty @@ -57,20 +64,18 @@ public: * \retval false Store contains 1 or more bytes of data. * \retval false Store contains negative content !!!!!! */ - bool isEmpty() const { - assert (mem_obj); - return mem_obj->endOffset() == 0; - } + bool isEmpty() const { return mem().endOffset() == 0; } bool isAccepting() const; size_t bytesWanted(Range const aRange, bool ignoreDelayPool = false) const; /// flags [truncated or too big] entry with ENTRY_BAD_LENGTH and releases it void lengthWentBad(const char *reason); void complete(); store_client_t storeClientType() const; - char const *getSerialisedMetaData(); + /// \returns a malloc()ed buffer containing a length-long packed swap header + const char *getSerialisedMetaData(size_t &length) const; /// Store a prepared error response. MemObject locks the reply object. void storeErrorResponse(HttpReply *reply); - void replaceHttpReply(HttpReply *, bool andStartWriting = true); + void replaceHttpReply(const HttpReplyPointer &, const bool andStartWriting = true); void startWriting(); ///< pack and write reply headers and, maybe, body /// whether we may start writing to disk (now or in the future) bool mayStartSwapOut(); @@ -173,6 +178,11 @@ public: /// whether this entry has an ETag; if yes, puts ETag value into parameter bool hasEtag(ETag &etag) const; + /// Updates easily-accessible non-Store-specific parts of the entry. + /// Use Controller::updateOnNotModified() instead of this helper. + /// \returns whether anything was actually updated + bool updateOnNotModified(const StoreEntry &e304); + /// the disk this entry is [being] cached on; asserts for entries w/o a disk Store::Disk &disk() const; /// whether one of this StoreEntry owners has locked the corresponding @@ -235,8 +245,8 @@ public: ESIElement::Pointer cachedESITree; #endif - int64_t objectLen() const; - int64_t contentLen() const; + int64_t objectLen() const { return mem().object_sz; } + int64_t contentLen() const { return objectLen() - mem().baseReply().hdr_sz; } /// claim shared ownership of this entry (for use in a given context) /// matching lock() and unlock() contexts eases leak triage but is optional diff --git a/src/StoreMeta.h b/src/StoreMeta.h index ebdc2385af..dde5b38503 100644 --- a/src/StoreMeta.h +++ b/src/StoreMeta.h @@ -137,7 +137,7 @@ public: /// \ingroup SwapStoreAPI char *storeSwapMetaPack(tlv * tlv_list, int *length); /// \ingroup SwapStoreAPI -tlv *storeSwapMetaBuild(StoreEntry * e); +tlv *storeSwapMetaBuild(const StoreEntry *); /// \ingroup SwapStoreAPI void storeSwapTLVFree(tlv * n); diff --git a/src/StoreMetaUnpacker.h b/src/StoreMetaUnpacker.h index 8dd2638f33..ab6b7d67e7 100644 --- a/src/StoreMetaUnpacker.h +++ b/src/StoreMetaUnpacker.h @@ -41,14 +41,5 @@ private: StoreMeta **tail; }; -/* - * store_swapmeta.c - */ -char *storeSwapMetaPack(StoreMeta * tlv_list, int *length); -StoreMeta *storeSwapMetaBuild(StoreEntry * e); -StoreMeta *storeSwapMetaUnpack(const char *buf, int *hdrlen); -void storeSwapTLVFree(StoreMeta * n); -StoreMeta ** storeSwapTLVAdd(int type, const void *ptr, size_t len, StoreMeta ** tail); - #endif /* SQUID_TYPELENGTHVALUEUNPACKER_H */ diff --git a/src/acl/Asn.cc b/src/acl/Asn.cc index 6a2af66277..a063b17b80 100644 --- a/src/acl/Asn.cc +++ b/src/acl/Asn.cc @@ -290,7 +290,7 @@ asHandleReply(void *data, StoreIOBuffer result) debugs(53, DBG_IMPORTANT, "asHandleReply: Called with Error set and size=" << (unsigned int) result.length); delete asState; return; - } else if (e->getReply()->sline.status() != Http::scOkay) { + } else if (e->mem().baseReply().sline.status() != Http::scOkay) { debugs(53, DBG_IMPORTANT, "WARNING: AS " << asState->as_number << " whois request failed"); delete asState; return; diff --git a/src/client_side.cc b/src/client_side.cc index 61f1c2f322..db1d0c93f2 100644 --- a/src/client_side.cc +++ b/src/client_side.cc @@ -383,12 +383,16 @@ ClientHttpRequest::logRequest() al->url = log_uri; debugs(33, 9, "clientLogRequest: al.url='" << al->url << "'"); - if (al->reply) { - al->http.code = al->reply->sline.status(); - al->http.content_type = al->reply->content_type.termedBuf(); - } else if (loggingEntry() && loggingEntry()->mem_obj) { - al->http.code = loggingEntry()->mem_obj->getReply()->sline.status(); - al->http.content_type = loggingEntry()->mem_obj->getReply()->content_type.termedBuf(); + const auto findReply = [this]() -> const HttpReply * { + if (al->reply) + return al->reply.getRaw(); + if (const auto le = loggingEntry()) + return le->hasFreshestReply(); + return nullptr; + }; + if (const auto reply = findReply()) { + al->http.code = reply->sline.status(); + al->http.content_type = reply->content_type.termedBuf(); } debugs(33, 9, "clientLogRequest: http.code='" << al->http.code << "'"); @@ -741,7 +745,7 @@ ClientHttpRequest::mRangeCLen() while (pos != request->range->end()) { /* account for headers for this range */ mb.reset(); - clientPackRangeHdr(memObject()->getReply(), + clientPackRangeHdr(&storeEntry()->mem().freshestReply(), *pos, range_iter.boundary, &mb); clen += mb.size; @@ -3515,11 +3519,12 @@ int varyEvaluateMatch(StoreEntry * entry, HttpRequest * request) { SBuf vary(request->vary_headers); - int has_vary = entry->getReply()->header.has(Http::HdrType::VARY); + const auto &reply = entry->mem().freshestReply(); + auto has_vary = reply.header.has(Http::HdrType::VARY); #if X_ACCELERATOR_VARY has_vary |= - entry->getReply()->header.has(Http::HdrType::HDR_X_ACCELERATOR_VARY); + reply.header.has(Http::HdrType::HDR_X_ACCELERATOR_VARY); #endif if (!has_vary || entry->mem_obj->vary_headers.isEmpty()) { @@ -3539,7 +3544,7 @@ varyEvaluateMatch(StoreEntry * entry, HttpRequest * request) /* virtual "vary" object found. Calculate the vary key and * continue the search */ - vary = httpMakeVaryMark(request, entry->getReply()); + vary = httpMakeVaryMark(request, &reply); if (!vary.isEmpty()) { request->vary_headers = vary; @@ -3551,7 +3556,7 @@ varyEvaluateMatch(StoreEntry * entry, HttpRequest * request) } } else { if (vary.isEmpty()) { - vary = httpMakeVaryMark(request, entry->getReply()); + vary = httpMakeVaryMark(request, &reply); if (!vary.isEmpty()) request->vary_headers = vary; diff --git a/src/client_side_reply.cc b/src/client_side_reply.cc index 24afc1cc92..3682389852 100644 --- a/src/client_side_reply.cc +++ b/src/client_side_reply.cc @@ -442,8 +442,6 @@ clientReplyContext::handleIMSReply(StoreIOBuffer result) /* update size of the request */ reqsize = result.length + reqofs; - const Http::StatusCode status = http->storeEntry()->getReply()->sline.status(); - // request to origin was aborted if (EBIT_TEST(http->storeEntry()->flags, ENTRY_ABORTED)) { debugs(88, 3, "request to origin aborted '" << http->storeEntry()->url() << "', sending old entry to client"); @@ -451,14 +449,17 @@ clientReplyContext::handleIMSReply(StoreIOBuffer result) sendClientOldEntry(); } - const HttpReply *old_rep = old_entry->getReply(); + const auto oldStatus = old_entry->mem().freshestReply().sline.status(); + const auto &new_rep = http->storeEntry()->mem().freshestReply(); + const auto status = new_rep.sline.status(); // origin replied 304 if (status == Http::scNotModified) { http->logType.update(LOG_TCP_REFRESH_UNMODIFIED); http->request->flags.staleIfHit = false; // old_entry is no longer stale - // update headers on existing entry + // TODO: The update may not be instantaneous. Should we wait for its + // completion to avoid spawning too much client-disassociated work? Store::Root().updateOnNotModified(old_entry, *http->storeEntry()); // if client sent IMS @@ -470,21 +471,20 @@ clientReplyContext::handleIMSReply(StoreIOBuffer result) } else { // send existing entry, it's still valid debugs(88, 3, "origin replied 304, revalidating existing entry and sending " << - old_rep->sline.status() << " to client"); + oldStatus << " to client"); sendClientOldEntry(); } } // origin replied with a non-error code else if (status > Http::scNone && status < Http::scInternalServerError) { - const HttpReply *new_rep = http->storeEntry()->getReply(); // RFC 7234 section 4: a cache MUST use the most recent response // (as determined by the Date header field) - if (new_rep->olderThan(old_rep)) { + if (new_rep.olderThan(&old_entry->mem().freshestReply())) { http->logType.err.ignored = true; debugs(88, 3, "origin replied " << status << " but with an older date header, sending old entry (" << - old_rep->sline.status() << ") to client"); + oldStatus << ") to client"); sendClientOldEntry(); } else { http->logType.update(LOG_TCP_REFRESH_MODIFIED); @@ -508,7 +508,7 @@ clientReplyContext::handleIMSReply(StoreIOBuffer result) // ignore and let client have old entry http->logType.update(LOG_TCP_REFRESH_FAIL_OLD); debugs(88, 3, "origin replied with error " << - status << ", sending old entry (" << old_rep->sline.status() << ") to client"); + status << ", sending old entry (" << oldStatus << ") to client"); sendClientOldEntry(); } } @@ -778,7 +778,7 @@ clientReplyContext::processMiss() triggerInitialStoreRead(); if (http->redirect.status) { - HttpReply *rep = new HttpReply; + const HttpReplyPointer rep(new HttpReply); http->logType.update(LOG_TCP_REDIRECT); http->storeEntry()->releaseRequest(); rep->redirect(http->redirect.status, http->redirect.location); @@ -820,8 +820,9 @@ clientReplyContext::processConditional(StoreIOBuffer &result) { StoreEntry *const e = http->storeEntry(); - if (e->getReply()->sline.status() != Http::scOkay) { - debugs(88, 4, "Reply code " << e->getReply()->sline.status() << " != 200"); + const auto replyStatusCode = e->mem().baseReply().sline.status(); + if (replyStatusCode != Http::scOkay) { + debugs(88, 4, "miss because " << replyStatusCode << " != 200"); http->logType.update(LOG_TCP_MISS); processMiss(); return true; @@ -877,18 +878,13 @@ clientReplyContext::blockedHit() const if (http->flags.internal) return false; // internal content "hits" cannot be blocked - if (const HttpReply *rep = http->storeEntry()->getReply()) { + const auto &rep = http->storeEntry()->mem().freshestReply(); + { std::unique_ptr chl(clientAclChecklistCreate(Config.accessList.sendHit, http)); - chl->reply = const_cast(rep); // ACLChecklist API bug + chl->reply = const_cast(&rep); // ACLChecklist API bug HTTPMSGLOCK(chl->reply); return !chl->fastCheck().allowed(); // when in doubt, block } - - // This does not happen, I hope, because we are called from CacheHit, which - // is called via a storeClientCopy() callback, and store should initialize - // the reply before calling that callback. - debugs(88, 3, "Missing reply!"); - return false; } void @@ -1118,7 +1114,7 @@ clientReplyContext::purgeDoPurgeHead(StoreEntry *newEntry) triggerInitialStoreRead(); - HttpReply *rep = new HttpReply; + const HttpReplyPointer rep(new HttpReply); rep->setHeaders(purgeStatus, NULL, NULL, 0, 0, -1); http->storeEntry()->replaceHttpReply(rep); http->storeEntry()->complete(); @@ -1137,7 +1133,7 @@ clientReplyContext::traceReply(clientStreamNode * node) localTempBuffer, SendMoreData, this); http->storeEntry()->releaseRequest(); http->storeEntry()->buffer(); - HttpReply *rep = new HttpReply; + const HttpReplyPointer rep(new HttpReply); rep->setHeaders(Http::scOkay, NULL, "text/plain", http->request->prefixLen(), 0, squid_curtime); http->storeEntry()->replaceHttpReply(rep); http->request->swapOut(http->storeEntry()); @@ -1210,17 +1206,24 @@ clientReplyContext::storeNotOKTransferDone() const /* haven't found end of headers yet */ return 0; - const HttpReplyPointer curReply(mem->getReply()); + // TODO: Use MemObject::expectedReplySize(method) after resolving XXX below. + const auto expectedBodySize = mem->baseReply().content_length; + + // XXX: The code below talks about sending data, and checks stats about + // bytes written to the client connection, but this method must determine + // whether we are done _receiving_ data from Store. This code should work OK + // when expectedBodySize is unknown or matches written data, but it may + // malfunction when we are writing ranges while receiving a full response. /* * Figure out how much data we are supposed to send. * If we are sending a body and we don't have a content-length, * then we must wait for the object to become STORE_OK. */ - if (curReply->content_length < 0) + if (expectedBodySize < 0) return 0; - uint64_t expectedLength = curReply->content_length + http->out.headers_sz; + const uint64_t expectedLength = expectedBodySize + http->out.headers_sz; if (http->out.size < expectedLength) return 0; @@ -1316,8 +1319,9 @@ clientReplyContext::replyStatus() return STREAM_FAILED; } + // TODO: See also (and unify with) storeNotOKTransferDone() checks. const int64_t expectedBodySize = - http->storeEntry()->getReply()->bodySize(http->request->method); + http->storeEntry()->mem().baseReply().bodySize(http->request->method); if (expectedBodySize >= 0 && !http->gotEnough()) { debugs(88, 5, "clientReplyStatus: client didn't get all it expected"); return STREAM_UNPLANNED_COMPLETE; @@ -1646,7 +1650,7 @@ clientReplyContext::cloneReply() { assert(reply == NULL); - reply = http->storeEntry()->getReply()->clone(); + reply = http->storeEntry()->mem().freshestReply().clone(); HTTPMSGLOCK(reply); http->al->reply = reply; @@ -1995,7 +1999,7 @@ clientReplyContext::sendNotModified() { StoreEntry *e = http->storeEntry(); const time_t timestamp = e->timestamp; - HttpReply *const temprep = e->getReply()->make304(); + const auto temprep = e->mem().freshestReply().make304(); // log as TCP_INM_HIT if code 304 generated for // If-None-Match request if (!http->request->flags.ims) diff --git a/src/client_side_reply.h b/src/client_side_reply.h index edd151305e..3e551c1cc7 100644 --- a/src/client_side_reply.h +++ b/src/client_side_reply.h @@ -80,6 +80,9 @@ public: virtual LogTags *loggingTags(); ClientHttpRequest *http; + /// Base reply header bytes received from Store. + /// Compatible with ClientHttpRequest::Out::offset. + /// Not to be confused with ClientHttpRequest::Out::headers_sz. int headers_sz; store_client *sc; /* The store_client we're using */ StoreIOBuffer tempBuffer; /* For use in validating requests via IMS */ diff --git a/src/client_side_request.cc b/src/client_side_request.cc index 21772b8c57..9781bec213 100644 --- a/src/client_side_request.cc +++ b/src/client_side_request.cc @@ -1628,9 +1628,9 @@ ClientHttpRequest::sslBumpStart() bool ClientHttpRequest::gotEnough() const { - /** TODO: should be querying the stream. */ + // TODO: See also (and unify with) clientReplyContext::storeNotOKTransferDone() int64_t contentLength = - memObject()->getReply()->bodySize(request->method); + memObject()->baseReply().bodySize(request->method); assert(contentLength >= 0); if (out.offset < contentLength) diff --git a/src/client_side_request.h b/src/client_side_request.h index 9f6652b8c6..de715d032f 100644 --- a/src/client_side_request.h +++ b/src/client_side_request.h @@ -109,8 +109,16 @@ public: struct Out { Out() : offset(0), size(0), headers_sz(0) {} + /// Roughly speaking, this offset points to the next body byte we want + /// to receive from Store. Without Ranges (and I/O errors), we should + /// have received (and written to the client) all the previous bytes. + /// XXX: The offset is updated by various receive-write steps, making + /// its exact meaning illusive. Its Out class placement is confusing. int64_t offset; + /// Response header and body bytes written to the client connection. uint64_t size; + /// Response header bytes written to the client connection. + /// Not to be confused with clientReplyContext::headers_sz. size_t headers_sz; } out; diff --git a/src/clients/Client.cc b/src/clients/Client.cc index e6db9a83de..220ec275ad 100644 --- a/src/clients/Client.cc +++ b/src/clients/Client.cc @@ -525,7 +525,7 @@ Client::blockCaching() // This relatively expensive check is not in StoreEntry::checkCachable: // That method lacks HttpRequest and may be called too many times. ACLFilledChecklist ch(acl, originalRequest().getRaw()); - ch.reply = const_cast(entry->getReply()); // ACLFilledChecklist API bug + ch.reply = const_cast(&entry->mem().freshestReply()); // ACLFilledChecklist API bug HTTPMSGLOCK(ch.reply); ch.al = fwd->al; if (!ch.fastCheck().allowed()) { // when in doubt, block diff --git a/src/fs/rock/RockHeaderUpdater.cc b/src/fs/rock/RockHeaderUpdater.cc index df523961cb..3edbf46fb8 100644 --- a/src/fs/rock/RockHeaderUpdater.cc +++ b/src/fs/rock/RockHeaderUpdater.cc @@ -176,22 +176,45 @@ Rock::HeaderUpdater::startWriting() IoState &rockWriter = dynamic_cast(*writer); rockWriter.staleSplicingPointNext = staleSplicingPointNext; + // here, prefix is swap header plus HTTP reply header (i.e., updated bytes) + uint64_t stalePrefixSz = 0; + uint64_t freshPrefixSz = 0; + off_t offset = 0; // current writing offset (for debugging) + const auto &mem = update.entry->mem(); + { debugs(20, 7, "fresh store meta for " << *update.entry); - const char *freshSwapHeader = update.entry->getSerialisedMetaData(); - const auto freshSwapHeaderSize = update.entry->mem_obj->swap_hdr_sz; + size_t freshSwapHeaderSize = 0; // set by getSerialisedMetaData() below + + // There is a circular dependency between the correct/fresh value of + // entry->swap_file_sz and freshSwapHeaderSize. We break that loop by + // serializing zero swap_file_sz, just like the regular first-time + // swapout code may do. De-serializing code will re-calculate it in + // storeRebuildParseEntry(). TODO: Stop serializing entry->swap_file_sz. + const auto savedEntrySwapFileSize = update.entry->swap_file_sz; + update.entry->swap_file_sz = 0; + const auto freshSwapHeader = update.entry->getSerialisedMetaData(freshSwapHeaderSize); + update.entry->swap_file_sz = savedEntrySwapFileSize; + Must(freshSwapHeader); writer->write(freshSwapHeader, freshSwapHeaderSize, 0, nullptr); + stalePrefixSz += mem.swap_hdr_sz; + freshPrefixSz += freshSwapHeaderSize; offset += freshSwapHeaderSize; xfree(freshSwapHeader); } { debugs(20, 7, "fresh HTTP header @ " << offset); - MemBuf *httpHeader = update.entry->mem_obj->getReply()->pack(); + const auto httpHeader = mem.freshestReply().pack(); writer->write(httpHeader->content(), httpHeader->contentSize(), -1, nullptr); + const auto &staleReply = mem.baseReply(); + Must(staleReply.hdr_sz >= 0); // for int-to-uint64_t conversion below + Must(staleReply.hdr_sz > 0); // already initialized + stalePrefixSz += staleReply.hdr_sz; + freshPrefixSz += httpHeader->contentSize(); offset += httpHeader->contentSize(); delete httpHeader; } @@ -203,7 +226,14 @@ Rock::HeaderUpdater::startWriting() exchangeBuffer.clear(); } - debugs(20, 7, "wrote " << offset); + debugs(20, 7, "wrote " << offset << + "; swap_file_sz delta: -" << stalePrefixSz << " +" << freshPrefixSz); + + // Optimistic early update OK: Our write lock blocks access to swap_file_sz. + auto &swap_file_sz = update.fresh.anchor->basics.swap_file_sz; + Must(swap_file_sz >= stalePrefixSz); + swap_file_sz -= stalePrefixSz; + swap_file_sz += freshPrefixSz; writer->close(StoreIOState::wroteAll); // should call noteDoneWriting() } diff --git a/src/http.cc b/src/http.cc index 1bfab0c9b5..742cf65d43 100644 --- a/src/http.cc +++ b/src/http.cc @@ -861,12 +861,12 @@ HttpStateData::peerSupportsConnectionPinning() const if (!_peer->connection_auth) return false; - const HttpReplyPointer rep(entry->mem_obj->getReply()); + const auto &rep = entry->mem().freshestReply(); /*The peer supports connection pinning and the http reply status is not unauthorized, so the related connection can be pinned */ - if (rep->sline.status() != Http::scUnauthorized) + if (rep.sline.status() != Http::scUnauthorized) return true; /*The server respond with Http::scUnauthorized and the peer configured @@ -895,7 +895,7 @@ HttpStateData::peerSupportsConnectionPinning() const reply and has in its list the "Session-Based-Authentication" which means that the peer supports connection pinning. */ - if (rep->header.hasListMember(Http::HdrType::PROXY_SUPPORT, "Session-Based-Authentication", ',')) + if (rep.header.hasListMember(Http::HdrType::PROXY_SUPPORT, "Session-Based-Authentication", ',')) return true; return false; @@ -919,7 +919,7 @@ HttpStateData::haveParsedReplyHeaders() if (StoreEntry *oldEntry = findPreviouslyCachedEntry(entry)) { oldEntry->lock("HttpStateData::haveParsedReplyHeaders"); - sawDateGoBack = rep->olderThan(oldEntry->getReply()); + sawDateGoBack = rep->olderThan(oldEntry->hasFreshestReply()); oldEntry->unlock("HttpStateData::haveParsedReplyHeaders"); } diff --git a/src/http/Stream.cc b/src/http/Stream.cc index 98c96a8aa4..6ce65d8b10 100644 --- a/src/http/Stream.cc +++ b/src/http/Stream.cc @@ -441,7 +441,7 @@ Http::Stream::buildRangeHeader(HttpReply *rep) } else if (rep->content_length < 0) range_err = "unknown length"; - else if (rep->content_length != http->memObject()->getReply()->content_length) + else if (rep->content_length != http->storeEntry()->mem().baseReply().content_length) range_err = "INCONSISTENT length"; /* a bug? */ /* hits only - upstream CachePeer determines correct behaviour on misses, @@ -649,9 +649,8 @@ Http::Stream::packRange(StoreIOBuffer const &source, MemBuf *mb) * multi-range */ if (http->multipartRangeRequest() && i->debt() == i->currentSpec()->length) { - assert(http->memObject()); clientPackRangeHdr( - http->memObject()->getReply(), /* original reply */ + &http->storeEntry()->mem().freshestReply(), i->currentSpec(), /* current range */ i->boundary, /* boundary, the same for all */ mb); diff --git a/src/icmp/net_db.cc b/src/icmp/net_db.cc index 9696aa3ac5..4e931b5ca3 100644 --- a/src/icmp/net_db.cc +++ b/src/icmp/net_db.cc @@ -696,7 +696,6 @@ netdbExchangeHandleReply(void *data, StoreIOBuffer receivedData) double hops; char *p; int j; - HttpReply const *rep; size_t hdr_sz; int nused = 0; int size; @@ -738,11 +737,11 @@ netdbExchangeHandleReply(void *data, StoreIOBuffer receivedData) if ((hdr_sz = headersEnd(p, ex->buf_ofs))) { debugs(38, 5, "netdbExchangeHandleReply: hdr_sz = " << hdr_sz); - rep = ex->e->getReply(); - assert(rep->sline.status() != Http::scNone); - debugs(38, 3, "netdbExchangeHandleReply: reply status " << rep->sline.status()); + const auto scode = ex->e->mem().baseReply().sline.status(); + assert(scode != Http::scNone); + debugs(38, 3, "netdbExchangeHandleReply: reply status " << scode); - if (rep->sline.status() != Http::scOkay) { + if (scode != Http::scOkay) { delete ex; return; } diff --git a/src/ipc/StoreMap.cc b/src/ipc/StoreMap.cc index 312cfd066c..735ab06816 100644 --- a/src/ipc/StoreMap.cc +++ b/src/ipc/StoreMap.cc @@ -582,7 +582,10 @@ Ipc::StoreMap::closeForUpdating(Update &update) update.stale.anchor->splicingPoint = update.stale.splicingPoint; freeEntry(update.stale.fileNo); - // make the stale anchor/chain reusable, reachable via its new location + // Make the stale anchor/chain reusable, reachable via update.fresh.name. If + // update.entry->swap_filen is still update.stale.fileNo, and the entry is + // using store, then the entry must have a lock on update.stale.fileNo, + // preventing its premature reuse by others. relocate(update.fresh.name, update.stale.fileNo); const Update updateSaved = update; // for post-close debugging below diff --git a/src/ipc/StoreMap.h b/src/ipc/StoreMap.h index b6d0917a78..25e45a60b1 100644 --- a/src/ipc/StoreMap.h +++ b/src/ipc/StoreMap.h @@ -186,8 +186,8 @@ public: StoreMapUpdate &operator =(const StoreMapUpdate &other) = delete; StoreEntry *entry; ///< the store entry being updated - Edition stale; ///< old anchor and chain being updated - Edition fresh; ///< new anchor and updated chain prefix + Edition stale; ///< old anchor and chain + Edition fresh; ///< new anchor and the updated chain prefix }; class StoreMapCleaner; diff --git a/src/peer_digest.cc b/src/peer_digest.cc index 19cc6d313a..3d100d592b 100644 --- a/src/peer_digest.cc +++ b/src/peer_digest.cc @@ -520,13 +520,12 @@ peerDigestFetchReply(void *data, char *buf, ssize_t size) return -1; if ((hdr_size = headersEnd(buf, size))) { - HttpReply const *reply = fetch->entry->getReply(); - assert(reply); - assert(reply->sline.status() != Http::scNone); - const Http::StatusCode status = reply->sline.status(); + const auto &reply = fetch->entry->mem().freshestReply(); + const auto status = reply.sline.status(); + assert(status != Http::scNone); debugs(72, 3, "peerDigestFetchReply: " << pd->host << " status: " << status << - ", expires: " << (long int) reply->expires << " (" << std::showpos << - (int) (reply->expires - squid_curtime) << ")"); + ", expires: " << (long int) reply.expires << " (" << std::showpos << + (int) (reply.expires - squid_curtime) << ")"); /* this "if" is based on clientHandleIMSReply() */ @@ -565,7 +564,7 @@ peerDigestFetchReply(void *data, char *buf, ssize_t size) } } else { /* some kind of a bug */ - peerDigestFetchAbort(fetch, buf, reply->sline.reason()); + peerDigestFetchAbort(fetch, buf, reply.sline.reason()); return -1; /* XXX -1 will abort stuff in ReadReply! */ } @@ -604,13 +603,13 @@ peerDigestSwapInHeaders(void *data, char *buf, ssize_t size) assert(!fetch->offset); if ((hdr_size = headersEnd(buf, size))) { - assert(fetch->entry->getReply()); - assert(fetch->entry->getReply()->sline.status() != Http::scNone); + const auto &reply = fetch->entry->mem().freshestReply(); + const auto status = reply.sline.status(); + assert(status != Http::scNone); - if (fetch->entry->getReply()->sline.status() != Http::scOkay) { + if (status != Http::scOkay) { debugs(72, DBG_IMPORTANT, "peerDigestSwapInHeaders: " << fetch->pd->host << - " status " << fetch->entry->getReply()->sline.status() << - " got cached!"); + " status " << status << " got cached!"); peerDigestFetchAbort(fetch, buf, "internal status error"); return -1; @@ -642,7 +641,8 @@ peerDigestSwapInCBlock(void *data, char *buf, ssize_t size) if (size >= (ssize_t)StoreDigestCBlockSize) { PeerDigest *pd = fetch->pd; - assert(pd && fetch->entry->getReply()); + assert(pd); + assert(fetch->entry->mem_obj); if (peerDigestSetCBlock(pd, buf)) { /* XXX: soon we will have variable header size */ diff --git a/src/refresh.cc b/src/refresh.cc index 4ef7a0bf10..19da664c8a 100644 --- a/src/refresh.cc +++ b/src/refresh.cc @@ -326,7 +326,7 @@ refreshCheck(const StoreEntry * entry, HttpRequest * request, time_t delta) debugs(22, 3, "Staleness = " << staleness); - const HttpReplyPointer reply(entry->mem_obj && entry->mem_obj->getReply() ? entry->mem_obj->getReply() : nullptr); + const auto reply = entry->hasFreshestReply(); // may be nil // stale-if-error requires any failure be passed thru when its period is over. int staleIfError = -1; @@ -550,11 +550,7 @@ refreshIsCachable(const StoreEntry * entry) /* no mem_obj? */ return true; - if (entry->getReply() == NULL) - /* no reply? */ - return true; - - if (entry->getReply()->content_length == 0) + if (entry->mem_obj->baseReply().content_length == 0) /* No use refreshing (caching?) 0 byte objects */ return false; diff --git a/src/store.cc b/src/store.cc index fdb389a086..0a9bce864f 100644 --- a/src/store.cc +++ b/src/store.cc @@ -385,10 +385,10 @@ StoreEntry::destroyMemObject() if (hasMemStore() && !shutting_down) Store::Root().memoryDisconnect(*this); - if (MemObject *mem = mem_obj) { + if (auto memObj = mem_obj) { setMemStatus(NOT_IN_MEMORY); mem_obj = NULL; - delete mem; + delete memObj; } } @@ -694,6 +694,7 @@ StoreEntry::adjustVary() return nullptr; HttpRequestPointer request(mem_obj->request); + const auto &reply = mem_obj->freshestReply(); if (mem_obj->vary_headers.isEmpty()) { /* First handle the case where the object no longer varies */ @@ -710,7 +711,7 @@ StoreEntry::adjustVary() /* Make sure the request knows the variance status */ if (request->vary_headers.isEmpty()) - request->vary_headers = httpMakeVaryMark(request.getRaw(), mem_obj->getReply().getRaw()); + request->vary_headers = httpMakeVaryMark(request.getRaw(), &reply); } // TODO: storeGetPublic() calls below may create unlocked entries. @@ -727,9 +728,9 @@ StoreEntry::adjustVary() throw TexcHere("failed to make Vary marker public"); } /* We are allowed to do this typecast */ - HttpReply *rep = new HttpReply; + const HttpReplyPointer rep(new HttpReply); rep->setHeaders(Http::scOkay, "Internal marker object", "x-squid-internal/vary", -1, -1, squid_curtime + 100000); - String vary = mem_obj->getReply()->header.getList(Http::HdrType::VARY); + auto vary = reply.header.getList(Http::HdrType::VARY); if (vary.size()) { /* Again, we own this structure layout */ @@ -738,7 +739,7 @@ StoreEntry::adjustVary() } #if X_ACCELERATOR_VARY - vary = mem_obj->getReply()->header.getList(Http::HdrType::HDR_X_ACCELERATOR_VARY); + vary = reply.header.getList(Http::HdrType::HDR_X_ACCELERATOR_VARY); if (vary.size() > 0) { /* Again, we own this structure layout */ @@ -808,8 +809,7 @@ StoreEntry::write (StoreIOBuffer writeBuffer) assert(store_status == STORE_PENDING); // XXX: caller uses content offset, but we also store headers - if (const HttpReplyPointer reply = mem_obj->getReply()) - writeBuffer.offset += reply->hdr_sz; + writeBuffer.offset += mem_obj->baseReply().hdr_sz; debugs(20, 5, "storeWrite: writing " << writeBuffer.length << " bytes for '" << getMD5Text() << "'"); PROF_stop(StoreEntry_write); @@ -839,7 +839,7 @@ StoreEntry::append(char const *buf, int len) * XXX sigh, offset might be < 0 here, but it gets "corrected" * later. This offset crap is such a mess. */ - tempBuffer.offset = mem_obj->endOffset() - (getReply() ? getReply()->hdr_sz : 0); + tempBuffer.offset = mem_obj->endOffset() - mem_obj->baseReply().hdr_sz; write(tempBuffer); } @@ -935,9 +935,10 @@ StoreEntry::checkTooSmall() if (mem_obj->object_sz >= 0 && mem_obj->object_sz < Config.Store.minObjectSize) return 1; - if (getReply()->content_length > -1) - if (getReply()->content_length < Config.Store.minObjectSize) - return 1; + + const auto clen = mem().baseReply().content_length; + if (clen >= 0 && clen < Config.Store.minObjectSize) + return 1; return 0; } @@ -947,10 +948,8 @@ StoreEntry::checkTooBig() const if (mem_obj->endOffset() > store_maxobjsize) return true; - if (getReply()->content_length < 0) - return false; - - return (getReply()->content_length > store_maxobjsize); + const auto clen = mem_obj->baseReply().content_length; + return (clen >= 0 && clen > store_maxobjsize); } // TODO: move "too many open..." checks outside -- we are called too early/late @@ -984,7 +983,7 @@ StoreEntry::checkCachable() debugs(20, 3, "StoreEntry::checkCachable: NO: negative cached"); ++store_check_cachable_hist.no.negative_cached; return 0; /* avoid release call below */ - } else if (!mem_obj || !getReply()) { + } else if (!mem_obj) { // XXX: In bug 4131, we forgetHit() without mem_obj, so we need // this segfault protection, but how can we get such a HIT? debugs(20, 2, "StoreEntry::checkCachable: NO: missing parts: " << *this); @@ -1078,9 +1077,6 @@ StoreEntry::complete() return; } - /* This is suspect: mem obj offsets include the headers. do we adjust for that - * in use of object_sz? - */ mem_obj->object_sz = mem_obj->endOffset(); store_status = STORE_OK; @@ -1250,13 +1246,15 @@ storeLateRelease(void *) eventAdd("storeLateRelease", storeLateRelease, NULL, 0.0, 1); } +/// whether the base response has all the body bytes we expect +/// \returns true for responses with unknown/unspecified body length +/// \returns true for responses with the right number of accumulated body bytes bool StoreEntry::validLength() const { int64_t diff; - const HttpReply *reply; assert(mem_obj != NULL); - reply = getReply(); + const auto reply = &mem_obj->baseReply(); debugs(20, 3, "storeEntryValidLength: Checking '" << getMD5Text() << "'"); debugs(20, 5, "storeEntryValidLength: object_len = " << objectLen()); @@ -1441,7 +1439,11 @@ StoreEntry::validToSend() const bool StoreEntry::timestampsSet() { - const HttpReply *reply = getReply(); + debugs(20, 7, *this << " had " << describeTimestamps()); + + // TODO: Remove change-reducing "&" before the official commit. + const auto reply = &mem().freshestReply(); + time_t served_date = reply->date; int age = reply->header.getInt(Http::HdrType::AGE); /* Compute the timestamp, mimicking RFC2616 section 13.2.3. */ @@ -1495,6 +1497,30 @@ StoreEntry::timestampsSet() timestamp = served_date; + debugs(20, 5, *this << " has " << describeTimestamps()); + return true; +} + +bool +StoreEntry::updateOnNotModified(const StoreEntry &e304) +{ + assert(mem_obj); + assert(e304.mem_obj); + + // update reply before calling timestampsSet() below + const auto &oldReply = mem_obj->freshestReply(); + const auto updatedReply = oldReply.recreateOnNotModified(e304.mem_obj->baseReply()); + if (updatedReply) // HTTP 304 brought in new information + mem_obj->updateReply(*updatedReply); + // else continue to use the previous update, if any + + if (!timestampsSet() && !updatedReply) + return false; + + // Keep the old mem_obj->vary_headers; see HttpHeader::skipUpdateHeader(). + + debugs(20, 5, "updated basics in " << *this << " with " << e304); + mem_obj->appliedUpdates = true; // helps in triage; may already be true return true; } @@ -1639,33 +1665,11 @@ StoreEntry::flush() } } -int64_t -StoreEntry::objectLen() const -{ - assert(mem_obj != NULL); - return mem_obj->object_sz; -} - -int64_t -StoreEntry::contentLen() const -{ - assert(mem_obj != NULL); - assert(getReply() != NULL); - return objectLen() - getReply()->hdr_sz; -} - -HttpReply const * -StoreEntry::getReply() const -{ - return (mem_obj ? mem_obj->getReply().getRaw() : nullptr); -} - void StoreEntry::reset() { - assert (mem_obj); debugs(20, 3, url()); - mem_obj->reset(); + mem().reset(); expires = lastModified_ = timestamp = -1; } @@ -1754,7 +1758,7 @@ StoreEntry::storeErrorResponse(HttpReply *reply) { lock("StoreEntry::storeErrorResponse"); buffer(); - replaceHttpReply(reply); + replaceHttpReply(HttpReplyPointer(reply)); flush(); complete(); negativeCache(); @@ -1767,7 +1771,7 @@ StoreEntry::storeErrorResponse(HttpReply *reply) * a new reply. This eats the reply. */ void -StoreEntry::replaceHttpReply(HttpReply *rep, bool andStartWriting) +StoreEntry::replaceHttpReply(const HttpReplyPointer &rep, const bool andStartWriting) { debugs(20, 3, "StoreEntry::replaceHttpReply: " << url()); @@ -1776,7 +1780,7 @@ StoreEntry::replaceHttpReply(HttpReply *rep, bool andStartWriting) return; } - mem_obj->replaceReply(HttpReplyPointer(rep)); + mem_obj->replaceBaseReply(rep); if (andStartWriting) startWriting(); @@ -1791,8 +1795,12 @@ StoreEntry::startWriting() assert (isEmpty()); assert(mem_obj); - const HttpReply *rep = getReply(); - assert(rep); + // Per MemObject replies definitions, we can only write our base reply. + // Currently, all callers replaceHttpReply() first, so there is no updated + // reply here anyway. Eventually, we may need to support the + // updateOnNotModified(),startWriting() sequence as well. + assert(!mem_obj->updatedReply()); + const auto rep = &mem_obj->baseReply(); buffer(); rep->packHeadersUsingSlowPacker(*this); @@ -1810,14 +1818,14 @@ StoreEntry::startWriting() } char const * -StoreEntry::getSerialisedMetaData() +StoreEntry::getSerialisedMetaData(size_t &length) const { StoreMeta *tlv_list = storeSwapMetaBuild(this); int swap_hdr_sz; char *result = storeSwapMetaPack(tlv_list, &swap_hdr_sz); storeSwapTLVFree(tlv_list); assert (swap_hdr_sz >= 0); - mem_obj->swap_hdr_sz = (size_t) swap_hdr_sz; + length = static_cast(swap_hdr_sz); return result; } @@ -1880,7 +1888,6 @@ StoreEntry::trimMemory(const bool preserveSwappable) bool StoreEntry::modifiedSince(const time_t ims, const int imslen) const { - int object_length; const time_t mod_time = lastModified(); debugs(88, 3, "modifiedSince: '" << url() << "'"); @@ -1890,11 +1897,7 @@ StoreEntry::modifiedSince(const time_t ims, const int imslen) const if (mod_time < 0) return true; - /* Find size of the object */ - object_length = getReply()->content_length; - - if (object_length < 0) - object_length = contentLen(); + assert(imslen < 0); // TODO: Either remove imslen or support it properly. if (mod_time > ims) { debugs(88, 3, "--> YES: entry newer than client"); @@ -1902,22 +1905,16 @@ StoreEntry::modifiedSince(const time_t ims, const int imslen) const } else if (mod_time < ims) { debugs(88, 3, "--> NO: entry older than client"); return false; - } else if (imslen < 0) { - debugs(88, 3, "--> NO: same LMT, no client length"); - return false; - } else if (imslen == object_length) { - debugs(88, 3, "--> NO: same LMT, same length"); - return false; } else { - debugs(88, 3, "--> YES: same LMT, different length"); - return true; + debugs(88, 3, "--> NO: same LMT"); + return false; } } bool StoreEntry::hasEtag(ETag &etag) const { - if (const HttpReply *reply = getReply()) { + if (const auto reply = hasFreshestReply()) { etag = reply->header.getETag(Http::HdrType::ETAG); if (etag.str) return true; @@ -1946,7 +1943,7 @@ StoreEntry::hasIfNoneMatchEtag(const HttpRequest &request) const bool StoreEntry::hasOneOfEtags(const String &reqETags, const bool allowWeakMatch) const { - const ETag repETag = getReply()->header.getETag(Http::HdrType::ETAG); + const auto repETag = mem().freshestReply().header.getETag(Http::HdrType::ETAG); if (!repETag.str) { static SBuf asterisk("*", 1); return strListIsMember(&reqETags, asterisk, ','); diff --git a/src/store/Controller.cc b/src/store/Controller.cc index a08063449a..f651bd3bc6 100644 --- a/src/store/Controller.cc +++ b/src/store/Controller.cc @@ -677,20 +677,36 @@ Store::Controller::handleIdleEntry(StoreEntry &e) } void -Store::Controller::updateOnNotModified(StoreEntry *old, const StoreEntry &newer) +Store::Controller::updateOnNotModified(StoreEntry *old, StoreEntry &e304) { - /* update the old entry object */ Must(old); - HttpReply *oldReply = const_cast(old->getReply()); - Must(oldReply); + Must(old->mem_obj); + Must(e304.mem_obj); + + // updateOnNotModified() may be called many times for the same old entry. + // e304.mem_obj->appliedUpdates value distinguishes two cases: + // false: Independent store clients revalidating the same old StoreEntry. + // Each such update uses its own e304. The old StoreEntry + // accumulates such independent updates. + // true: Store clients feeding off the same 304 response. Each such update + // uses the same e304. For timestamps correctness and performance + // sake, it is best to detect and skip such repeated update calls. + if (e304.mem_obj->appliedUpdates) { + debugs(20, 5, "ignored repeated update of " << *old << " with " << e304); + return; + } + e304.mem_obj->appliedUpdates = true; - const bool modified = oldReply->updateOnNotModified(newer.getReply()); - if (!old->timestampsSet() && !modified) + if (!old->updateOnNotModified(e304)) { + debugs(20, 5, "updated nothing in " << *old << " with " << e304); return; + } + + if (sharedMemStore && old->mem_status == IN_MEMORY && !EBIT_TEST(old->flags, ENTRY_SPECIAL)) + sharedMemStore->updateHeaders(old); - // XXX: Call memStore->updateHeaders(old) and swapDir->updateHeaders(old) to - // update stored headers, stored metadata, and in-transit metadata. - debugs(20, 3, *old << " headers were modified: " << modified); + if (old->swap_dirn > -1) + swapDir->updateHeaders(old); } bool diff --git a/src/store/Controller.h b/src/store/Controller.h index 92404d7f1a..a209ceed47 100644 --- a/src/store/Controller.h +++ b/src/store/Controller.h @@ -88,8 +88,8 @@ public: /// called to get rid of no longer needed entry data in RAM, if any void memoryOut(StoreEntry &, const bool preserveSwappable); - /// update old entry metadata and HTTP headers using a newer entry - void updateOnNotModified(StoreEntry *old, const StoreEntry &newer); + /// using a 304 response, update the old entry (metadata and reply headers) + void updateOnNotModified(StoreEntry *old, StoreEntry &e304); /// tries to make the entry available for collapsing future requests bool allowCollapsing(StoreEntry *, const RequestFlags &, const HttpRequestMethod &); diff --git a/src/store_client.cc b/src/store_client.cc index 8c9923b917..1db21f89bf 100644 --- a/src/store_client.cc +++ b/src/store_client.cc @@ -509,18 +509,16 @@ store_client::readBody(const char *, ssize_t len) if (len < 0) return fail(); - if (copyInto.offset == 0 && len > 0 && entry->getReply()->sline.status() == Http::scNone) { + const auto rep = entry->mem_obj ? &entry->mem().baseReply() : nullptr; + if (copyInto.offset == 0 && len > 0 && rep && rep->sline.status() == Http::scNone) { /* Our structure ! */ - HttpReply *rep = (HttpReply *) entry->getReply(); // bypass const - - if (!rep->parseCharBuf(copyInto.data, headersEnd(copyInto.data, len))) { + if (!entry->mem_obj->adjustableBaseReply().parseCharBuf(copyInto.data, headersEnd(copyInto.data, len))) { debugs(90, DBG_CRITICAL, "Could not parse headers from on disk object"); } else { parsed_header = 1; } } - const HttpReply *rep = entry->getReply(); if (len > 0 && rep && entry->mem_obj->inmem_lo == 0 && entry->objectLen() <= (int64_t)Config.Store.maxInMemObjSize && Config.onoff.memory_cache_disk) { storeGetMemSpace(len); // The above may start to free our object so we need to check again @@ -845,27 +843,35 @@ CheckQuickAbortIsReasonable(StoreEntry * entry) return true; } - int64_t expectlen = entry->getReply()->content_length + entry->getReply()->hdr_sz; + const auto &reply = mem->baseReply(); - if (expectlen < 0) { - /* expectlen is < 0 if *no* information about the object has been received */ + if (reply.hdr_sz <= 0) { + // TODO: Check whether this condition works for HTTP/0 responses. debugs(90, 3, "quick-abort? YES no object data received yet"); return true; } - int64_t curlen = mem->endOffset(); - if (Config.quickAbort.min < 0) { debugs(90, 3, "quick-abort? NO disabled"); return false; } if (mem->request && mem->request->range && mem->request->getRangeOffsetLimit() < 0) { - /* Don't abort if the admin has configured range_ofset -1 to download fully for caching. */ + // the admin has configured "range_offset_limit none" debugs(90, 3, "quick-abort? NO admin configured range replies to full-download"); return false; } + if (reply.content_length < 0) { + // XXX: cf.data.pre does not document what should happen in this case + // We know that quick_abort is enabled, but no limit can be applied. + debugs(90, 3, "quick-abort? YES unknown content length"); + return true; + } + const auto expectlen = reply.hdr_sz + reply.content_length; + + int64_t curlen = mem->endOffset(); + if (curlen > expectlen) { debugs(90, 3, "quick-abort? YES bad content length (" << curlen << " of " << expectlen << " bytes received)"); return true; @@ -881,6 +887,7 @@ CheckQuickAbortIsReasonable(StoreEntry * entry) return true; } + // XXX: This is absurd! TODO: For positives, "a/(b/c) > d" is "a*c > b*d". if (expectlen < 100) { debugs(90, 3, "quick-abort? NO avoid FPE"); return false; diff --git a/src/store_log.cc b/src/store_log.cc index 3b82c53a84..584f5a556b 100644 --- a/src/store_log.cc +++ b/src/store_log.cc @@ -48,7 +48,7 @@ storeLog(int tag, const StoreEntry * e) ++storeLogTagsCounts[tag]; if (mem != NULL) { - reply = e->getReply(); + reply = &mem->freshestReply(); /* * XXX Ok, where should we print the dir number here? * Because if we print it before the swap file number, it'll break diff --git a/src/store_swapmeta.cc b/src/store_swapmeta.cc index bd9217c05a..32cb052fcf 100644 --- a/src/store_swapmeta.cc +++ b/src/store_swapmeta.cc @@ -35,7 +35,7 @@ storeSwapTLVFree(tlv * n) * Build a TLV list for a StoreEntry */ tlv * -storeSwapMetaBuild(StoreEntry * e) +storeSwapMetaBuild(const StoreEntry *e) { tlv *TLV = NULL; /* we'll return this */ tlv **T = &TLV; diff --git a/src/store_swapout.cc b/src/store_swapout.cc index 57611984ac..0c6eb5fcd4 100644 --- a/src/store_swapout.cc +++ b/src/store_swapout.cc @@ -57,7 +57,7 @@ storeSwapOutStart(StoreEntry * e) */ // Create metadata now, possibly in vain: storeCreate needs swap_hdr_sz. - const char *buf = e->getSerialisedMetaData (); + const auto buf = e->getSerialisedMetaData(mem->swap_hdr_sz); assert(buf); /* Create the swap file */ diff --git a/src/tests/stub_HttpHeader.cc b/src/tests/stub_HttpHeader.cc index b8a7b49a93..538cc246b9 100644 --- a/src/tests/stub_HttpHeader.cc +++ b/src/tests/stub_HttpHeader.cc @@ -28,7 +28,7 @@ HttpHeader::~HttpHeader() {STUB} HttpHeader &HttpHeader::operator =(const HttpHeader &other) STUB_RETVAL(*this) void HttpHeader::clean() STUB void HttpHeader::append(const HttpHeader *) STUB -bool HttpHeader::update(HttpHeader const *) STUB_RETVAL(false) +void HttpHeader::update(const HttpHeader *) STUB void HttpHeader::compact() STUB int HttpHeader::parse(const char *, size_t, Http::ContentLengthInterpreter &) STUB_RETVAL(-1) int HttpHeader::parse(const char *, size_t, bool, size_t &, Http::ContentLengthInterpreter &) STUB_RETVAL(-1) diff --git a/src/tests/stub_HttpReply.cc b/src/tests/stub_HttpReply.cc index 4d187dbd2c..9abf6247b8 100644 --- a/src/tests/stub_HttpReply.cc +++ b/src/tests/stub_HttpReply.cc @@ -29,7 +29,7 @@ bool HttpReply::parseFirstLine(const char *start, const char *end) STUB_RETVAL(f void HttpReply::hdrCacheInit() STUB HttpReply * HttpReply::clone() const STUB_RETVAL(NULL) bool HttpReply::inheritProperties(const Http::Message *aMsg) STUB_RETVAL(false) -bool HttpReply::updateOnNotModified(HttpReply const*) STUB_RETVAL(false) +HttpReply::Pointer HttpReply::recreateOnNotModified(const HttpReply &) const STUB_RETVAL(nullptr) int64_t HttpReply::bodySize(const HttpRequestMethod&) const STUB_RETVAL(0) const HttpHdrContRange *HttpReply::contentRange() const STUB_RETVAL(nullptr) void HttpReply::configureContentLengthInterpreter(Http::ContentLengthInterpreter &) STUB diff --git a/src/tests/stub_libstore.cc b/src/tests/stub_libstore.cc index 27325deef5..04c3cafee8 100644 --- a/src/tests/stub_libstore.cc +++ b/src/tests/stub_libstore.cc @@ -41,7 +41,7 @@ void Controller::updateLimits() STUB void Controller::handleIdleEntry(StoreEntry &) STUB void Controller::freeMemorySpace(const int) STUB void Controller::memoryOut(StoreEntry &, const bool) STUB -void Controller::updateOnNotModified(StoreEntry *, const StoreEntry &) STUB +void Controller::updateOnNotModified(StoreEntry *, StoreEntry &) STUB bool Controller::allowCollapsing(StoreEntry *, const RequestFlags &, const HttpRequestMethod &) STUB_RETVAL(false) void Controller::addReading(StoreEntry *, const cache_key *) STUB void Controller::addWriting(StoreEntry *, const cache_key *) STUB diff --git a/src/tests/stub_store.cc b/src/tests/stub_store.cc index c778338ece..af3261895f 100644 --- a/src/tests/stub_store.cc +++ b/src/tests/stub_store.cc @@ -26,14 +26,13 @@ bool StoreEntry::checkDeferRead(int fd) const STUB_RETVAL(false) const char *StoreEntry::getMD5Text() const STUB_RETVAL(NULL) StoreEntry::StoreEntry() STUB StoreEntry::~StoreEntry() STUB -HttpReply const *StoreEntry::getReply() const STUB_RETVAL(NULL) void StoreEntry::write(StoreIOBuffer) STUB bool StoreEntry::isAccepting() const STUB_RETVAL(false) size_t StoreEntry::bytesWanted(Range const, bool) const STUB_RETVAL(0) void StoreEntry::complete() STUB store_client_t StoreEntry::storeClientType() const STUB_RETVAL(STORE_NON_CLIENT) -char const *StoreEntry::getSerialisedMetaData() STUB_RETVAL(NULL) -void StoreEntry::replaceHttpReply(HttpReply *, bool andStartWriting) STUB +char const *StoreEntry::getSerialisedMetaData(size_t &length) const STUB_RETVAL(NULL) +void StoreEntry::replaceHttpReply(const HttpReplyPointer &, bool andStartWriting) STUB bool StoreEntry::mayStartSwapOut() STUB_RETVAL(false) void StoreEntry::trimMemory(const bool preserveSwappable) STUB void StoreEntry::abort() STUB @@ -87,8 +86,6 @@ void StoreEntry::operator delete(void *address) STUB void StoreEntry::buffer() STUB void StoreEntry::flush() STUB int StoreEntry::unlock(const char *) STUB_RETVAL(0) -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(const bool shareable) STUB diff --git a/src/tests/stub_store_swapout.cc b/src/tests/stub_store_swapout.cc index 61785e1456..a3cec9a7a6 100644 --- a/src/tests/stub_store_swapout.cc +++ b/src/tests/stub_store_swapout.cc @@ -18,6 +18,6 @@ void storeUnlink(StoreEntry * e) STUB char *storeSwapMetaPack(tlv * tlv_list, int *length) STUB_RETVAL(NULL) -tlv *storeSwapMetaBuild(StoreEntry * e) STUB_RETVAL(NULL) +tlv *storeSwapMetaBuild(const StoreEntry *) STUB_RETVAL(nullptr) void storeSwapTLVFree(tlv * n) STUB diff --git a/src/tests/testRock.cc b/src/tests/testRock.cc index c51c6031d4..b75bc327f6 100644 --- a/src/tests/testRock.cc +++ b/src/tests/testRock.cc @@ -189,8 +189,8 @@ testRock::createEntry(const int i) flags.cachable = true; StoreEntry *const pe = storeCreateEntry(storeId(i), "dummy log url", flags, Http::METHOD_GET); - HttpReply *const rep = const_cast(pe->getReply()); - rep->setHeaders(Http::scOkay, "dummy test object", "x-squid-internal/test", 0, -1, squid_curtime + 100000); + auto &rep = pe->mem().adjustableBaseReply(); + rep.setHeaders(Http::scOkay, "dummy test object", "x-squid-internal/test", 0, -1, squid_curtime + 100000); pe->setPublicKey(); @@ -203,7 +203,7 @@ testRock::addEntry(const int i) StoreEntry *const pe = createEntry(i); pe->buffer(); - pe->getReply()->packHeadersUsingSlowPacker(*pe); + pe->mem().freshestReply().packHeadersUsingSlowPacker(*pe); pe->flush(); pe->timestampsSet(); pe->complete(); diff --git a/src/tests/testUfs.cc b/src/tests/testUfs.cc index 8dcfb82c92..c77fbfec23 100644 --- a/src/tests/testUfs.cc +++ b/src/tests/testUfs.cc @@ -146,13 +146,13 @@ testUfs::testUfsSearch() RequestFlags flags; flags.cachable = true; StoreEntry *pe = storeCreateEntry("dummy url", "dummy log url", flags, Http::METHOD_GET); - HttpReply *rep = (HttpReply *) pe->getReply(); // bypass const - rep->setHeaders(Http::scOkay, "dummy test object", "x-squid-internal/test", 0, -1, squid_curtime + 100000); + auto &reply = pe->mem().adjustableBaseReply(); + reply.setHeaders(Http::scOkay, "dummy test object", "x-squid-internal/test", 0, -1, squid_curtime + 100000); pe->setPublicKey(); pe->buffer(); - pe->getReply()->packHeadersUsingSlowPacker(*pe); + pe->mem().freshestReply().packHeadersUsingSlowPacker(*pe); pe->flush(); pe->timestampsSet(); pe->complete(); diff --git a/src/urn.cc b/src/urn.cc index a6a2823eb3..24717438f9 100644 --- a/src/urn.cc +++ b/src/urn.cc @@ -293,7 +293,7 @@ urnHandleReply(void *data, StoreIOBuffer result) } s = buf + k; - assert(urlres_e->getReply()); + // TODO: Check whether we should parse urlres_e reply, as before 528b2c61. rep = new HttpReply; rep->parseCharBuf(buf, k); debugs(52, 3, "reply exists, code=" << rep->sline.status() << "."); -- 2.47.3