Implementations of new HttpContent sync methods. #38635

ManickaP · 2020-06-30T18:58:11Z

Adds missing overrides of sync SerializeToStream and CreateContentReadStream to other HttpContent implementations in libraries.

Fixes #37477

cc: @jozkee @GrabYourPitchforks @stephentoub @scalablecory

Cleanup after move to WinHttpHandler, removed unused compiler constant.

ghost · 2020-06-30T18:58:16Z

Tagging subscribers to this area: @dotnet/ncl
Notify danmosemsft if you want to be subscribed.

src/libraries/System.Net.Http.WinHttpHandler/src/System.Net.Http.WinHttpHandler.csproj

src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/NoWriteNoSeekStreamContent.cs

src/libraries/System.Net.Http.Json/src/System/Net/Http/Json/JsonContent.cs

src/libraries/System.Net.Http.Json/src/Resources/Strings.resx

src/libraries/System.Net.Http.Json/src/System/Net/Http/Json/JsonContent.cs

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

src/libraries/System.Net.Http.Json/src/System/Net/Http/Json/JsonContent.cs

…sts.

ManickaP · 2020-07-01T21:06:41Z

/azp run runtime-libraries outerloop

azure-pipelines · 2020-07-01T21:06:54Z

Azure Pipelines successfully started running 1 pipeline(s).

jozkee

Changes to JsonContent LGTM.

src/libraries/System.Net.Http.Json/tests/FunctionalTests/TestClasses.cs

src/libraries/System.Net.Http.Json/src/System/Net/Http/Json/JsonContent.cs

src/libraries/System.Net.Http.Json/tests/FunctionalTests/JsonContentTests.netcoreapp.cs

ManickaP · 2020-07-02T12:15:55Z

src/libraries/System.Net.Http/ref/System.Net.Http.cs

@@ -11,6 +11,7 @@ public partial class ByteArrayContent : System.Net.Http.HttpContent
    {
        public ByteArrayContent(byte[] content) { }
        public ByteArrayContent(byte[] content, int offset, int count) { }
+        protected override System.IO.Stream CreateContentReadStream(System.Threading.CancellationToken cancellationToken) { throw null; }


Unfortunately, this was not regenerated in #37494

ManickaP · 2020-07-02T12:18:49Z

WinHttpHandler changes reverted, regenerated ref sources, added/enables more tests.

@stephentoub It's ready for re-review.

scalablecory

lgtm after question re: CreateRequest is addressed.

scalablecory · 2020-07-09T01:50:21Z

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

-            using (HttpResponseMessage response = await client.GetAsync(uri, HttpCompletionOption.ResponseHeadersRead))
+            using (HttpResponseMessage response = await client.SendAsync(TestAsync, CreateRequest(HttpMethod.Get, uri, remoteServer.HttpVersion), HttpCompletionOption.ResponseHeadersRead))


What is this accomplishing? Shouldn't the client's DefaultRequestVersion be the same as remoteServer.HttpVersion here? HttpClient does essentially the same thing as this CreateRequest when you call GetAsync.

SendAsync|Send ignores HttpClient.DefaultRequestVersion since it accepts fully constructed request. The default initialization of HttpRequestMessage will set it to 1.1. And because we plug in VersionChecker set up with the remote server version, it will blow up for 2.0 servers.

If it's about why Send versus Get, then it's because we weren't testing decompression with sync code paths at all due to using GetAsync (helpers didn't get sync overloads). And we had an error in missing overrides for decompression content 😞 So I changed the tests to use SendAsync|Send instead and now we're testing both, sync and async.

src/libraries/System.Net.Http.Json/src/System/Net/Http/Json/JsonContent.cs

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

src/libraries/System.Net.Http.Json/tests/FunctionalTests/JsonContentTests.netcoreapp.cs

src/libraries/System.Net.Http.Json/tests/FunctionalTests/TestClasses.cs

...es/System.Utf8String.Experimental/tests/System/Net/Http/Utf8StringContentTests.netcoreapp.cs

stephentoub

The product changes LGTM. I didn't review the tests.

scalablecory

lgtm

ManickaP added 3 commits June 30, 2020 15:57

WinHttpHandler NoWriteNoSeekStreamContent sync API.

196cebe

Cleanup after move to WinHttpHandler, removed unused compiler constant.

JsonContent and DecompressionHandlerContent sync.

7fa4d49

Utf8StringContent sync

4635ab6

ManickaP requested a review from a team June 30, 2020 18:58

Dotnet-GitSync-Bot added the area-System.Net.Http label Jun 30, 2020

ManickaP force-pushed the mapichov/37477_sync_http_contents branch from 4b27d68 to 8a68107 Compare June 30, 2020 19:03

ManickaP commented Jun 30, 2020

View reviewed changes

src/libraries/System.Net.Http.WinHttpHandler/src/System.Net.Http.WinHttpHandler.csproj Outdated Show resolved Hide resolved

ManickaP commented Jun 30, 2020

