TS-4044: The server_vc should be established (writeable) before request transform start.

[oknet]

The request transform would be applied to POST and/or PUT request.
The server_vc should be established (writeable) before request transform start.
The CheckConnect is created by connect_s,
It will callback NET_EVENT_OPEN to HttpSM if server_vc is WRITE_READY,
Otherwise NET_EVENT_OPEN_FAILED is callbacked.

[oknet]

This PR revert some code changed by commit 35d492a.

[oknet]

@shinrich I think there is a mistaken in the commit 35d492a.

The POST / PUT retry can not working on connection failed. ATS should get a established connection by connect_s() first before request transform start.

[oknet]

@zwoop This is a 7.1.0 candidate.

[SolidWallOfCode]

What is the issue if the transform begins before the origin is writeable? Does it cause the buffer to fill up, or is content discarded on a retry? Is the computation expense of the transform too much when requests fail frequently?

[oknet]

@SolidWallOfCode

The request transform starts after NET_EVENT_OPEN is received. Currently, the NET_EVENT_OPEN is callbacked by connect_re() but it doesnt mean the connection is ready to send request to server.

If the connection is not ready and the server request header sent out:

• The EVENT_ERROR will callback to HttpSM if NetHandler get an error on write().
• The EVENT_*_TIMEOUT will callback to HttpSM if connect is timeout

If ATS receives these above events, it will trigger the retry mechanism and chain_abort_all() will discard the data of transform

With the connect_s(), the EVENT_ERROR and EVENT_*_TIMEOUT are converted to NET_EVENT_OPEN_FAILED. The NET_EVENT_OPEN is ignored and the WRITE_READY is converted to NET_EVENT_OPEN.

With the connect_s(), the EVENT_ERROR and EVENT_*_TIMEOUT are converted to NET_EVENT_OPEN_FAILED. The NET_EVENT_OPEN is ignored then it callbacks NET_EVENT_OPEN after receiving the WRITE_READY.

When ATS receives NET_EVENT_OPEN_FAILED, it will also trigger the retry mechanism but the request transform is not started yet.

[oknet]

According to the email send from @keesspoelstra , if the the server connection retry mechanism enabled on POST and/or PUT request, ATS may crash on assert.

The retry will obviously decrease by this PR for POST and/or PUT request.
And the retry should be done before request transform starts.

@zwoop

[SolidWallOfCode]

Overall it seems reasonable. The question I have is, why use connect_re at all? Why not skip the method check and always use connect_s?

[shinrich]

Looks reasonable to me. The previous commit I made because it appeared that post/put would never make it to the else case. But if you explicitly push them to the else, then the extra timeout logic makes sense.

[zwoop]

Should we land this?

[bryancall]

Looks good. 👍

[oknet]

@SolidWallOfCode

Because different from connect_s, connect_re sends request to OS directly and only check WRITE_COMPLETE event while connect_s checks both WRITE_READY and WRITE_COMPLETE, which I believe connect_re has better performance.

[zwoop]

Cherry-picked to 7.1.x