proxy.config.http.connect_attempts_timeout tracks TTBF instead of connect

[shinrich]

I ran tests with proxy.config.http.connect_attempts_timeout set to 3 and the http transact inactivity timeout set to 30. I had an origin that would connect right away but wait for 10 seconds before returning. Without this fix, curl requests would return with a timeout error. With this fix, curl will return in 10 seconds.

We will add a ua_test soon. Need to set up a test origin (or augment microserver) to have a delay between connect and response.

[zwoop]

Nice! This is a long outstanding issue, I’ll do some tests with it next week as well.

[zwoop]

[approve ci autest]

[shinrich]

Pushing a new version. The autest caught a regression with posts. Turns out for posts/pushes there was existing code in connect_s that kind of did the wait for write logic. I don't know why we would treat get differently from push/post. In this version I unified the logic and removed the connect_s path entirely.

[duke8253]

Looks good to me. I've also setup microserver with a 10 second delay when sending response, and this fix works without a problem.

[zwoop]

I'm going to remove the 7.1.5 Project on this, since going forward, we will need to make a second PR against the 7.1.x for proposed back ports.

[ezelkow1]

Reverting this in #4503 for 7x, it was causing a crash for us with the latest 7x RC

[zwoop]

From #4503:

Testing the latest 7.1.x we saw a crash that kept coming up, https://gist.github.com/ezelkow1/6514f3ddb7a68e2b3279ae3a26cb0021

This seemed to be triggered by an unknown event in the state machine:
[Oct 26 18:25:07.032] Server {0x2b9b78a09700} ERROR: <HttpSM.cc:1803 (state_http_server_open)> [HttpSM::state_http_server_open] Unknown event: 105 (VC_EVENT_INACTIVITY_TIMEOUT)

I tracked it down to fadddc, and to do the revert I also had to back out a few changes from b6f422

The crash would be triggered within a few minutes of running synthetic-production traffic. With this revert I ran for 30 minutes and never got the crash

[shinrich]

The problem is that 7.1.x and 8.0.x are missing the fix in PR #4020. Specifically the addition of the VC_EVENT_INACTIVITY_TIMEOUT (next to the VC_EVENT_ACTIVE_TIMEOUT) in the case statement in HttpSM::state_http_server_open.

[shinrich]

This PR is in master and 7.1.x but not 8.x.

[bryancall]

I cherry picked it here:

commit 3434a5d51b3f8b8a64c9d4fdd1873914c8a1c950
Author:     Susan Hinrichs <shinrich@oath.com>
AuthorDate: Fri Jul 27 15:01:23 2018
Commit:     Bryan Call <bcall@apache.org>
CommitDate: Mon Oct 22 09:27:15 2018

    proxy.config.http.connect_attempts_timeout tracks TTBF instead of connect

    (cherry picked from commit 9b47aa6799db12fd302ea58e3ae0ba8485fd0bbe)

And @zwoop reverted it here:

commit 639372c4d559ffe4910a93ca98d011c88789ac30
Author:     Leif Hedstrom <leif@ogre.com>
AuthorDate: Fri Oct 26 16:07:00 2018
Commit:     Leif Hedstrom <leif@ogre.com>
CommitDate: Fri Oct 26 16:07:00 2018

    Revert "proxy.config.http.connect_attempts_timeout tracks TTBF instead of connect"

    This reverts commit 9b47aa6799db12fd302ea58e3ae0ba8485fd0bbe.

Don't remember why it was reverted. @zwoop do you remember why?

[jvgutierrez]

@bryancall please note that (at least) 8.0.x needs #5811 along with #4028

[zwoop]

@SolidWallOfCode @shinrich Are there any other PRs related to this one that we would need ? We'd like this for 8.1.x I think.

[jvgutierrez]

as mentioned in my previous comment and in #6415 , we needed #5811 @zwoop

[zwoop]

Cherry-picked to 8.1.x

[zwoop]

Reverted this again, because it caused build failures.