<div dir="ltr"><div dir="ltr">>> Could you please give me some advice on a better/proper fix, if close()<br>>> at "abandoning" time is wrong?<br>><br>> Unfortunately, I cannot. <br><br>In fact, your message was very helpful! Thank you.<br><br><br>> In general, responding with a 403 does not invalidate the client<br>> connection, so it does not have to be closed. Said that, I am sure there<br>> are use cases where such closure would be a good idea. I would not be<br>> surprised if Squid closes the connection after denying regular requests.<br><br>Yes, rfc7231 states that:<br>   Any 2xx (Successful) response indicates that the sender (and all<br>   inbound proxies) will switch to tunnel mode immediately after the<br>   blank line that concludes the successful response's header section;<br>   data received after that blank line is from the server identified by<br>   the request-target.  Any response other than a successful response<br>   indicates that the tunnel has not yet been formed and that the<br>   connection remains governed by HTTP.<br><br>In case of non-2xx http response from icap reqmod, the client connection should continue to function in http mode, so there is no need to close it.<br><br>In case of 2xx http response from icap reqmod, Squid will not do any tunneling stuff for this CONNECT request, and thus it would be better to close the client connection. However I am not aware of any reasonable scenario where icap (reqmod) server would rewrite a CONNECT request with 2xx http response.<br><br><br>> What is currently missing (at least) is understanding of what is going<br>> on. The correct fix, whatever it is, would be determined by that<br>> understanding.<br><br>The reason ConnStateData::kick() gets into the "abandoning" branch is due to a combination of the following circumstances:<br><br>1. Function clientProcessRequest() executes this code:<br>    // Let tunneling code be fully responsible for CONNECT requests<br>    if (http->request->method == Http::METHOD_CONNECT) {<br>        context->mayUseConnection(true);<br>        conn->flags.readMore = false;<br>    }<br><br>2. No tunneling is actually set up in case of icap reqmod rewrite of CONNECT request with http response;<br><br>3. Conditions for ConnStateData::kick() "abandoning" branch are satisfied after the processing of CONNECT request finishes.<br><br><br>ACL deny of CONNECT request could trigger the same "abandoning" conditions, but it does not, because:<br><br>1. ClientRequestContext::clientAccessCheckDone(ACCESS_DENIED) sets<br>ClientRequestContext::readNextRequest and ClientRequestContext::error variables;<br><br>2. ClientHttpRequest::doCallouts() checks those variables and executes:<br>    getConn()->flags.readMore = true; // resume any pipeline reads.<br><br><br>I tried to apply the same approach (as in the ACL) for the case of ICAP REQMOD, please see attached patch.<br>I've done limited testing and the patch seems to fix the problem.<br><br>Is ClientHttpRequest::handleAdaptedHeader() time suitable for re-enabling connection readMore flag and for connection stopReceiving()?<br>Would it be better to postpone the re-enabling /  to a later stage stopReceiving()?<br>Maybe to ClientHttpRequest::endRequestSatisfaction() ?<br><br><br>Thanks!<br></div><div><br></div><div><br></div><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Dec 11, 2020 at 6:40 PM Alex Rousskov <<a href="mailto:rousskov@measurement-factory.com">rousskov@measurement-factory.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On 12/10/20 3:33 PM, Alexey Sergin wrote:<br>
<br>
> - Squid writes to cache.log a message like "kick abandoning <....>";<br>
<br>
These messages indicate a Squid bug, most likely in REQMOD request<br>
satisfaction implementation specific to CONNECT use cases. The messages<br>
are not prefixed with a "BUG" label, but they should be.<br>
<br>
<br>
> - Squid does not close the file descriptor used for http client connection.<br>
<br>
Yes, that is a likely side effect of the above-mentioned bug.<br>
<br>
<br>
> client_side.cc. Closing clientConnection right after<br>
> "debugs(<....>abandoning<....>)" fixes the leak.<br>
<br>
> Is it ok to always close() clientConnection when "abandoning" thing<br>
> happens? <br>
<br>
> Are there any known scenarios where this close() would be<br>
> inappropriate?<br>
<br>
Unknown. Such a closure (alone) is not a proper fix. If well-tested, it<br>
may be considered to be a good-enough workaround, but no more than that.<br>
<br>
What is currently missing (at least) is understanding of what is going<br>
on. The correct fix, whatever it is, would be determined by that<br>
understanding.<br>
<br>
In general, responding with a 403 does not invalidate the client<br>
connection, so it does not have to be closed. Said that, I am sure there<br>
are use cases where such closure would be a good idea. I would not be<br>
surprised if Squid closes the connection after denying regular requests.<br>
<br>
<br>
> Could you please give me some advice on a better/proper fix, if close()<br>
> at "abandoning" time is wrong?<br>
<br>
Unfortunately, I cannot. Somebody needs to investigate the problem and<br>
identify the bug in ConnStateData::kick() logic (or elsewhere).<br>
<br>
Alex.<br>
</blockquote></div></div>