Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tweak AndroidMessageHandler behavior for WCF support #7785

Merged
merged 23 commits into from
Mar 2, 2023

Conversation

grendello
Copy link
Contributor

Context: #7230
Context: dotnet/runtime#80935

When a WCF application receives compressed content that is automatically
decompressed in AndroidMessageHandler, it will most of the time fail
to properly read the response, cutting reading of the decompressed
content short. The reason for this is that when AndroidMessageHandler
creates a wrapper decompression stream, it does not update the
Content-Length to match the length of the decoded content, because it
doesn't have a way to know what the length is without first reading the
stream to the end, and that might prevent the end user to read the
content. Additionally, I think the Content-Length header should
reflect the original content length, for the end user to be able to
interpret the response as it was sent.

WCF, on the other hand, looks at the Content-Length header and, if
found, it takes its value and reads only this many bytes from the
content stream and no more - it will almost always result in short reads
and failure to correctly interpret the response.

I implemented this workaround which makes AndroidMessageHandler behave
the same way as other handlers implemented in the BCL. What they do in
this situation, is to remove the Content-Length header, making WCF
read stream to the end. Additionally, the clients remove the compressed
content encoding identifier from the Content-Encoding header.

As a bonus, this commit also adds support for decompression of responses
compressed with the Brotli compression (using the br encoding ID in
the Content-Encoding header)

Context: dotnet#7230
Context: dotnet/runtime#80935

When a WCF application receives compressed content that is automatically
decompressed in `AndroidMessageHandler`, it will most of the time fail
to properly read the response, cutting reading of the decompressed
content short.  The reason for this is that when `AndroidMessageHandler`
creates a wrapper decompression stream, it does not update the
`Content-Length` to match the length of the decoded content, because it
doesn't have a way to know what the length is without first reading the
stream to the end, and that might prevent the end user to read the
content.  Additionally, I think the `Content-Length` header should
reflect the **original** content length, for the end user to be able to
interpret the response as it was sent.

WCF, on the other hand, looks at the `Content-Length` header and, if
found, it takes its value and reads only this many bytes from the
content stream and no more - it will almost always result in short reads
and failure to correctly interpret the response.

I implemented this workaround which makes `AndroidMessageHandler` behave
the same way as other handlers implemented in the BCL.  What they do in
this situation, is to remove the `Content-Length` header, making WCF
read stream to the end.  Additionally, the clients remove the compressed
content encoding identifier from the `Content-Encoding` header.

As a bonus, this commit also adds support for decompression of responses
compressed with the `Brotli` compression (using the `br` encoding ID in
the `Content-Encoding` header)
@simonrozsival
Copy link
Member

The code looks good to me. Could you please add a test that would get a response from https://httpbin.org/gzip and https://httpbin.org/brotli and check that it works properly?

