HttpClient.SendAsync() should not attempt to read response body on a HEAD or PUT request. #38129

baulig · 2019-06-01T00:12:12Z

When using HttpClient.SendAsync() to issue a HEAD request, we shouldn't attempt to allocate a buffer for the return content regardless of the HttpCompletionOption (because ResponseContentRead is the default).

The problem is that even though the server won't send any response, the Content-Length could be quite large, so we would unnecessarily allocate a huge buffer. Furthermore, you even get an exception if the length is above 2 GB (HttpContent.MaxBufferSize is int.MaxValue).

We have a regression in Mono about this: mono/mono#14214.

I have investigated this and what's happening is that when using FinishSendAsyncBuffered(), the response.Content is an instance of HttpConnection.HttpConnectionResponseContent, so response.Content.LoadIntoBufferAsync() is called.

The then calls CreateMemoryStream(maxBufferSize, out error), the Headers.ContentLength is (in our example 2167849215) larger than int.MaxValue and it throws. But even it it were smaller, we'd still allocate a potentially huge buffer for no reason.

…HEAD or PUT request.

src/System.Net.Http/tests/FunctionalTests/HttpRequestMessageTest.cs

src/System.Net.Http/src/System/Net/Http/HttpMethod.cs

stephentoub · 2019-06-03T12:54:30Z

src/System.Net.Http/src/System/Net/Http/HttpClient.cs

+                return FinishSendAsyncUnbuffered(sendTask, request, cts, disposeCts);
+            }
+
            return completionOption == HttpCompletionOption.ResponseContentRead ?


Can we just add the above to this expression, after the completion options check? e.g.

return completionOptions == HttpCompletionOption.ResponseContentRead && request.Method.MayHaveResponseBody ? FinishSendAsyncBuffered(sendTask, request, cts, disposeCts) : FinishSendAsyncUnbuffered(sendTask, request, cts, disposeCts);

?

HEAD requests are relatively rare; we shouldn't need to pay for the extra dictionary lookup if completionOptions == ResponseHeadersRead.

Would the comparison still work without that Normalize() thing?

return completionOption == HttpCompletionOption.ResponseContentRead && !HttpMethod.Normalize(request.Method).NoResponseBody ? FinishSendAsyncBuffered(sendTask, request, cts, disposeCts) : FinishSendAsyncUnbuffered(sendTask, request, cts, disposeCts);

Would the comparison still work without that Normalize() thing?

It depends how it's implemented. Right now the code is calling Normalize (which does a dictionary lookup) and then does a reference comparison... you could instead just do the string comparison.

Okay, cleaned it up and removed the (now unused) property that I added.

src/System.Net.Http/tests/FunctionalTests/HttpRequestMessageTest.cs

geoffkizer · 2019-06-04T20:47:15Z

Re this:

Hmm. Yeah, HttpContent is largely separated from the method, so it doesn't really make sense to check that here. We could remove the optimization that preallocates the array, or we could add a try/catch to handle the OOM, but both of those are probably overkill. We can just leave out the assert I suggested.

This is a bit of a tangent, but I don't understand why LoadIntoBufferAsync doesn't use TryComputeLength to try to determine the content length, rather than reading the Content-Length header directly. If it did this, then we could provide an appropriate implementation of TryComputeLength for HEAD requests that just always returns 0.

Right now, we don't actually support TryComputeLength on HttpConnectionResponseContent at all. We just always return false. See:

corefx/src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionResponseContent.cs

Line 51 in 24aa561

protected internal sealed override bool TryComputeLength(out long length)

While this is perfectly legal, it seems like it would be more useful to actually return the known content length when possible, i.e. because we have a Content-Length header or because it's a HEAD request or a No Content response, etc.

I think @davidsh understands this better than I do...

stephentoub · 2019-06-04T20:51:12Z

@geoffkizer, your comments and question make sense... I unfortunately don't know the answer.

