Do not abandon client connection after CONNECT satisfaction

[Le-onardo]

kick abandoning conn... local=... remote=... FD ... flags=1

When a REQMOD service satisfies a CONNECT request (e.g., by sending
Squid an HTTP error response known as a "block page"), Squid should
forward the received HTTP response to the client and then, if various
conditions allow it to do so, resume processing new requests on the
client-to-Squid connection. Instead, connection handling code got stuck
after logging a cryptic "abandoning" notice for the client connection.

Squid ignored client-initiated connection closures and did not close the
abandoned connection for client_lifetime that defaults to 24 hours.

Since 2011 commit f35961a, CONNECT turned flags.readMore off. That
change created several bugs because it is difficult for developers to
realize that some flag needs to be turned back on in a given context:

• 2011 commit 83d4cd1 fixed bumped TLS connection handling
• 2011 commit 6b355e5 fixed CONNECT authentication and REQMOD failures
• this commit fixes CONNECT requests satisfied by a REQMOD service

The following enhancements are outside this surgical fix scope: We may
find or add a place where all CONNECT handling is completed (without
passing control to TunnelStateData). We might start monitoring client
connection for closures and prefetch client data while handling CONNECT.

[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]

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.

[yadij]

FWIW; HTTP specification compliance would be for Squid to obey any the "Connection:close" or "Connection:keep-alive" header passed in by ICAP. Adding "Connection:close" explicitly if none is provided on a status 200-599 response.

A tunnel can then be considered the same as an unknown-length POST transaction for closure conditions specifically regarding "Connection" and "Expect" header implications on whether more data is expected from each endpoint.

[yadij]

To clarify, what I mean is that we cannot just have this blanket always-close behaviour.

What if the reply generated is a "100 Continue" status message required to make the client start sending tunnel data? or a connection authentication challenge with "Connection:keep-alive"? this PR as-is would close immediately and break things when we actually want keep-alive behaviour.

However a 2xx-5xx "final" response type should be passed on and then close (no tunnel == nothing after the CONNECT headers). Even if they lack a "Connection:close" from ICAP ... unless if they explicitly say "Connection:keep-alive".

[rousskov]

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

Yes, it does. I just need more time to propose the right fix; unfortunately, it will probably take me more than two weeks. Thank you for answering my earlier questions. The ball is on my side.

[rousskov]

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

Alex: Yes, it does. I just need more time to propose the right fix; unfortunately, it will probably take me more than two weeks. Thank you for answering my earlier questions. The ball is on my side.

Finally done! I pushed the changes that address my concern. They work in my primitive CONNECT satisfaction tests that are based on your environment description. Please test in your environment.

I also adjusted PR title/description (i.e. the future official commit message). Please review and adjust further as needed.

Finally, CI tests fail because you are not listed in the CONTRIBUTORS file. Please add yourself, using the diff from the failed CI source maintenance test as a guidance -- you should pick one of the two entries shown in the diff. That correction will cancel my PR approval, but I will restore it.

N.B. There is no need to squash your PR branch changes and usually not need to rebase your PR branch on the latest master. Anubis, our merge bot, will squash branch commits for you when this PR is ready to go in.

P.S. It would be good to refactor related code to stop chasing special cases where flags.readMore needs to be turned back on. As far as CONNECTs are concerned, it is even possible to completely remove our reliance on that flag. I started to work on this, but had to stop because these improvements would require significant code refactoring that Squid is not ready for yet. This surgical fix should be good enough for now.

[rousskov]

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

Yes, it does. I just need more time to propose the right fix; unfortunately, it will probably take me more than two weeks. Thank you for answering my earlier questions. The ball is on my side.

[rousskov]

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

Alex: Yes, it does. I just need more time to propose the right fix; unfortunately, it will probably take me more than two weeks. Thank you for answering my earlier questions. The ball is on my side.

Finally done! I pushed the changes that address my concern. They work in my primitive CONNECT satisfaction tests that are based on your environment description. Please test in your environment.

I also adjusted PR title/description (i.e. the future official commit message). Please review and adjust further as needed.

Finally, CI tests fail because you are not listed in the CONTRIBUTORS file. Please add yourself, using the diff from the failed CI source maintenance test as a guidance -- you should pick one of the two entries shown in the diff. That correction will cancel my PR approval, but I will restore it.

N.B. There is no need to squash your PR branch changes and usually not need to rebase your PR branch on the latest master. Anubis, our merge bot, will squash branch commits for you when this PR is ready to go in.

P.S. It would be good to refactor related code to stop chasing special cases where flags.readMore needs to be turned back on. As far as CONNECTs are concerned, it is even possible to completely remove our reliance on that flag. I started to work on this, but had to stop because these improvements would require significant code refactoring that Squid is not ready for yet. This surgical fix should be good enough for now.