Add extra state for allow_half_open setting

[shinrich]

Updates proxy.config.http.allow_half_open to have three values: 0 disabled, 1 enabled for non-tls, 2 enabled for tls and non-tls.

[shinrich]

Looks like do not allow half open changes more cases than strictly half open clients since it is affecting several autests. I must head out for jury duty, but I will take a look at this again this evening.

[shinrich]

For the redirect test anyway, the current tcp_client.py requires that half_open is on. I'm trying to adjust the test, so that is no longer required.

[zwoop]

Are we sure this is the route we want to go? I believe setting 0 and 1 will both cause assertions, leaving only #2 a viable configuration. And as such, at a minimum the default ought to be "2".

[zwoop]

Assuming this is the path we want to take, I feel strongly that the default should be "2", which is the only setting that has a chance of actually working reliably.

[zizhong]

do we want to change the default behavior to be half-open disabled?

[shinrich]

As it turns out any of the tests using tcp_client.py or netcat assume the half open semantics.

The slice plugin also does shutdown read and shutdown write. I think that is probably not what you want to do since it limits you to only one request from client connection. I removed the calls to TSVConnShutdown here.

If you want to keep the default to 1 or 2, I am ok with that. Probably without extra prod testing, we shouldn't change the default. If that is the consensus, I can drop the autest changes. But I think the slice changes should remain. Perhaps in a separate PR.

[shinrich]

Pushed a new version that does not change the default from 1. Based on discussions on slack, some people wanted to have the option of allowing half-open on TLS connections in cache there existed TLS clients that would take advantage of it. Thus the addition of the allow_half_open = 2.

Also in HttpSM::tunnel_handler_ua I added a comment about the POST check there. I don't think the logic there is really to handle half open, but rather to handle clean up in the error case. Should address this in another PR to handle that clean up more directly.

[zwoop]

I'm still not thrilled about this, I think this is the worst possible scenario, in that we have a configuration option where 2 out of 3 settings are almost certain to cause problems (assertion). I expressed this on the Slack channel as well, but I'd much rather see one of these three solutions:

1. Revert more of the things that breaks (i.e. the code that breaks TLS)

2. (preferred) Restore the patch from Two more places to check if attempting half_closed connection logic is feasible #3714 (which was reverted in 8.0.2, Revert "Two more places to check whether attempting half_closed connecton logic is feasible" #4716). Then we have to fix the keep-alive issues with HTTPS.

3. Fix the assertion such that it doesn't happen. This is likely tricky, since we still don't understand under what conditions this reproduces, and it's a rare event.

#1 would then mostly be the same as option "2" here I think, and #2 would be mostly as the existing code.

I'm having a hard time seeing that people can make educated guesses which of 0|1|2 they would want set here. At a minimum, I think we should default to "2" if we still want this patch, since "2" is the only known state without crashes.

[zwoop]

@thelounge-zz Is there any chance you can test this patch, and set this setting to 2? This doesn't cherry-pick cleanly into the 8.0.x branch, I can either make you a patch for 8.0.x, or make a new tar-file with this patch applied.

[sudheerv]

+1 to half of what @zwoop said :) We do need the assert-less support for half-open = 0 (disabled) as we've half open disabled on our parent layer and partially disabled (for some endpoints) on our edge layer. If disabling half-open still causes an assert, it'd be a problem for us.

[shinrich]

Are other people seeing the assert as well? I have been unable to reproduce the assert described, so kind of shooting in the dark here. I don't see how allowing half-open connections over TLS would prevent the dead state machine assert other than by introducing an inactivity timeout on POST requests and having the delays avoid the race condition.

[thelounge-zz]

@thelounge-zz Is there any chance you can test this patch, and set this setting to 2? This doesn't cherry-pick cleanly into the 8.0.x branch, I can either make you a patch for 8.0.x, or make a new tar-file with this patch applied.

i guess it's about #4921 (comment)
update and "records.conf: CONFIG proxy.config.http.allow_half_open INT 2"

please give me a fresh tarball because i don't do anything outside rpmbuild and so "Version: 8.0.6" in the specfile with a matching trafficserver-8.0.6.tar.bz2 would a) not introdcue messing around and b) make it easy to upgrade / downgrade in case things aren't smooth

has "records.conf: CONFIG proxy.config.http.allow_half_open INT 2" any impact on 8.0.1 or can it stay there in case of upgrades/downgrades?

[zwoop]

@shinrich So, I made a tar-ball for @thelounge-zz , which essentially does this except without a configuration option, and he's still seeing the assertion. This makes me believe that this patch is unlikely to fix anything either.

[zwoop]

I think we should close this for now, the tests done by @thelounge-zz indicates that this would not help the assertion :-/.

[shinrich]

Closing