Skip to content

Conversation

@shinrich
Copy link
Member

@shinrich shinrich commented Jun 22, 2021

This PR sets up the response header read in parallel with the post tunnel. In some cases the origin may return an error response header for a POST before processing all the post data. With the current code that response is lost if the server closes the connection.

This PR is built upon PR #7962. Once that is committed, I will rebase this branch. All references to 100-continue belong with that PR.

I have added some tests to try and exercise the early server response.

This closes #4006

@shinrich shinrich added the HTTP label Jun 22, 2021
@shinrich shinrich added this to the 10.0.0 milestone Jun 22, 2021
@shinrich shinrich self-assigned this Jun 22, 2021
@shinrich shinrich requested review from bryancall and zwoop as code owners June 22, 2021 22:03
@shinrich shinrich force-pushed the early-post-response branch from 13a1bc0 to d6e2400 Compare June 23, 2021 16:28
@shinrich
Copy link
Member Author

Have been doing some production testing. Another commit coming to fix an edge case. But an interesting observation from the testing. Our number of ERR_CONNECT_FAIL in a 15 minute access log file drops to single digits from triple digits (~250) on a peer running without this patch. Digging in the differs seem to be POST requests to one of our origins. We the patches, we see non 200 responses coming back for the origin (503's and 400's) instead of non origin status and 502 sent back to the user agent.

@shinrich
Copy link
Member Author

Testing on 1 production machine. Added commit to clean up correctly in the cases where the server returns early with an error (i.e. EOS) during the dual post sending and response reading time.

@bryancall
Copy link
Contributor

Talked in the PR scrub and @shinrich is going to close the connection to the origin.

@shinrich
Copy link
Member Author

shinrich commented Jul 6, 2021

Updated to close connection to origin and adjustments made based on production experience. Ran on production machines in two different colos for a week.

@shinrich
Copy link
Member Author

Pulled into our standard build.

@bryancall bryancall requested a review from randall July 12, 2021 23:28
@bryancall
Copy link
Contributor

@randall is going to look at this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ATS does not propagate response header if response is sent and connection closed while post data tunnel is active

4 participants