Comment on lines 67 to 69
"assemblies/System.IO.Compression.Brotli.dll": {
"Size": 11852
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to avoid the new assembly? It seems like any app that uses HttpClient will now bring in BrotliStream? Maybe that is OK?

If we added some feature flag, then you'd have to enable a setting to use it. I'm not sure if that is better.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The assembly is so tiny that I don't think it's problem, especially that the native library is 700k and included anyway (it contains all the System.IO.Compression native code to handle Deflate, GZip and Brotli).

* main:
  [lgtm] Fix LGTM-reported issues (dotnet#1074)
* main:
  [tests] Bump NUnit versions to latest (dotnet#7802)
  [Microsoft.Android.Sdk.ILLink] target `net7.0` temporarily (dotnet#7803)
  [tests] `InstallAndroidDependenciesTest` can use `platform-tools` 34.0.0 (dotnet#7800)
  Bump to xamarin/Java.Interop/main@9e0a469 (dotnet#7797)
  [Xamarin.Android.Build.Tasks] FileWrites&libraryprojectimports.cache (dotnet#7780)
  Bump to dotnet/installer@d25a3bb 8.0.100-preview.2.23105.6 (dotnet#7769)
* main:
  Add Unit Test for testOnly apps (dotnet#7637)
  [build] Only build the latest API level (dotnet#7786)
  [Xamarin.Android.Build.Tasks] Improve aapt2+file not found handling (dotnet#7644)
  [MSBuildDeviceIntegration] Fix duplicated test parameter (dotnet#7809)
* main:
  LEGO: Merge pull request 7825
  Bump to xamarin/Java.Interop/main@bbaeda6f (dotnet#7799)
  Bump NDK to r25c (dotnet#7808)
  [Xamarin.Android.Build.Tests] Improve logcat logging in failed tests (dotnet#7816)
  [Mono.Android] Update api-compat reference file for current API-33 (dotnet#7822)
  Localized file check-in by OneLocBuild Task (dotnet#7820)
* main:
  Revert "Bump to dotnet/installer@d25a3bb 8.0.100-preview.2.23105.6 (dotnet#7769)"
  LEGO: Merge pull request 7828
HttpClient adds it back, reflecting the uncompressed content length
@grendello
Copy link
Contributor Author

@simonrozsival turns out the test cannot check whether the Content-Length header has been removed. We remove it in the message handler, but HttpClient adds it back, with the value specifying the size of uncompressed content.

@simonrozsival
Copy link
Member

@grendello I see. That's not what I was expecting, but it makes sense. Let's add an assert that verifies that the number of bytes in the response content is equal to the value returned by the ContentLength property. That'll confirm that this PR fixes the WCF issue.


Assert.IsTrue (responseBody.Length > 0, "Response was empty");
Assert.AreEqual (response.Content.Headers.ContentLength, responseBody.Length, "Retrieved data length is different than the one specified in the Content-Length header");
Assert.IsTrue (responseBody.Contains ($"\"{jsonFieldName}\"", StringComparison.OrdinalIgnoreCase), $"\"{jsonFieldName}\" should have been in the response JSON");
Copy link
Member

@simonrozsival simonrozsival Feb 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's strange that it's failing in some test configurations and passing in other, especially for "gzipped". Maybe we could dump the responseBody to see if the response from the server is what we expect?

Is it possible that sometimes the server doesn't respond with a 200 (because of some rate limiting)? We should ignore the test result if the response is 4xx or 5xx.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I found it curious too. Rate limiting is a possibility, definitely. It would be great if we could host httpbin somewhere, probably need to ask around about this.

I ran the tests many times locally and always got the valid answer, so the principle of the test is solid. I will add check for 4xx and 5xx errors.

* main:
  [ci] Use AZDO built-in parallelization strategy. (dotnet#7804)
  Bump to dotnet/installer@e3ab0b5 8.0.100-preview.2.23123.10 (dotnet#7813)
  [ci] Run nunit tests with stable .NET version (dotnet#7826)
  [monodroid] Update p/invoke symbol names from net8 alpha1 (dotnet#7829)
* main:
  [monodroid] Properly process satellite assemblies (dotnet#7823)
  Bump to xamarin/java.interop/main@77800dda (dotnet#7824)
* main:
  [Xamarin.Android.Build.Tasks] `unable to open file as zip archive`? (dotnet#7759)
@jonpryor
Copy link
Member

jonpryor commented Mar 2, 2023

Context: https://github.com/xamarin/xamarin-android/issues/7230
Context: https://github.com/dotnet/runtime/issues/80935

When a WCF application invokes an endpoint which returns compressed
content, and `AndroidMessageHandler` is doing the network requests
([the default when `$(UseNativeHttpHandler)`=True][0]):

	var soapClient = new WebServiceSoapClient(WebServiceSoapClient.EndpointConfiguration.WebServiceSoap);
	//Async test
	var helloResponse = await soapClient.HelloWorldAsync();

then the method will throw:

	The formatter threw an exception while trying to deserialize the message: There was an error while trying to deserialize parameter http://tempuri.org/:HelloWorldResponse.
	---> There was an error deserializing the object of type ServiceReference1.HelloWorldResponseBody. Unexpected end of file. Following elements are not closed: HelloWorldResult, HelloWorldResponse, Body, Envelope. Line 1, position 298.
	   at System.Runtime.Serialization.XmlObjectSerializer.ReadObjectHandleExceptions(XmlReaderDelegator reader, Boolean verifyObjectName, DataContractResolver dataContractResolver)
	   at System.Runtime.Serialization.XmlObjectSerializer.ReadObjectHandleExceptions(XmlReaderDelegator reader, Boolean verifyObjectName)
	   at System.Runtime.Serialization.DataContractSerializer.ReadObject(XmlDictionaryReader reader, Boolean verifyObjectName)
	   at System.ServiceModel.Dispatcher.DataContractSerializerOperationFormatter.PartInfo.ReadObject(XmlDictionaryReader reader, XmlObjectSerializer serializer) in /_/src/System.Private.ServiceModel/src/System/ServiceModel/Dispatcher/DataContractSerializerOperationFormatter.cs:line 657
	   at System.ServiceModel.Dispatcher.DataContractSerializerOperationFormatter.PartInfo.ReadObject(XmlDictionaryReader reader) in /_/src/System.Private.ServiceModel/src/System/ServiceModel/Dispatcher/DataContractSerializerOperationFormatter.cs:line 652
	   at System.ServiceModel.Dispatcher.DataContractSerializerOperationFormatter.DeserializeParameterPart(XmlDictionaryReader reader, PartInfo part, Boolean isRequest) in /_/src/System.Private.ServiceModel/src/System/ServiceModel/Dispatcher/DataContractSerializerOperationFormatter.cs:line 521

The reason for this is that when `AndroidMessageHandler` creates a
wrapping decompression stream, it does not update `Content-Length` to
match the length of the decoded content, because it doesn't have a
way to know what the length is without first reading the stream to the
end, and that might prevent the end user to read the content.
(Additionally, I think the `Content-Length` header should reflect the
*original* content length, for the end user to be able to
interpret the response as it was sent.)

WCF, on the other hand, looks at the `Content-Length` header and, if
found, takes the value and reads only that many bytes from the content
stream and no more, which will almost always result in short reads and
failure to correctly interpret the response.

Workaround this issue by making `AndroidMessageHandler` behave the
same way as other handlers implemented in the BCL.  What they do in
this situation is remove the `Content-Length` header, making WCF
read the stream to the end.  Additionally, the clients remove the
compressed content encoding identifier from the `Content-Encoding`
header.

	var handler = new AndroidMessageHandler {
	    AutomaticDecompression = DecompressionMethods.All
	};
	var client    = new HttpClient (handler);
	var response  = await client.GetAsync ("https://httpbin.org/gzip");
	// response.Content.Headers won't contain Content-Length,
	// and response.Content.Headers.ContentEncoding won't contain `gzip`.

As a bonus, also adds support for decompression of responses
compressed with the `Brotli` compression which use the `br` encoding
ID in the `Content-Encoding` header.

[0]: https://learn.microsoft.com/en-us/dotnet/core/deploying/trimming/trimming-options?pivots=dotnet-7-0

@jonpryor jonpryor merged commit 5d46685 into dotnet:main Mar 2, 2023
@grendello grendello deleted the http-compression-fix branch March 2, 2023 14:26
grendello added a commit to grendello/xamarin-android that referenced this pull request Mar 6, 2023
* main:
  Bump to dotnet/installer@632ddca 8.0.100-preview.3.23128.1 (dotnet#7836)
  LEGO: Merge pull request 7852
  [ci] Reduce overhead for MSBuildIntegration unit test jobs. (dotnet#7832)
  [ci] Allow dynamic`$(NuGetArtifactName)` values (dotnet#7848)
  [Xamarin.Android.Build.Tasks] guard `AutoImport.props` against empty values (dotnet#7837)
  [Mono.Android] Print type & member remapping info (dotnet#7844)
  [Mono.Android] Tweak AndroidMessageHandler behavior for WCF support (dotnet#7785)
  LEGO: Merge pull request 7845
  Localized file check-in by OneLocBuild Task (dotnet#7842)
  [ci] Use compliance stage template (dotnet#7818)
  [build] pass `--skip-sign-check` to `dotnet workload` (dotnet#7840)
grendello added a commit to grendello/xamarin-android that referenced this pull request Mar 6, 2023
* main:
  Bump to dotnet/installer@632ddca 8.0.100-preview.3.23128.1 (dotnet#7836)
  LEGO: Merge pull request 7852
  [ci] Reduce overhead for MSBuildIntegration unit test jobs. (dotnet#7832)
  [ci] Allow dynamic`$(NuGetArtifactName)` values (dotnet#7848)
  [Xamarin.Android.Build.Tasks] guard `AutoImport.props` against empty values (dotnet#7837)
  [Mono.Android] Print type & member remapping info (dotnet#7844)
  [Mono.Android] Tweak AndroidMessageHandler behavior for WCF support (dotnet#7785)
  LEGO: Merge pull request 7845
  Localized file check-in by OneLocBuild Task (dotnet#7842)
  [ci] Use compliance stage template (dotnet#7818)
  [build] pass `--skip-sign-check` to `dotnet workload` (dotnet#7840)
  Replace K4os.Hash.xxHash with System.IO.Hashing (dotnet#7831)
  $(AndroidPackVersionSuffix)=preview.3; net8 is 34.0.0-preview.3 (dotnet#7839)
grendello added a commit to grendello/xamarin-android that referenced this pull request Mar 6, 2023
* main: (22 commits)
  Bump to dotnet/installer@632ddca 8.0.100-preview.3.23128.1 (dotnet#7836)
  LEGO: Merge pull request 7852
  [ci] Reduce overhead for MSBuildIntegration unit test jobs. (dotnet#7832)
  [ci] Allow dynamic`$(NuGetArtifactName)` values (dotnet#7848)
  [Xamarin.Android.Build.Tasks] guard `AutoImport.props` against empty values (dotnet#7837)
  [Mono.Android] Print type & member remapping info (dotnet#7844)
  [Mono.Android] Tweak AndroidMessageHandler behavior for WCF support (dotnet#7785)
  LEGO: Merge pull request 7845
  Localized file check-in by OneLocBuild Task (dotnet#7842)
  [ci] Use compliance stage template (dotnet#7818)
  [build] pass `--skip-sign-check` to `dotnet workload` (dotnet#7840)
  Replace K4os.Hash.xxHash with System.IO.Hashing (dotnet#7831)
  $(AndroidPackVersionSuffix)=preview.3; net8 is 34.0.0-preview.3 (dotnet#7839)
  [Xamarin.Android.Build.Tasks] Remove support for mkbundle (dotnet#7772)
  [Xamarin.Android.Build.Tasks] `unable to open file as zip archive`? (dotnet#7759)
  [monodroid] Properly process satellite assemblies (dotnet#7823)
  Bump to xamarin/java.interop/main@77800dda (dotnet#7824)
  [ci] Use AZDO built-in parallelization strategy. (dotnet#7804)
  Bump to dotnet/installer@e3ab0b5 8.0.100-preview.2.23123.10 (dotnet#7813)
  [ci] Run nunit tests with stable .NET version (dotnet#7826)
  ...
grendello added a commit to grendello/xamarin-android that referenced this pull request Mar 6, 2023
Context: dotnet#7785
Context: dotnet@7b2e172

7b2e172 added changes to retry the test up to 5 times should
https://httpbin.org fail the request (e.g. because of throttling).
However, I skipped building the changes locally believing that should
anything be wrong, that CI would catch it.  And it did, but the failures
weren't in the `[Tests]` tab but rather in a stage build log: the
`Mono.Android.NET-Tests.csproj` failed to build because the `[Retry]`
attribute is not available in `NUnitLite` which this test suite uses.
Additionally, `Assert.Warn` isn't supported either and the
`DoCompression` method should have been marked `async`.

Murphy must be laughing **really** hard now... :)

It should work now.
@github-actions github-actions bot locked and limited conversation to collaborators Jan 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants