Skip to content

Conversation

@masaori335
Copy link
Contributor

Prior to this change, HTTP/2 has its own ChunkedHandler and do de-chunk in Http2Stream. But think about HTTP/1.0, it doesn't support chunked transfer coding either, but HttpTunnel can handle it correctly.

HttpTransact::handle_response_keep_alive_headers() set HttpTransact::ConnectionAttributes::receive_chunked_response depends on the HTTP version and configs, but the conditions doesn't check the HTTP/2 or HTTP/3 correctly.

Mark as draft PR for discussion & testing.

@masaori335 masaori335 added this to the 10.0.0 milestone Sep 2, 2019
@masaori335 masaori335 self-assigned this Sep 2, 2019
@maskit
Copy link
Member

maskit commented Sep 3, 2019

strncmp(s->state_machine->plugin_tag, "http/2", 6) v.s. strncmp(s->state_machine->client_protocol, "http/2", 6) ?

@masaori335
Copy link
Contributor Author

strncmp(s->state_machine->plugin_tag, "http/2", 6) v.s. strncmp(s->state_machine->client_protocol, "http/2", 6) ?

"Hide whitespace changes" would show the real changes clearly.

@shinrich
Copy link
Member

shinrich commented Sep 3, 2019

So if the http/1.1 origin returns a chunked response, we would fail to deliver it to a HTTP/2 or 3 client with this change?

What about a HTTP/2 client that sends a post with no content length for the post body?

@masaori335
Copy link
Contributor Author

So if the http/1.1 origin returns a chunked response, we would fail to deliver it to a HTTP/2 or 3 client with this change?

HttpTunnel does de-chunk the chunked response from the origin server and gives it to HTTP/2 or HTTP/3 TXN. From HTTP/2 and HTTP/3 TXN, it looks a normal response without a Content-Length header.
This is the same behavior of what ATS does when the client is HTTP/1.0 and the origin returns a chunked response.

What about a HTTP/2 client that sends a post with no content length for the post body?

I think this change affects the response side only. Because the action of Http2Stream chunk_handler is DECHUNK. I'll make sure.

@masaori335 masaori335 changed the title Disable chunked transfer coding if client protocol is HTTP/2 or HTTP/3 Dechunk chunked contents on HttpTunnel if client protocol is HTTP/2 or HTTP/3 Sep 6, 2019
@masaori335
Copy link
Contributor Author

Pushed new commit using ProxyTransaction::is_chunked_encoding_supported() instead of checking HTTP versions.

@masaori335
Copy link
Contributor Author

masaori335 commented Sep 6, 2019

I did a test with nghttp and --no-content-length. It looks like this change has no impact on the POST request. https://gist.github.com/masaori335/73e217db5efe70b7b7400c8da7e5501e

Also, Test Case 6 of http2.test.py covers this

# Test Case 6: Post with chunked body
# While HTTP/2 does not support Tranfer-encoding we pass that into curl to encourage it to not set the content length
# on the post body
tr = Test.AddTestRun()
tr.Processes.Default.Command = 'curl -s -k -H "Transfer-Encoding: chunked" -d "{0}" https://127.0.0.1:{1}/postchunked'.format( post_body, ts.Variables.ssl_port)
tr.Processes.Default.ReturnCode = 0
tr.Processes.Default.Streams.All = "gold/post_chunked.gold"
tr.StillRunningAfter = server

@masaori335 masaori335 force-pushed the h2-disable-chunk branch 4 times, most recently from 5b5df90 to e785557 Compare September 11, 2019 02:19
@masaori335 masaori335 marked this pull request as ready for review September 11, 2019 03:34
vmamidi
vmamidi previously approved these changes Oct 18, 2019
@masaori335
Copy link
Contributor Author

@vmamidi PTAL. Rebased on the latest master to resolve conflicts.

@masaori335 masaori335 merged commit 3ac74b5 into apache:master Oct 25, 2019
@zwoop
Copy link
Contributor

zwoop commented Nov 7, 2019

Cherry-picked to v9.0.x branch.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants