Fix default chunked transfer encoding and expose as configurable #3958
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit reverts the behaviour of the client to not use chunked tranfer encoding by default, and exposes chunked transfer encoding as a configuration value on IConnectionConfigurationValues and RequestConfiguration. With the introduction of the pull based streaming model in RequestDataContent, a ContentLength value cannot be computed for what will be written to the request stream; a ContentLength value is attempted to be determined, and TryComputeLength() executed, _before_ PostData is serialized to the request stream, making it not possible to provide a ContentLength value in the implementation, and falling into the HttpClient behaviour of using chunked transfer encoding. A potential fix could have been applied in RequestDataContent, to serialize PostData ahead of time and store in a byte array or stream field, and the logical place to do this would have been the ctor. It was determined however that this would be less than optimal to do because 1. It starts to overcomplicate the RequestDataContent implementation 2. An implementation in the RequestDataContent ctor would not have been able to use aynchronous methods for the RequestAsync<T> asynchronous path. Instead, the serialization of PostData happens in sync and async paths in HttpConnection, using ByteArrayContent and the written request bytes when DisabledDirectStreaming is enabled to avoid keeping two copies of the request bytes around. Additional integration tests added for HttpConnection and HttpWebRequestConnection to assert behaviour. Fixes #3952
russcam
commented
Jul 18, 2019
src/Tests/Tests/ClientConcepts/Connection/HttpWebRequestConnectionTests.cs
Outdated
Show resolved
Hide resolved
src/Tests/Tests/ClientConcepts/Connection/HttpWebRequestConnectionTests.cs
Outdated
Show resolved
Hide resolved
codebrain
approved these changes
Jul 18, 2019
Contributor
codebrain
left a comment
codebrain
left a comment
There was a problem hiding this comment.
LGTM, one small comment
russcam
added a commit
that referenced
this pull request
Jul 18, 2019
This commit reverts the behaviour of the client to not use chunked tranfer encoding by default, and exposes chunked transfer encoding as a configuration value on IConnectionConfigurationValues and RequestConfiguration. With the introduction of the pull based streaming model in RequestDataContent, a ContentLength value cannot be computed for what will be written to the request stream; a ContentLength value is attempted to be determined, and TryComputeLength() executed, _before_ PostData is serialized to the request stream, making it not possible to provide a ContentLength value in the implementation, and falling into the HttpClient behaviour of using chunked transfer encoding. A potential fix could have been applied in RequestDataContent, to serialize PostData ahead of time and store in a byte array or stream field, and the logical place to do this would have been the ctor. It was determined however that this would be less than optimal to do because 1. It starts to overcomplicate the RequestDataContent implementation 2. An implementation in the RequestDataContent ctor would not have been able to use aynchronous methods for the RequestAsync<T> asynchronous path. Instead, the serialization of PostData happens in sync and async paths in HttpConnection, using ByteArrayContent and the written request bytes when DisabledDirectStreaming is enabled to avoid keeping two copies of the request bytes around. Additional integration tests added for HttpConnection and HttpWebRequestConnection to assert behaviour. Fixes #3952 * Update default value * Update test name * Update test name (cherry picked from commit 3dcbd45)
russcam
added a commit
that referenced
this pull request
Jul 18, 2019
This commit reverts the behaviour of the client to not use chunked tranfer encoding by default, and exposes chunked transfer encoding as a configuration value on IConnectionConfigurationValues and RequestConfiguration. With the introduction of the pull based streaming model in RequestDataContent, a ContentLength value cannot be computed for what will be written to the request stream; a ContentLength value is attempted to be determined, and TryComputeLength() executed, _before_ PostData is serialized to the request stream, making it not possible to provide a ContentLength value in the implementation, and falling into the HttpClient behaviour of using chunked transfer encoding. A potential fix could have been applied in RequestDataContent, to serialize PostData ahead of time and store in a byte array or stream field, and the logical place to do this would have been the ctor. It was determined however that this would be less than optimal to do because 1. It starts to overcomplicate the RequestDataContent implementation 2. An implementation in the RequestDataContent ctor would not have been able to use aynchronous methods for the RequestAsync<T> asynchronous path. Instead, the serialization of PostData happens in sync and async paths in HttpConnection, using ByteArrayContent and the written request bytes when DisabledDirectStreaming is enabled to avoid keeping two copies of the request bytes around. Additional integration tests added for HttpConnection and HttpWebRequestConnection to assert behaviour. Fixes #3952 * Update default value * Update test name * Update test name
codebrain
pushed a commit
that referenced
this pull request
Jul 19, 2019
This commit reverts the behaviour of the client to not use chunked tranfer encoding by default, and exposes chunked transfer encoding as a configuration value on IConnectionConfigurationValues and RequestConfiguration. With the introduction of the pull based streaming model in RequestDataContent, a ContentLength value cannot be computed for what will be written to the request stream; a ContentLength value is attempted to be determined, and TryComputeLength() executed, _before_ PostData is serialized to the request stream, making it not possible to provide a ContentLength value in the implementation, and falling into the HttpClient behaviour of using chunked transfer encoding. A potential fix could have been applied in RequestDataContent, to serialize PostData ahead of time and store in a byte array or stream field, and the logical place to do this would have been the ctor. It was determined however that this would be less than optimal to do because 1. It starts to overcomplicate the RequestDataContent implementation 2. An implementation in the RequestDataContent ctor would not have been able to use aynchronous methods for the RequestAsync<T> asynchronous path. Instead, the serialization of PostData happens in sync and async paths in HttpConnection, using ByteArrayContent and the written request bytes when DisabledDirectStreaming is enabled to avoid keeping two copies of the request bytes around. Additional integration tests added for HttpConnection and HttpWebRequestConnection to assert behaviour. Fixes #3952 * Update default value * Update test name * Update test name
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR reverts the behaviour of the client to not use chunked tranfer encoding by default, and exposes chunked transfer encoding as a configuration value on IConnectionConfigurationValues and RequestConfiguration.
With the introduction of the pull based streaming model in RequestDataContent, a ContentLength value cannot be computed for what will be written to the request stream; a ContentLength value is attempted to be determined, and TryComputeLength() executed, before PostData is serialized to the request stream, making it not possible to provide a ContentLength value in the implementation, and falling into the HttpClient behaviour of using chunked transfer encoding.
A potential fix could have been applied in RequestDataContent, to serialize PostData ahead of time and store in a byte array or stream field, and the logical place to do this would have been the ctor. It was determined however that this would be less than optimal to do because
Instead, the serialization of PostData happens in sync and async paths in HttpConnection, using ByteArrayContent and the written request bytes when DisabledDirectStreaming is enabled to avoid keeping two copies of the request bytes around.
Additional integration tests added for HttpConnection and HttpWebRequestConnection to assert behaviour.
Fixes #3952