]> git.ipfire.org Git - thirdparty/squid.git/commit - src/ipc/StoreMap.h
Re-enabled updates of stored headers on HTTP 304 responses (#485)
authorAlex Rousskov <rousskov@measurement-factory.com>
Sat, 12 Oct 2019 00:40:35 +0000 (00:40 +0000)
committerSquid Anubis <squid-anubis@squid-cache.org>
Sat, 12 Oct 2019 00:40:40 +0000 (00:40 +0000)
commit66d51f4febd9fb534d065e50423addae265ee966
treeee455f2adcbcc58fbb1729488a0e6387fa59d0ad
parentccfbe8f49d36e8cce928b552a2ad7b4343ccdabd
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/
41 files changed:
src/FwdState.cc
src/HttpHeader.cc
src/HttpHeader.h
src/HttpReply.cc
src/HttpReply.h
src/MemObject.cc
src/MemObject.h
src/MemStore.cc
src/Store.h
src/StoreMeta.h
src/StoreMetaUnpacker.h
src/acl/Asn.cc
src/client_side.cc
src/client_side_reply.cc
src/client_side_reply.h
src/client_side_request.cc
src/client_side_request.h
src/clients/Client.cc
src/fs/rock/RockHeaderUpdater.cc
src/http.cc
src/http/Stream.cc
src/icmp/net_db.cc
src/ipc/StoreMap.cc
src/ipc/StoreMap.h
src/peer_digest.cc
src/refresh.cc
src/store.cc
src/store/Controller.cc
src/store/Controller.h
src/store_client.cc
src/store_log.cc
src/store_swapmeta.cc
src/store_swapout.cc
src/tests/stub_HttpHeader.cc
src/tests/stub_HttpReply.cc
src/tests/stub_libstore.cc
src/tests/stub_store.cc
src/tests/stub_store_swapout.cc
src/tests/testRock.cc
src/tests/testUfs.cc
src/urn.cc