…HEAD or PUT request. (dotnet#38129) * HttpClient.SendAsync() should not attempt to read response body on a HEAD or PUT request. * Update HttpMethod.cs * Address feedback. * Cleanup. * Update HttpRequestMessageTest.cs (cherry picked from commit 73e610f)

HttpClient.SendAsync() should not attempt to read response body on a HEAD request. * CoreFX PR dotnet/corefx#38129. * Backported in mono/corefx#297. * Fixes mono#14214.

davidsh · 2019-06-05T21:08:36Z

This is a bit of a tangent, but I don't understand why LoadIntoBufferAsync doesn't use TryComputeLength to try to determine the content length, rather than reading the Content-Length header directly. If it did this, then we could provide an appropriate implementation of TryComputeLength for HEAD requests that just always returns 0.
While this is perfectly legal, it seems like it would be more useful to actually return the known content length when possible, i.e. because we have a Content-Length header or because it's a HEAD request or a No Content response, etc.
I think @davidsh understands this better than I do...

The reading of the Content-Length header implicitly will call TryComputeLength() if the Content-Length header is null. That is part of the original design of how HttpContent uses a lazy-initialized Content-Length calculation which is especially useful for stream-based content (not as useful for memory based like StringContent, etc.).

LoadIntoBufferAsync() calls GetComputedOrBufferLength() via accessing the 'HttpContentHeaders.ContentLength' property.

corefx/src/System.Net.Http/src/System/Net/Http/Headers/HttpContentHeaders.cs

Lines 69 to 106 in c63e5eb

    
           public long? ContentLength 
        
           { 
        
               get 
        
               { 
        
                   // 'Content-Length' can only hold one value. So either we get 'null' back or a boxed long value. 
        
                   object storedValue = GetParsedValues(KnownHeaders.ContentLength.Descriptor); 
        
                   // Only try to calculate the length if the user didn't set the value explicitly using the setter. 
        
                   if (!_contentLengthSet && (storedValue == null)) 
        
                   { 
        
                       // If we don't have a value for Content-Length in the store, try to let the content calculate 
        
                       // it's length. If the content object is able to calculate the length, we'll store it in the 
        
                       // store. 
        
                       long? calculatedLength = _parent.GetComputedOrBufferLength(); 
        
                       if (calculatedLength != null) 
        
                       { 
        
                           SetParsedValue(KnownHeaders.ContentLength.Descriptor, (object)calculatedLength.Value); 
        
                       } 
        
                       return calculatedLength; 
        
                   } 
        
                   if (storedValue == null) 
        
                   { 
        
                       return null; 
        
                   } 
        
                   else 
        
                   { 
        
                       return (long)storedValue; 
        
                   } 
        
               } 
        
               set 
        
               { 
        
                   SetOrRemoveParsedValue(KnownHeaders.ContentLength.Descriptor, value); // box long value 
        
                   _contentLengthSet = true; 
        
               } 
        
           }

which calls TryComputeLength():

corefx/src/System.Net.Http/src/System/Net/Http/HttpContent.cs

Lines 469 to 493 in c63e5eb

    
           internal long? GetComputedOrBufferLength() 
        
           { 
        
               CheckDisposed(); 
        
               if (IsBuffered) 
        
               { 
        
                   return _bufferedContent.Length; 
        
               } 
        
               // If we already tried to calculate the length, but the derived class returned 'false', then don't try 
        
               // again; just return null. 
        
               if (_canCalculateLength) 
        
               { 
        
                   long length = 0; 
        
                   if (TryComputeLength(out length)) 
        
                   { 
        
                       return length; 
        
                   } 
        
                   // Set flag to make sure next time we don't try to compute the length, since we know that we're unable 
        
                   // to do so. 
        
                   _canCalculateLength = false; 
        
               } 
        
               return null; 
        
           }

HttpConnectionResponseContent currently doesn't implement TryComputeLength(). Conceptually, it can't because it is a response stream which could have arbitrary length despite the promise of a 'Content-Length' header. And chunked responses would result in TryComputeLength() being unable to know ahead of time the real length, of course.

You might choose to change HttpConnectionResponseContent.TryComputeLength() to optimize in the case of a 'Content-Length' header being part of the http response headers returned by the server. Not sure if this changes the end result for optimization.

baulig · 2019-06-05T21:38:19Z

The problem that I have with this is that HttpContent does not know whether it came from a HEAD request. Attempting to actually read the content (after a HEAD request and HttpCompletionOptions.ResponseContentRead will result in a timeout / exception being thrown - rather than returning a zero-length array.
We should probably address this in a separate PR.

HttpClient.SendAsync() should not attempt to read response body on a HEAD request. * CoreFX PR dotnet/corefx#38129. * Backported in mono/corefx#297. * Fixes #14214.

HttpClient.SendAsync() should not attempt to read response body on a HEAD request. * CoreFX PR dotnet/corefx#38129. * Backported in mono/corefx#297. * Fixes mono#14214.

HttpClient.SendAsync() should not attempt to read response body on a HEAD request. * CoreFX PR dotnet/corefx#38129. * Backported in mono/corefx#297. * Fixes mono#14214. (cherry picked from commit 2b3a23c)

HttpClient.SendAsync() should not attempt to read response body on a HEAD request. * CoreFX PR dotnet/corefx#38129. * Backported in mono/corefx#297. * Fixes #14214. (cherry picked from commit 2b3a23c)

HttpClient.SendAsync() should not attempt to read response body on a HEAD request. * CoreFX PR dotnet/corefx#38129. * Backported in mono/corefx#297. * Fixes #14214.

…HEAD or PUT request. (dotnet/corefx#38129) * HttpClient.SendAsync() should not attempt to read response body on a HEAD or PUT request. * Update HttpMethod.cs * Address feedback. * Cleanup. * Update HttpRequestMessageTest.cs Commit migrated from dotnet/corefx@73e610f

HttpClient.SendAsync() should not attempt to read response body on a …

6eee788

…HEAD or PUT request.

baulig mentioned this pull request Jun 1, 2019

HttpClient.SendAsync() with > 2 GB response body. mono/mono#14214

Closed

davidsh requested review from a team and stephentoub June 1, 2019 00:56

davidsh added the area-System.Net.Http label Jun 1, 2019

davidsh reviewed Jun 1, 2019

View reviewed changes

src/System.Net.Http/tests/FunctionalTests/HttpRequestMessageTest.cs Show resolved Hide resolved

davidsh added this to the 3.0 milestone Jun 1, 2019

stephentoub reviewed Jun 1, 2019

View reviewed changes

src/System.Net.Http/src/System/Net/Http/HttpMethod.cs Outdated Show resolved Hide resolved

Update HttpMethod.cs

c122d43

stephentoub reviewed Jun 3, 2019

View reviewed changes

src/System.Net.Http/tests/FunctionalTests/HttpRequestMessageTest.cs Outdated Show resolved Hide resolved

stephentoub reviewed Jun 3, 2019

View reviewed changes

src/System.Net.Http/tests/FunctionalTests/HttpRequestMessageTest.cs Show resolved Hide resolved

Martin Baulig added 2 commits June 3, 2019 11:25

Address feedback.

c4ce787

Cleanup.

4c196b5

stephentoub reviewed Jun 3, 2019

View reviewed changes

src/System.Net.Http/tests/FunctionalTests/HttpRequestMessageTest.cs Outdated Show resolved Hide resolved

Update HttpRequestMessageTest.cs

deb635b

stephentoub merged commit 73e610f into dotnet:master Jun 4, 2019

baulig mentioned this pull request Jun 5, 2019

HttpClient.SendAsync() should not attempt to read response body on a HEAD request. mono/corefx#297

Merged

baulig deleted the work-head-large-content branch June 5, 2019 18:07

baulig mentioned this pull request Jun 5, 2019

Bump CoreFX to pickup the fix for #14214. mono/mono#14821

Merged

monojenkins mentioned this pull request Jun 5, 2019

[mono-2019-02] HttpClient.SendAsync() should not attempt to read response body on a HEAD request. mono/corefx#299

Merged

monojenkins mentioned this pull request Jun 6, 2019

[2019-06] Bump CoreFX to pickup the fix for #14214. mono/mono#14832

Merged

BrennanConroy mentioned this pull request Jun 7, 2019

[master] Update dependencies from 3 repositories dotnet/aspnetcore#10939

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

HttpClient.SendAsync() should not attempt to read response body on a HEAD or PUT request. #38129

HttpClient.SendAsync() should not attempt to read response body on a HEAD or PUT request. #38129

Uh oh!

baulig commented Jun 1, 2019

Uh oh!

Uh oh!

Uh oh!

stephentoub Jun 3, 2019

Uh oh!

baulig Jun 3, 2019

Uh oh!

stephentoub Jun 3, 2019 •

edited

Loading

Uh oh!

baulig Jun 3, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

geoffkizer commented Jun 4, 2019

Uh oh!

stephentoub commented Jun 4, 2019

Uh oh!

davidsh commented Jun 5, 2019 •

edited

Loading

Uh oh!

baulig commented Jun 5, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

HttpClient.SendAsync() should not attempt to read response body on a HEAD or PUT request. #38129

HttpClient.SendAsync() should not attempt to read response body on a HEAD or PUT request. #38129

Uh oh!

Conversation

baulig commented Jun 1, 2019

Uh oh!

Uh oh!

Uh oh!

stephentoub Jun 3, 2019

Choose a reason for hiding this comment

Uh oh!

baulig Jun 3, 2019

Choose a reason for hiding this comment

Uh oh!

stephentoub Jun 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

baulig Jun 3, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

geoffkizer commented Jun 4, 2019

Uh oh!

stephentoub commented Jun 4, 2019

Uh oh!

davidsh commented Jun 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

baulig commented Jun 5, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

stephentoub Jun 3, 2019 •

edited

Loading

davidsh commented Jun 5, 2019 •

edited

Loading