- ICAP-unrelated improvements from the squid3-icap branch on SF
(see further below for ICAP-specific improvements):
- Replaced BodyReader with BodyPipe. BodyReader was a
collection of function pointers augmented with body size
calculation logic. BodyReader was used to deliver request
body (of a known size) from the client side to the server
side. Reference counting was used to communicate abort
conditions to the other side (it did not work well because
decreasing the reference count does not have any side-effects
if the count remains positive). Direct calls between sides
sometimes resulted in a call-me-when-I-am-calling-you "loops"
and related bugs.
BodyPipe is used to deliver request or response body (possibly
of unknown size) from the body producer to the body consumer.
A producer can be the client side (for virgin requests), the
server side (for virgin replies), or the ICAP side (for
adapted messages). A consumer can be the client side (for
adapted responses, including responses in a request
satisfaction mode), the server side (for adapted requests),
and the ICAP side (for virgin requests and responses).
BodyPipe uses asynchronous calls for communication between
sides to avoid call-me-when-I-am-calling-you "loops".
BodyPipe has methods to communicate normal termination and
abort conditions to the other side. The use of those methods
is mandatory. Reference counting is used only as a garbage
collection mechanism.
BodyPipe is used to read request bodies, including requests
for which there is no consumer and the connection is in a
'closing' state. BodyPipe can auto-consume body so that a
'closing' connection does not have to rely on the body
consumer presence when eating up remaining body data.
If auto-consumption is turned on and the pipe starts
consuming before a real consumer is attached to the pipe, the
setConsumerIfNotLate call fails, and the real consumer has to
handle the failure.
The new BodyPipe approach should make support for HTTP/1.1
chunked requests easier. Only a few places in the pipe-related
code assume that the request size is known.
- Removed ClientBody as unused, replaced by BodyReader, then
BodyPipe.
- Moved HttpRequest::body_reader to HttpMsg::body_pipe so that
all HTTP message bodies can be communicated via pipes. This
is needed for the server side to supply response bodies to
ICAP and for the ICAP side to supply adapted message bodies
to others.
- When cleaning HttpRequest or HttpReply, reset body_pipe to
NULL instead of asserting that it is already NULL. BodyPipes
are owned and maintained by other objects and HttpMsg is used
only as a mechanism to pass the pipe pointer from the body
producer to the consumer. To maintain guarantees similar to
the old code, the BodyPipe destructor asserts that both the
producer and the consumer are gone when the pipe is
destructed.
- When appending body data, do not append more than the known
body size. This fixes the following assertion when POSTing
from IE in my tests: assertion failed: client_side.cc:3205:
"bodySizeLeft() > 0".
I suspect IE or some Javascripts running on IE were appending
extra CRLF to a POST, exposing the bug, and triggering the
above assertion.
- WARNING: Ftp-specific BodyPipe changes are untested, but the
old code probably did not work well with ICAP either. More
testing is needed.
- Moved more common server-side code from http.* and ftp.* into
Server.*. Most ICAP-related code is in the Server class now.
The code move to the Server class and migration to BodyPipe
exposed several FTP/HTTP inconsistencies and bugs. I marked
those I could not fix with XXXs.
- Distinguish the end of communication with the origin server
from the end of communication with ICAP. Clean them up
separately when possible. Terminate when both are completed
(or aborted).
- Polished persistentConnStatus() to avoid calling
statusIfComplete() until really necessary (and appropriate).
This makes debugging easier to understand for some.
- Use auto-consumption feature to consume data from closing
connections for which there is no real body consumer.
- Use BodyPipe for maintaining the "closing" state of a
connection instead of in.abortedSize. This change "removes" a
few memory leaks and an assertion, but does need more work,
especially when the regular BodyPipe consumer leaves early
and does not consume the request body.
- The client stream code sometimes marks the "closing"
connection as STREAM_UNPLANNED_COMPLETE, leading to a
double-close. I do not yet understand why. There is now code
to ignore multiple attempts to enter the "closing" state.
- ICAP improvements from the squid3-icap branch on SF, including:
- Added icap_service_failure_limit squid.conf option. The limit
specifies the number of failures that Squid tolerates when
establishing a new TCP connection with an ICAP service. If
the number of failures exceeds the limit, the ICAP service is
not used for new ICAP requests until it is time to refresh
its OPTIONS. The per-service failure counter is reset to zero
each time Squid fetches new service OPTIONS.
A negative value disables the limit.
The limit used to be hardcoded to 10.
(based on the patch by Axel Westerhold)
- Added icap_service_revival_delay squid.conf option. The
delay specifies the number of seconds to wait after an ICAP
OPTIONS request failure before requesting the options again.
The failed ICAP service is considered "down" until fresh
OPTIONS are fetched.
The actual delay cannot be smaller than the [still] hardcoded
minimum delay of 60 seconds.
(based on the patch by Axel Westerhold)
- Added icap_client_username_header and
icap_client_username_encode squid.conf options to control how
the authenticated client username should be sent to the ICAP
service. (based on the patch by Axel Westerhold)
- Handle REQMOD transaction failures where we cannot proceed
with the normal request flow.
- Use ICAPInitiator API to send "success" or "abort" messages
to ICAP transaction initiator. Store virgin and adapted
metadata as public fields (if the newly added ICAPInOut type)
that the initiator can access when receiving our "successful
adaptation" message. This keeps messages simple.
- Using ICAPInitiator API and a "universal" BodyPipe API makes
it possible to exchange bodies directly with client- or
server-side code without ICAPClient* translators, which are
now gone along with the ICAPInitXaction function in
ICAPClient.
- Added ICAPInitiator interface that classes initiating ICAP
transactions must now support. The API currently has just two
methods: one for receiving adapted message headers
(indicating a successful ICAP transaction, at least to the
point of fetching adapted headers) and one for receiving a
notification of an aborted ICAP transaction.
Most ICAP initiators (or their close relatives) will also need
to implement BodyConsumer and/or BodyProducer APIs to exchange
virgin and/or adapted HTTP message bodies with the initiated
ICAP transaction. However, that activity is not addressed in
this API. New AsyncCall API is used to declare the callback
wrappers.
- Use BodyPipe instead of MsgPipe for receiving virgin and
sending adapted message bodies. BodyPipe is not much
different from MsgPipeBody, but it is better to use a
"universal" class that the rest of Squid code now uses. One
complication is that BodyPipes are currently not created for
messages with zero-size bodies. The code had to be changed to
not assume that a zero-size body comes with a pipe.
- Deleted MsgPipe and related classes. Message pipes had two
purposes: coordinate HTTP message adaptation (start, get the
adapted headers, abort) and exchange HTTP message bodies. The
latter is now done via BodyPipe API. The former can be
implemented directly in ICAPModXact.
Deleted ICAPClient* and related classes as (my) design
failure.
The original idea behind message pipes and ICAPClient* classes
was to isolate ICAP code from the Squid core. The core code
was supposed to use ICAPClient* classes for all ICAP-related
needs, and ICAPClient* classes were supposed to translate core
needs into "ICAP needs" and use message pipes to communicate
with asynchronously running ICAP transactions. The latter part
worked fine, but the former did not.
The core code still did a lot of ICAP-specific work on its
own. This could be because ICAP processing affects the flow so
much or because the core code had not been refactored enough
to minimize ICAP interactions. Whatever the reason, we ended
up with a lot of complex code/logic coordinating the core code
and ICAPClient* classes. While ICAPClient* classes were
"translating", they could not hide the key actions or events
(such as message body exchange or transaction aborts) from the
core. The core code still had to support those actions or
handle those events. Thus, every major action or event was
handled twice: once in the core side code and once in a
ICAPClient* class.
Removing ICAPClient* "translation" step simplified the code
and possibly improved performance. As for the "ICAP
separation" goal, the current exposure to the ICAPModXact
class can be hidden by a generic "Message Adaptation
Transaction" class if we need to support more adaptation
protocols. The core code should not be affected much by such a
change.
- ClientHttpRequest: Support the new ICAPInitiator API and talk
to ICAPModXact directly instead of using ICAPClient* classes,
which are now gone.
- ConnStateData: Use BodyPipe for delivering virgin request
bodies to the server or ICAP side. Implement the BodyProducer
interface. ClientHttpRequest: Use BodyPipe instead of
BodyReader when receiving request bodies (from client side or
ICAP). Implement the BodyConsumer interface. See the first
BodyPipe CVS log message for the rationale.
- Use BodyPipe for delivering virgin reply bodies to ICAP and
receiving adapted reply bodies from ICAP. Implement the
BodyProducer interface.
Use BodyPipe instead of BodyReader when receiving request
bodies (from client side or ICAP). Implement the BodyConsumer
interface.
- Replaced never-failing doIcap() with startIcap() that fails
if we cannot select an ICAP service or the selected service
is not usable. Rearranged
ClientRequestContext::icapAclCheckDone() to bypass ICAP
errors when possible. Now, ClientRequestContext::startIcap()
is very similar to Server::startIcap(). Same for
icapAclCheckDone(). Made
ClientHttpRequest::handleIcapFailure() public because
ClientRequestContext::icapAclCheckDone() calls it.
- Polished TTL handling to make sure we use the default TTL
when the ICAP server did not provide an explicit value or if
we failed to communicate with the server. The latter case may
not have been handled correctly before.
- The minimum options update gap (currently hard-coded) must be
smaller than the default options TTL. Otherwise, we get stale
options and down ICAP services around the update time because
we cannot update soon enough.
- Support asynchronous transaction start. This allows for a
better handling of startup errors (or at least makes them
similar to other transaction errors).
- Call a swanSong() method upon expected transaction
termination (including aborts). This allows for proper and
prompt [partial] transaction cleanup, without waiting for the
destructor to be called. The destruction may be delayed by
refcounting if we have other transaction users waiting for
some transaction notifications.
- Do not reuse a connection if we are still reading or writing
(even if no actual I/O is scheduled). The old code would
reuse such connections, and read/write leftovers from aborted
transactions from/to the ICAP server.
- Do not send last-chunk in ICAP Preview with a null-body. It is
possible that the old code would send the last-chunk under
some Preview conditions with null-body, but I am not sure.
- Fixed HttpStateData memory leak visible when no RESPMOD
services are enabled. ICAPAccessCheck constructor was
cbdata-locking HttpStateData, but was not releasing the lock
when there was no matching service class, leading to an
HttpStateData leak. Furthermore, ICAPAccessCheck would then
call HttpStateData back without validating the cbdata
pointer, probably calling wrong or invalid HttpStateData.
- Fixed "is it too late to bypass?" conditions in
ClientHttpRequest::handleIcapFailure(). We should be able to
bypass more often now. However, handleIcapFailure() still has
the old bug: it does not check whether the service is
optional. The current fix implies that now Squid may bypass
essential services more often.
- Call storeEntry()->complete() when ending request
satisfaction. Without this call, we may keep the connection
open, which does not work with responses that mark the end of
their body by closing a connection. (Christos Tsantilas)
- Fixed ieof condition detection. Squid was sending last-chunk
without ieof bit and was sending two last chunks when doing
preview (Tsantilas Christos).
- When ICAP server wants the entire virgin body and sends 100
Continue after Preview, do not stop backing up virgin body
data for echoing if we promised to support 204 No Content
responses outside of Preview. If we allow 204s, 100 Continue
may be followed by a 204 No Content and we will need the
entire virgin body to echo back.
- Rewrote MemBufClaim into a VirginBodyAct class to simplify
and clarify code in hope to minimize the number of bugs like
the one mentioned above. MemBufClaim was protecting an area
of virgin body pipe content and, as a side effect, was
providing the transaction with the content pointer for the
write or send actions.
Now VirginBodyAct just maintains the activity offset and the
transaction code uses that to consume virgin body correctly.
The size of the area is no longer maintained because it was
usually unknown or unused; and when it was known and used
(i.e., Preview), it could be taken from the preview state
object anyway. Renamed and documented VirginBodyAct-related
methods to clarify the intent.
- When sending last-chunk in Preview, send ieof extension if we
wrote the entire body. The old code would not send ieof if we
wrote as many bytes as promised in the Preview header, even
if we promised to write everything. This would mislead
compliant ICAP servers that do not look at the Content-Length
header and reply with 100 Continue, expecting more body data.
- Do not reset Preview size to zero when expecting a virgin
body of unknown size. A Squid user reported that this change
works.
- Polished debugging: Instead of using pointers, use unique ICAP
transaction IDs. This helps with isolating a transaction in a
large log, where pointers may be reused many times. Print
connection descriptor like most of the core code does. Other
minor improvements.