Fix abandoned connections on CONNECT ICAP handler

[Le-onardo]

When an ICAP REQMOD service returns a replacement reply (block page)
for a CONNECT request, handleAdaptedHeader() in
client_side_request.cc enters request_satisfaction_mode and sends
the reply directly through the HTTP stream chain. Since the CONNECT
was intercepted before reaching processRequest() in
client_side_request.cc where tunnelStart() would be called, there
is no tunnel and the block page is delivered as a normal HTTP
response.

CONNECT sets readMore to false in client_side.cc (expecting tunnel
takeover) and proxyKeepalive to true in client_side.cc (HTTP/1.1
default). Since request_satisfaction_mode bypasses the normal error
handling path where proxyKeepalive would be corrected, it is never
set to false. Therefore writeComplete() in Stream.cc skips the
closing of the connection and kick() in client_side.cc subsequently
finds an empty pipeline with readMore still set to false, abandoning
the connection with no read handler, no timeout, and no close for
up to 24 hours (client_lifetime).

A blocked CONNECT has no tunnel, so the connection should close
after delivering the reply. Set proxyKeepalive to false for CONNECT
requests entering request_satisfaction_mode in
handleAdaptedHeader(), so writeComplete() in Stream.cc closes the
connection after the block page is sent.

To reproduce: configure an ICAP REQMOD service that returns a
replacement reply (such as a block page) for CONNECT requests, then
send a CONNECT to a blocked destination. The ICAP server returns a
block page via REQMOD reply. Without the fix, the connection remains
open after the block page is delivered, abandoning a connection.
The fix introduces a way to close the connection after delivering
the block page.

[rousskov]

Thank you for working on this bug fix and detailing your understanding of the related code logic.

Just to avoid a misunderstanding: None of my specific review questions and concerns are an indication that Squid functions correctly or that no fix is needed here.

[rousskov]

PR description: When an ICAP REQMOD service returns a replacement reply (block page) for a CONNECT request

In your environment,

1. Was the CONNECT request received from a client (as opposed to faked by Squid)?
2. Does the client open a TCP connection to Squid (as opposed to Squid intercepting a client connection)?

For example, if you are dealing with a real CONNECT that the client sends to Squid after opening a TCP connection to http_port, then the answers are "yes" and "yes".

3. When the bug you are fixing manifests itself, do you get a level-1 WARNING: kick..abandoning... message in cache.log?

Squid needs to correctly handle all answer combinations, but knowing your environment(s) may help me guide this PR towards a proper long-term fix of the underlying problem...

[Le-onardo]

1 and 2 are a yes. It is a real connection to Squid from a client.

As to 3, this specific log line in client_side.cc (952-955) gets triggered if my described case is manifesting, which would be DBG_IMPORTANT

} else {
        // XXX: Can this happen? CONNECT tunnels have deferredRequest set.
        debugs(33, DBG_IMPORTANT, MYNAME << "abandoning " << clientConnection);
    }

As the comment indicates this is a path that would be unexpected to even be hit, but it is, as the connection is kept alive via the proxyKeepalive flag and flags.readMore being false.

[rousskov]

I suspect that we do not want to force the client-Squid connection to be closed after serving this adaptation-generated response, regardless of the request method. I assume that closing the connection makes the bug you observe go away, but that improvement alone is not sufficient to justify adding this special behavior. Most likely, we need more/different changes to properly address the underlying bug.

[Le-onardo]

If the connection is not closed it lingers as an open file descriptor and is not re-used anywhere. Rationally speaking I would expect that the connection would need to be closed at some point. Whether it is in the handleAdaptedHeader function or somewhere later. It seems as if there is a confusion between when a CONNECT tunnel is set and actually a HTTP response is set via the adaption generated response; if the latter case is hit, the tunnel should probably be not regarded as such but rather like a regular HTTP connection, thus going through the Stream handler, where the connection is closed "properly".

If this fix in he handleAdaptedHeader is not sufficient, I hope it works as an indication of an underlying issue.