View reviewed changes

src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/NoWriteNoSeekStreamContent.cs Outdated Show resolved Hide resolved

net fw compilaris

7ff4989

ManickaP force-pushed the mapichov/37477_sync_http_contents branch from 8a68107 to 7ff4989 Compare June 30, 2020 19:10

jozkee reviewed Jul 1, 2020

View reviewed changes

stephentoub reviewed Jul 1, 2020

View reviewed changes

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

stephentoub reviewed Jul 1, 2020

View reviewed changes

src/libraries/System.Net.Http.Json/src/System/Net/Http/Json/JsonContent.cs Show resolved Hide resolved

ManickaP added 2 commits July 1, 2020 11:16

Reverted WinHttpHandler changes.

775664f

Decompression tests for sync send.

14eae27

ManickaP force-pushed the mapichov/37477_sync_http_contents branch from 98a46f2 to 14eae27 Compare July 1, 2020 11:26

JsonContent PR feedback.

6e90249

ManickaP force-pushed the mapichov/37477_sync_http_contents branch from ce19b45 to 6e90249 Compare July 1, 2020 12:38

Updated ref sources for HttpContents.

aa915d5

ManickaP force-pushed the mapichov/37477_sync_http_contents branch from 7de7713 to 5c20f40 Compare July 1, 2020 14:20

Fixed test compilation for net fw, added and improved json content te…

4505b53

…sts.

ManickaP force-pushed the mapichov/37477_sync_http_contents branch from 5c20f40 to 4505b53 Compare July 1, 2020 14:23

ManickaP added 2 commits July 1, 2020 16:31

Utf8 tests added and fixed.

96f6ac0

net fx compilation

bc77fbb

ManickaP force-pushed the mapichov/37477_sync_http_contents branch from a52ca4a to bc77fbb Compare July 1, 2020 16:23

jozkee reviewed Jul 1, 2020

View reviewed changes

PR feedback.

6d34cfc

ManickaP force-pushed the mapichov/37477_sync_http_contents branch from ad11c9a to 6d34cfc Compare July 2, 2020 08:55

whitespaces and vars.

7cb03ab

ManickaP commented Jul 2, 2020

View reviewed changes

jaredpar mentioned this pull request Jul 2, 2020

OSX machines are de-provisioned during CI / PR runs leading to failures #34472

Closed

This was referenced Jul 2, 2020

Sync HTTP Client #38720

Closed

Rewrite System.Net.Http.Json functional tests to use a custom HttpMessageHandler #38733

Merged

Merge branch 'master' into mapichov/37477_sync_http_contents

d927afc

scalablecory reviewed Jul 9, 2020

View reviewed changes

Addressed PR feedback.

01be142

stephentoub approved these changes Jul 10, 2020

View reviewed changes

scalablecory approved these changes Jul 10, 2020

View reviewed changes

ManickaP merged commit 149aa1e into dotnet:master Jul 10, 2020

ManickaP deleted the mapichov/37477_sync_http_contents branch July 10, 2020 06:52

karelz added this to the 5.0.0 milestone Aug 18, 2020

ghost locked as resolved and limited conversation to collaborators Dec 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementations of new HttpContent sync methods. #38635

Implementations of new HttpContent sync methods. #38635

ManickaP commented Jun 30, 2020

ghost commented Jun 30, 2020

ManickaP commented Jul 1, 2020

azure-pipelines bot commented Jul 1, 2020

jozkee left a comment

ManickaP Jul 2, 2020 •

edited

Loading

ManickaP commented Jul 2, 2020

scalablecory left a comment

scalablecory Jul 9, 2020

ManickaP Jul 9, 2020 •

edited

Loading

stephentoub left a comment

scalablecory left a comment

		using (HttpResponseMessage response = await client.GetAsync(uri, HttpCompletionOption.ResponseHeadersRead))
		using (HttpResponseMessage response = await client.SendAsync(TestAsync, CreateRequest(HttpMethod.Get, uri, remoteServer.HttpVersion), HttpCompletionOption.ResponseHeadersRead))

Implementations of new HttpContent sync methods. #38635

Implementations of new HttpContent sync methods. #38635

Conversation

ManickaP commented Jun 30, 2020

ghost commented Jun 30, 2020

ManickaP commented Jul 1, 2020

azure-pipelines bot commented Jul 1, 2020

jozkee left a comment

Choose a reason for hiding this comment

ManickaP Jul 2, 2020 • edited Loading

Choose a reason for hiding this comment

ManickaP commented Jul 2, 2020

scalablecory left a comment

Choose a reason for hiding this comment

scalablecory Jul 9, 2020

Choose a reason for hiding this comment

ManickaP Jul 9, 2020 • edited Loading

Choose a reason for hiding this comment

stephentoub left a comment

Choose a reason for hiding this comment

scalablecory left a comment

Choose a reason for hiding this comment

ManickaP Jul 2, 2020 •

edited

Loading

ManickaP Jul 9, 2020 •

edited

Loading