HTTP Compression - Improve/fix support for user-specified Accept-Encoding encodings by EatonZ · Pull Request #47000 · dotnet/runtime

EatonZ · 2021-01-14T18:28:02Z

geoffkizer · 2021-01-14T18:35:10Z

If you're using LINQ methods, then Any seems like a better choice than FirstOrDefault.

That said, LINQ is going to allocate IEnumerable instances, so we generally try to avoid it to minimize allocation. You could just add a helper method that does a foreach and then call it in each case.

geoffkizer · 2021-01-14T18:37:28Z

BTW are the encoding names case-insensitive? Previously we were doing a case-sensitive check (I assume that's what the Contains call is doing...).

geoffkizer · 2021-01-14T18:43:01Z

Yeah, they are case-insensitive: https://tools.ietf.org/html/rfc2616#section-3.5

geoffkizer · 2021-01-14T18:43:09Z

We need tests for this.

geoffkizer · 2021-01-14T18:44:01Z

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/DecompressionHandler.cs

        internal override async ValueTask<HttpResponseMessage> SendAsync(HttpRequestMessage request, bool async, CancellationToken cancellationToken)
        {
-            if (GZipEnabled && !request.Headers.AcceptEncoding.Contains(s_gzipHeaderValue))
+            if (GZipEnabled && request.Headers.AcceptEncoding.FirstOrDefault(encoding => string.Compare(encoding.Value, s_gzipHeaderValue.Value, StringComparison.OrdinalIgnoreCase) == 0) == null)


I think string.Equal is better than string,Compare, since only care about equality

geoffkizer · 2021-01-14T18:45:03Z

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/DecompressionHandler.cs

        internal override async ValueTask<HttpResponseMessage> SendAsync(HttpRequestMessage request, bool async, CancellationToken cancellationToken)
        {
-            if (GZipEnabled && !request.Headers.AcceptEncoding.Contains(s_gzipHeaderValue))
+            if (GZipEnabled && request.Headers.AcceptEncoding.FirstOrDefault(encoding => string.Compare(encoding.Value, s_gzipHeaderValue.Value, StringComparison.OrdinalIgnoreCase) == 0) == null)


Rather than s_gzipHeaderValue.Value you can just use const string Gzip defined above.

stephentoub · 2021-01-14T19:03:48Z

If you're using LINQ methods, then Any seems like a better choice than FirstOrDefault. That said, LINQ is going to allocate IEnumerable instances, so we generally try to avoid it to minimize allocation. You could just add a helper method that does a foreach and then call it in each case.

Yeah, we should not be using LINQ here. I'd be a little surprised if it even compiles; we try to avoid even referencing System.Linq.dll in assemblies like this.

geoffkizer · 2021-01-14T19:05:32Z

I'd be a little surprised if it even compiles

It does not. https://github.com/dotnet/runtime/pull/47000/checks?check_run_id=1703936268

EatonZ · 2021-01-14T19:05:43Z

I will update to remove LINQ.

ghost · 2021-01-14T19:47:00Z

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #46948

I replaced the Contains check with a FirstOrDefault implementation. DecompressionHandler will now check each AcceptEncoding value to ensure it does not exist before adding its own. StringComparison.OrdinalIgnoreCase is used for comparing in the event users specify mixed-case encodings.
I believe this is a sufficient fix. If there are any other areas of concern I may have missed, let me know,

cc @geoffkizer @karelz

Author:	EatonZ
Assignees:	-
Labels:	`area-System.Net.Http`
Milestone:	-

…risons

dnfadmin · 2021-01-14T19:49:11Z

All CLA requirements met.

EatonZ · 2021-01-14T19:50:02Z

Updated based on feedback.

geoffkizer · 2021-01-14T19:52:07Z

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/DecompressionHandler.cs

+        {
+            foreach (var existingEncoding in acceptEncodingHeader)
+            {
+                if (string.Equals(existingEncoding.Value, encoding, StringComparison.OrdinalIgnoreCase)) return true;


return true should be on a separate line and enclosed by braces

geoffkizer · 2021-01-14T19:53:08Z

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/DecompressionHandler.cs

        internal bool DeflateEnabled => (_decompressionMethods & DecompressionMethods.Deflate) != 0;
        internal bool BrotliEnabled => (_decompressionMethods & DecompressionMethods.Brotli) != 0;
+
+        private bool EncodingExists(HttpHeaderValueCollection<StringWithQualityHeaderValue> acceptEncodingHeader, string encoding)


This can be static

geoffkizer · 2021-01-14T19:53:30Z

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/DecompressionHandler.cs

+
+        private bool EncodingExists(HttpHeaderValueCollection<StringWithQualityHeaderValue> acceptEncodingHeader, string encoding)
+        {
+            foreach (var existingEncoding in acceptEncodingHeader)


Don't use var; use the full type name

stephentoub · 2021-01-14T19:52:30Z

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/DecompressionHandler.cs

+        {
+            foreach (var existingEncoding in acceptEncodingHeader)
+            {
+                if (string.Equals(existingEncoding.Value, encoding, StringComparison.OrdinalIgnoreCase)) return true;


Nit:

Suggested change

if (string.Equals(existingEncoding.Value, encoding, StringComparison.OrdinalIgnoreCase)) return true;

if (string.Equals(existingEncoding.Value, encoding, StringComparison.OrdinalIgnoreCase))

{

return true;

}

stephentoub · 2021-01-14T19:52:54Z

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/DecompressionHandler.cs

+
+        private bool EncodingExists(HttpHeaderValueCollection<StringWithQualityHeaderValue> acceptEncodingHeader, string encoding)
+        {
+            foreach (var existingEncoding in acceptEncodingHeader)


Nit:

Suggested change

foreach (var existingEncoding in acceptEncodingHeader)

foreach (StringWithQualityHeaderValue existingEncoding in acceptEncodingHeader)

stephentoub · 2021-01-14T19:55:36Z

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/DecompressionHandler.cs

        internal bool DeflateEnabled => (_decompressionMethods & DecompressionMethods.Deflate) != 0;
        internal bool BrotliEnabled => (_decompressionMethods & DecompressionMethods.Brotli) != 0;
+
+        private bool EncodingExists(HttpHeaderValueCollection<StringWithQualityHeaderValue> acceptEncodingHeader, string encoding)


Suggested change

private bool EncodingExists(HttpHeaderValueCollection<StringWithQualityHeaderValue> acceptEncodingHeader, string encoding)

private static bool EncodingExists(HttpHeaderValueCollection<StringWithQualityHeaderValue> acceptEncodingHeader, string encoding)

stephentoub · 2021-01-14T19:59:32Z

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/DecompressionHandler.cs

        internal bool DeflateEnabled => (_decompressionMethods & DecompressionMethods.Deflate) != 0;
        internal bool BrotliEnabled => (_decompressionMethods & DecompressionMethods.Brotli) != 0;
+
+        private bool EncodingExists(HttpHeaderValueCollection<StringWithQualityHeaderValue> acceptEncodingHeader, string encoding)


I don't know what, if anything, can be done about it in this PR, but it looks like this is going to allocate more than we do today, e.g. an enumerator for the header collection. It's also going to force a StringWithQualityHeaderValue to be materialized for every entry in the collection, though I don't know if that was happening already.

Would it be more expensive than the existing Contains check?
One thing I thought about was changing it to check all encodings in 1 loop instead of 3 loops. I didn't want to stray too far from the existing implementation, though.

it looks like this is going to allocate more than we do today, e.g. an enumerator for the header collection

Presumably we could fix this in HttpHeaderValueCollection<T> by adding a struct enumerator, right?

Also, the common case here is that this header isn't set at all, so we could at least optimize for the case where there are no header values and return a singleton empty enumerator in that case.

It's also going to force a StringWithQualityHeaderValue to be materialized for every entry in the collection, though I don't know if that was happening already.

Probably happening already. Definitely happening if you set the header value by constructing a StringWithQualityHeaderValue. But likely also happening anyway, even if you set the value as a string, because we always force these values to get parsed eventually before send them (currently, that is).

Also, the common case here is that this header isn't set at all, so we could at least optimize for the case where there are no header values and return a singleton empty enumerator in that case.

Ok, if that is indeed the common case, I'll go ahead and do that in a PR in parallel to this one. I don't want to block this PR on adding new API.

geoffkizer · 2021-01-14T20:14:59Z

Existing tests are here: https://github.com/dotnet/runtime/blob/master/src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.Decompression.cs#L264

We should be able to add some similar cases to cover this new behavior.

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/DecompressionHandler.cs

Co-authored-by: Stephen Toub <stoub@microsoft.com>

stephentoub

Thanks for the fix. The product change LGTM. But tests would be good, as Geoff noted.

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/DecompressionHandler.cs

Co-authored-by: Stephen Toub <stoub@microsoft.com>

EatonZ · 2021-01-15T17:26:27Z

I'll look at getting a test in soon.

EatonZ · 2021-01-24T18:32:04Z

I need some pointers on getting these tests to run.
For example, if I run this in cmd:

cd src\libraries\Common\tests
dotnet build /t:Test /p:XunitMethodName=System.Net.Http.Functional.Tests.HttpClientHandler_Decompression_Test.GetAsync_SetAutomaticDecompression_AcceptEncodingHeaderSentWithNoDuplicates

It says: Discovered: Common.Tests (found 0 of 241 test cases)

Please let me know what I might be missing.

stephentoub · 2021-01-24T18:38:31Z

The class you're specifying is an abstract base class. If you want to run a specific test and so give xunit a test name, it needs to include the actual class name, e.g. SocketsHttpHandler_HttpClientHandler_Decompression_Tests.

stephentoub · 2021-01-24T18:39:50Z

Also, you need to be in the System.Net.Http functional tests directory to run the HTTP functional tests.

EatonZ · 2021-01-24T21:55:42Z

Thank you.

Test added.

geoffkizer · 2021-01-25T16:24:51Z

src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.Decompression.cs

+#if NETCOREAPP
+        [InlineData(DecompressionMethods.GZip | DecompressionMethods.Deflate | DecompressionMethods.Brotli, "gzip; q=1.0, deflate; q=1.0, br; q=1.0")]
+#endif
+        [InlineData(DecompressionMethods.GZip | DecompressionMethods.Deflate, "gzip; q=1.0, deflate; q=1.0")]


We should add a case here where the decompression handler will add at least one new header value, but not overwrite an existing one (or ones).

e.g. something like [InlineData(DecompressionMethods.GZip | DecompressionMethods.Deflate | DecompressionMethods.Brotli, "deflate; q=1.0")]

and check that the result is "deflate; q=1.0, gzip, brotli".

geoffkizer · 2021-01-25T20:27:34Z

The test is failing for WinHttpHandler. We should just check for IsWinHttpHandler (as some other tests do) and return without doing the test.

EatonZ · 2021-01-25T20:38:32Z

@geoffkizer Ok, so just remove the "br" check?

geoffkizer · 2021-01-25T21:02:41Z

No, just do if (IsWinHttpHandler) { return; } at the beginning of the test.

If you check the test output, you'll see that WinHttpHandler is not handling these cases correctly. It either needs to be fixed (as we've done with SocketsHttpHandler in this PR) or the tests need to be disabled for WinHttpHandler. I don't think it's crucial to fix WinHttpHandler, and it may not be straightforward to do, so let's just do the latter.

EatonZ · 2021-01-25T21:07:11Z

@geoffkizer Yup - that is what I meant, just remove the "br" check from the if.

Done

geoffkizer · 2021-01-25T21:19:33Z

Ah, I understand now, thanks.

Changes look good. If CI results look good I will merge.

Thanks for the contribution!

geoffkizer · 2021-01-25T21:21:22Z

/azp run runtime-libraries-coreclr outerloop

azure-pipelines · 2021-01-25T21:21:35Z

Azure Pipelines successfully started running 1 pipeline(s).

EatonZ · 2021-01-25T21:21:49Z

Happy to help!

Could this be considered for .NET 5.0.3? .NET 6 is rather far away, and there is no workaround to stop duplicates from being added.

geoffkizer · 2021-01-25T21:23:09Z

Ask @karelz

EatonZ · 2021-01-26T18:57:59Z

@karelz I guess that's a no?😥

karelz · 2021-01-26T19:57:10Z

@EatonZ sorry I missed the question here. Let's move the discussion to the issue.
We would need to have good business case - how often does it hurt? How much does it hurt your service? (anything larger?) ... We can take details offline if needed.

Implement fix in DecompressionHandler

0333740

EatonZ mentioned this pull request Jan 14, 2021

HTTP Compression - Improve/fix support for user-specified Accept-Encoding encodings #46948

Closed

geoffkizer reviewed Jan 14, 2021

View reviewed changes

ghost added the area-System.Net.Http label Jan 14, 2021

Removed LINQ in favor of helper method, use constant values for compa…

9305c56

…risons

geoffkizer reviewed Jan 14, 2021

View reviewed changes

stephentoub reviewed Jan 14, 2021

View reviewed changes

Touch up helper method

a2dfbf1

stephentoub reviewed Jan 14, 2021

View reviewed changes

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/DecompressionHandler.cs Outdated Show resolved Hide resolved

Remove extra spacing

dcef015

Co-authored-by: Stephen Toub <stoub@microsoft.com>

stephentoub reviewed Jan 15, 2021

View reviewed changes

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/DecompressionHandler.cs Show resolved Hide resolved

Add line

13caf4e

Co-authored-by: Stephen Toub <stoub@microsoft.com>

Add test

bf25325

geoffkizer reviewed Jan 25, 2021

View reviewed changes

Added test case for handler-added encoding values

b8953cf

Fix failing test

956a260

geoffkizer approved these changes Jan 26, 2021

View reviewed changes

geoffkizer merged commit abcf07b into dotnet:master Jan 26, 2021

karelz added this to the 6.0.0 milestone Jan 26, 2021

ghost locked as resolved and limited conversation to collaborators Feb 25, 2021

	foreach (var existingEncoding in acceptEncodingHeader)
	foreach (StringWithQualityHeaderValue existingEncoding in acceptEncodingHeader)

	private bool EncodingExists(HttpHeaderValueCollection<StringWithQualityHeaderValue> acceptEncodingHeader, string encoding)
	private static bool EncodingExists(HttpHeaderValueCollection<StringWithQualityHeaderValue> acceptEncodingHeader, string encoding)

Conversation

EatonZ commented Jan 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

geoffkizer commented Jan 14, 2021

Uh oh!

geoffkizer commented Jan 14, 2021

Uh oh!

geoffkizer commented Jan 14, 2021

Uh oh!

geoffkizer commented Jan 14, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephentoub commented Jan 14, 2021

Uh oh!

geoffkizer commented Jan 14, 2021

Uh oh!

EatonZ commented Jan 14, 2021

Uh oh!

ghost commented Jan 14, 2021

Uh oh!

dnfadmin commented Jan 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

EatonZ commented Jan 14, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geoffkizer commented Jan 14, 2021

Uh oh!

Uh oh!

stephentoub left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

EatonZ commented Jan 15, 2021

Uh oh!

EatonZ commented Jan 24, 2021

Uh oh!

stephentoub commented Jan 24, 2021

Uh oh!

stephentoub commented Jan 24, 2021

Uh oh!

EatonZ commented Jan 24, 2021

Uh oh!

EatonZ commented Jan 14, 2021 •

edited

Loading

dnfadmin commented Jan 14, 2021 •

edited

Loading