Bug 3190: Large HTTP POST stuck after early ICAP 400 error response
When an ICAP REQMOD service responds with an error to
(or the REQMOD transaction aborts while processing) a large HTTP
request, the HTTP request may get stuck because the request body
buffer gets full and nobody consumes the no-longer-needed content.
The ICAP code quits but leaves the body buffer intact in case the
client-side code wants to bypass the error. After that, nobody consumes
the request body because the buggy client side does not inform the body
pipe that there will be no other consumers, which would have triggered
a noteBodyConsumerAborted() callback and enable auto-consumption or closed
the client connection.
While the server writes the response to Store, the client side may
synchronously abort the entry. This happens, for example, when the
server receives a 304 response and handleIMSReply calls
sendClientOldEntry, which calls storeUnregister with our entry,
resulting in CheckQuickAbort.
Once server store write returns, if the server is done, it calls
FwdState::completed(). At that time, the server does not know that (and
should not care whether) the entry was aborted. Thus, we need to handle
aborted entries in FwdState::completed.
Optimized HttpHdrCc::packInto and clarified its behaviour in comments
Negative values to Cache-Control directives are now explicitly discarded
Clarified documentation
Explicitly check that integer-carrying Cache-control directives are positive.
Rename MAX_STALE_ALWAYS to MAX_STALE_ANY and changed its representation to very large positive integer.
Implemented internal HttpHdrCc::setValue call.
Removed unnecessarily explicitly-masked StringArea default constructor.
avoided temporary in HttpHdrCc name-to-id lookup
Started implementing have/set/get/clear combo for all members of HttpHdrCc
Fixed zero-pointer-dereference in http.cc
Remove SwapDir::reconfigure() arguments since they are not used.
Before the change, SwapDir::reconfigure() took index and path
arguments, but none of them was actually used: neither index nor path
can be changed during reconfigure. And both index and path are
available as SwapDir members so there is no reason to have these
arguments.
Ignore and warn about attempts to reconfigure static Rock store options.
Some Rock store options cannot be changed dynamically: path, size, and
max-size. Before the change, there were no checks during reconfigure
to prevent changing these options. This may lead to Rock cache
corruption and other bugs. The patch adds necessary checks to Rock
store code. If user tries to change an option that cannot be updated
dynamically, a warning is reported and the value is left unchanged.
Bug fix: "(ssl_crtd): Cannot add certificate to db" when updating expired cert
When ssl_crtd helper needs to add a fresh certificate to the database but
finds an expired certificate already stored, ssl_crtd deletes the expired
certificate file from disk before adding the fresh one. However, the addition
still fails because the expired certificate was not removed from database
indexes.
This fix:
- Adds code to update database indexes upon deletion of a row.
- Polishes certificates deletion code to avoid duplication.
TODO: Report failure details to Squid and make certificate-specific failures
not fatal for the ssl_crtd helper.
Alex Rousskov [Wed, 21 Sep 2011 18:05:55 +0000 (12:05 -0600)]
Temporary fix: Avoid killing Coordinator with unregistered cache mgr actions
that cause isOpen() assertions.
If a worker forwards a cache manager request to Coordinator and Coordinator
does not have that action registered, CacheManager::createRequestedAction()
throws (as it should) and Mgr::Request cleanup asserts when its half-baked
connection tries to close a not-yet-imported socket descriptor.
This workaround catches the exception, reports it, and manually closes the
socket descriptor. It also prevents an ACK response from being sent to the
worker, which triggers a worker timeout.
Mid-term TODO: Coordinator should register all actions that are known to kids.
Should Coordinator respond with an error instead of relying on a timeout?
Long-term TODO: Consider an API where cache manager responses can be
aggregated and formatted by Coordinator without knowing action-specific
details. After all, there are not so many types of action information (size,
count, rate, etc.) and most actions have simple reporting formats. Currently,
it is awkward to guarantee that Coordinator and all workers know all actions,
especially when some actions may be specific to non-worker kids such as
Coordinator and diskers.
Alex Rousskov [Thu, 15 Sep 2011 18:16:59 +0000 (12:16 -0600)]
Do not let cache manager requests kill SMP Squid using isOpen() assertion.
As the comment above the close call implies, we have not imported the foreign
socket descriptor into our fd_table yet. We must use raw close(2), just like
the corresponding Mgr::Request::Request(msg) code that allocates request.conn,
uses raw assignment to give that half-baked connection a descriptor.
TODO: This direct manipulation of Connection::fd is ugly, and this half-baked
connection will most likely cause more [hidden] problems down the road. For
example, Mgr::Request destructor will assert in a similar way if the request
object is destroyed before Action::respond() is called.
Alex Rousskov [Thu, 15 Sep 2011 03:44:50 +0000 (21:44 -0600)]
Check for and use __sync_sub_and_fetch() and such for atomic decrement.
This is possibly more efficient or perhaps even the only correct way to
decrement atomically. It also helps ICC compiler happier. Somehow, I missed
that __sync_fetch_and_sub() and such exist!
Alex Rousskov [Wed, 14 Sep 2011 20:07:28 +0000 (14:07 -0600)]
Made Ipc::StoreMap::Shared public to make ICC compiler happier.
Ipc::StoreMap class kids need access to Shared::SharedMemorySize (at least) so
the Shared type declaration should be at least protected. Made it public
because the currently public Ipc::StoreMap::Owner typedef uses it.
Introduced experimental strblob class to more efficiently handle Cache-Control header lookups
Various performance improvements.
Renamed HttpHdrCc::parseInit() to parse()
Reverted some changes in HTtpHdrCc::parse() to facilitate review.
Removed useless HttpHdrCc::setSMaxAge
Removed Author attribution - too many authors to single any one out
Removed parametric HttpHdrCc constructor, made parametric.
Removed duplicate mempool definition
Alex Rousskov [Tue, 13 Sep 2011 16:47:32 +0000 (10:47 -0600)]
SMP Caching: Core changes, IPC primitives, Shared memory cache, and Rock Store
Core changes
------------
* Added MemObject::expectedReplySize() and used it instead of object_sz.
When deciding whether an object with a known content length can be
swapped out, do not wait until the object is completely received and its
size (mem_obj->object_sz) becomes known (while asking the store to
recheck in vain with every incoming chunk). Instead, use the known
content length, if any, to make the decision.
This optimizes the common case where the complete object is eventually
received and swapped out, preventing accumulating potentially large
objects in RAM while waiting for the end of the response. Should not
affect objects with unknown content length.
Side-effect1: probably fixes several cases of unknowingly using negative
(unknown) mem_obj->object_sz in calculations. I added a few assertions
to double check some of the remaining object_sz/objectLen() uses.
Side-effect2: When expectedReplySize() is stored on disk as StoreEntry
metadata, it may help to detect truncated entries when the writer
process dies before completing the swapout.
* Removed mem->swapout.memnode in favor of mem->swapout.queue_offset.
The code used swapout.memnode pointer to keep track of the last page
that was swapped out. The code was semi-buggy because it could reset the
pointer to NULL if no new data came in before the call to doPages().
Perhaps the code relied on the assumption that the caller will never
doPages if there is no new data, but I am not sure that assumption was
correct in all cases (it could be that I broke the calling code, of course).
Moreover, the page pointer was kept without any protection from page
disappearing during asynchronous swapout. There were "Evil hack time"
comments discussing how the page might disappear.
Fortunately, we already have mem->swapout.queue_offset that can be fed
to getBlockContainingLocation to find the page that needs to be swapped
out. There is no need to keep the page pointer around. The
queue_offset-based math is the same so we are not adding any overheads
by using that offset (in fact, we are removing some minor computations).
* Added "close how?" parameter to storeClose() and friends.
The old code would follow the same path when closing swapout activity
for an aborted entry and when completing a perfectly healthy swapout. In
non-shared case, that could have been OK because the abort code would
then release the entry, removing any half-written entry from the index
and the disk (but I am not sure that release happened fast enough in
100% of cases).
When the index and disk storage is shared among workers, such
"temporary" inconsistencies result in truncated responses being
delivered by other workers to the user because once the swapout activity
is closed, other workers can start using the entry.
By adding the "close how?" parameter to closing methods we allow the
core and SwapDir-specific code to handle aborted swapouts appropriately.
Since swapin code is "read only", we do not currently distinguish
between aborted and fully satisfied readers: The readerGone enum value
applies to both cases. If needed, the SwapDir reading code can make that
distinction by analyzing how much was actually swapped in.
* Moved "can you store this entry?" code to virtual SwapDir::canStore().
The old code had some of the tests in SwapDir-specific canStore()
methods and some in storeDirSelect*() methods. This resulted in
inconsistencies, code duplication, and extra calculation overheads.
Making this call virtual allows individual cache_dir types to do custom
access controls.
The same method is used for cache_dir load reporting (if it returns
true). Load management needs more work, but the current code is no worse
than the old one in this aspect, and further improvements are outside
this change scope.
Moved common (and often rather complex!) code from store modules into
storeRebuildLoadEntry, storeRebuildParseEntry, and storeRebuildKeepEntry.
* Do not set object_sz when the entry is aborted because the true object
size (HTTP reply headers + body) is not known in this case. Setting
object_sz may fool client-side code into believing that the object is
complete.
This addresses an old RBC's complaint.
* When swapout initiation fails, mark swapout decision as
MemObject::SwapOut::swImpossible. This prevents the caller code from trying to
swap out again and again because swap_status becomes SWAPOUT_NONE.
TODO: Consider add SWAPOUT_ERROR, STORE_ERROR, and similar states. It
may solve several problems where the code sees _NONE or _OK and thinks
everything is peachy when in fact there was an error.
* Call haveParsedReplyHeaders() before entry->replaceHttpReply().
HaveParsedReplyHeaders() sets the entry public key and various flags (at
least). ReplaceHttpReply() packs reply headers, starting swapout process.
It feels natural to adjust the entry _before_ we pack/swap it, but I may be
missing some side-effects here.
The change was necessary because we started calling checkCachable() from
swapoutPossible(). If haveParsedReplyHeaders() is not called before we swap
out checks, the entry will still have the private key and will be declared
impossible to cache.
* Extracted the write-to-store step from StoreEntry::replaceHttpReply().
This allows the caller to set the reply for the entry and then update the
entry and the reply before writing them to store. For example, the server-side
haveParsedReplyHeaders() code needs to set the entry timestamps and make the
entry key public before the entry starts swapping out, but the same code also
needs access to entry->getReply() and such for timestampsSet() and similar
code to work correctly.
TODO: Calls to StoreEntry::replaceHttpReply() do not have to be modified
because replaceHttpReply() does write by default. However, it is likely that
callers other than ServerStateData::setFinalReply() should take advantage of
the new split interface because they call timestampsSet() and such after
replaceHttpReply().
* Moved SwapDir::cur_size and n_disk_objects to specific SwapDirs. Removed
updateSize(). Some cache_dirs maintain their own maps and size statistics,
making the one-size-fits-all SwapDir members inappropriate.
* A new SwapDir public method swappedOut() added. It is called from
storeSwapOutFileClosed() to notify SwapDir that an object was swapped
out.
* Change SwapDir::max_size to bytes, make it protected, use maxSize() instead.
Change SwapDir::cur_size to bytes, make it private, use currentSize() instead.
Store Config.Store.avgObjectSize in bytes to avoid repeated and error-prone
KB<->bytes conversions.
* Change Config.cacheSwap.swapDirs and StoreEntry::store() type to SwapDir.
This allows using SwapDir API without dynamic_cast.
* Always call StoreEntry::abort() instead of setting ENTRY_ABORTED manually.
* Rely on entry->abort() side-effects if ENTRY_ABORTED was set.
* Added or updated comments to better document current code.
* Added operator << for dumping StoreEntry summary into the debugging
log. Needs more work to report more info (and not report yet-unknown info).
* Fixed blocking reads that were sometimes reading from random file offsets.
Core "disk file" reading code assumed that if the globally stored disk.offset
matches the desired offset, there is no reason to seek. This was probably done
to reduce seek overhead between consecutive reads. Unfortunately, the disk
writing code did not know about that optimization and left F->disk.offset
unchanged after writing.
This may have worked OK for UFS if it never writes to the file it reads from,
but it does not work for store modules that do both kinds of I/O at different
offsets of the same disk file.
Eventually, implement this optimization correctly or remove disk.offset.
IPC primitives
--------------
To make SMP disk and memory caching non-blocking and correct, worker and
disker processes must asynchronously communicate with each other. We are
adding a collection of classes that support such communication.
At the base of the collection is the AtomicWordT template that uses GCC atomic
primitives such as __sync_add_and_fetch() to perform atomic operations on
integral values in memory shared by multiple Squid kids. AtomicWordT is used
to implement non-blocking shared locks, queues, store tables, and page pools.
To avoid blocking or very long searches, many operations are "optimistic" in
nature. For example, it is possible that an atomic store map will refuse to
allocate an entry for two processes even though a blocking implementation
would have allowed one of the processes to get the map slot. We speculate that
such conflict resolution is better than blocking locks when it comes to
caching, especially if the conflicts are rare due to large number of cache
entries, fast operations, and relatively small number of kids.
TODO: Eventually, consider breaking locks left by dead kids.
The shared memory cache keeps its own compact index of cached entries using
extended Ipc::StoreMap class (MemStoreMap). The cache also strives to keep its
Root.get() results out of the store_table except during transit.
Eventually, the non-shared/local memory cache should also be implemented
using a MemStore-like class, I think. This will allow to clearly isolate
local from shared memory cache code.
Allow the user to explicitly disable shared memory caching in SMP mode via
memory_cache_shared to squid.conf. Report whether mem_cache is shared.
Disable shared memory caching by default if atomic operations are not
supported. Prohibit shared memory caching if atomic operations are not
supported.
TODO: Better limits/separation for cache and I/O shared memory pages.
Eventually, support shared memory caching of multi-page entries.
Rock Store
----------
Rock Store uses a single [large] database-style file per cache_dir to store
cached responses and metadata. This part of the design is similar to COSS.
Rock Store does not maintain or rely on swap.state "log" for recovery.
Instead, the database is scanned in the background to load entries when Squid
starts. Rock Store maintains its own index of cached entries and avoids global
store_table. All entries must be max-size or smaller.
In SMP mode, each Rock cache_dir is given a dedicated Kid processes called
"disker". All SMP workers communicate with diskers to store misses and load
hits, using shared memory pages and atomic shared memory queues. Disker blocks
when doing disk I/O but workers do not. Any Diskers:Workers ratio is supported
so that the user can find and configure the optimal number of workers and
diskers for a given number of disks and CPU cores.
In non-SMP mode, should use good old blocking disk I/O, without any diskers,
but this has not been tested recently and probably needs more work.
Removed httpHdrCcParseCreate as a first-class method, it only had one in-class caller.
HttpHdrCc::parseInit now takes a const String reference argument.
Same function of httpHdrCcParseCreate can be obtained by new HttpHdrCc() + HttpHdrCc::parseInit()
Alex Rousskov [Sat, 10 Sep 2011 01:25:27 +0000 (19:25 -0600)]
Moved squid.conf global disk_io_timeout to cache_dir-local swap-timeout.
The I/O timeout option belongs to cache_dir because not all cache_dir types
support it and because different cache_dirs may need different timeout values,
especially if some of them handle very large or otherwise unusual files.
To propagate the knowledge of the option down to DiskIO/IpcIoFile I decided
to add a DiskFile::Config class and DiskFile::configure() method. At first
glance that API does not belong to DiskFile because only IpcIoFile supports
it. However, DiskFile may be a better location for it because
* Other specific DiskIO files may want to support the same configuration
API.
* Placing API in IpcIoFile would require either making Rock Store dependent
on IpcIoFile (in terms of linking and in terms of availability) or more
complex API with multiple inheritance, dynamic casting and such. We can
introduce the "more complex API" mentioned above later if needed.
Renamed "disk_io" to "swap" timeout because the option is about the whole swap
out/in delay (something an admin may care about) and not individual I/O
(something only low-level code should know about).
This patch adjusts the %la logformat code handling for intercepted connections
based on the following rules:
- If the corresponding http_port or https_port option has an explicit
listening host name or IP address, then log the IP address.
- Otherwise, log a dash character.
Also adjusts %lp logformat code handling for intercepted connections to always
log the port number from the corresponding http_port or https_port option.
Amos comments about %la formating code:
For the record these are the permutations we seek to cover...
Scenario 1: client 192.168.0.3 connects to google (74.125.237.81). Gets intercepted into Squid.
Alex Rousskov [Fri, 9 Sep 2011 16:33:54 +0000 (10:33 -0600)]
Fixed max-stale check. Entities not exceeding max-stale were marked as stale.
Since the fixed check is performed for entities already suspected of being
stale by refreshCheck(), it is difficult to describe exactly which entities
were affected by the bug. A rough description would be "entities which would
otherwise qualify for a FRESH_OVERRIDE_EXPIRES or FRESH_OVERRIDE_LASTMOD
exceptions located below the fixed check.
Other concerns about staleness checks have been discussed on squid-dev's
"max_stale broken?" email thread.
Alex Rousskov [Fri, 9 Sep 2011 16:16:43 +0000 (10:16 -0600)]
Added RunnersRegistry, an API to register and, later, run a group of actions.
Useful for keeping general initialization/cleanup management code (e.g.,
main.cc) independent from specific initialization/cleanup code (e.g.,
Store file systems or memory cache) during staged initialization and
cleaning.
Designed with Rock Store needs in mind. Currently unused. Should eventually be
used for most modules initialization and cleanup, removing main.cc dependency
on those modules and perfecting [de]initialization order.
projects / thirdparty / squid.git / log
