Convert PictureBox WebRequest methods to HttpClient #1952

neldridg · 2019-09-22T00:14:33Z

Fixes #1756
This change follows the suggestion from #1756 to change the WebRequest methods in PictureBox.cs to HttpClient.

Proposed changes

Move WebRequest methods to asynchronous HttpClient Get methods.

Customer Impact

Low/None. Customer should not see any visible changes

Regression?

Yes

Risk

Low

Test methodology

Ran unit tests.
Validated tests were accurate after update (no changes were needed)

Test environment(s)

.NET Core SDK (reflecting any global.json):
 Version:   5.0.100-alpha1-014723
 Commit:    5504e0070a

Runtime Environment:
 OS Name:     Windows
 OS Version:  10.0.18362
 OS Platform: Windows
 RID:         win10-x64
 Base Path:   C:\Program Files\dotnet\sdk\5.0.100-alpha1-014723\

Host (useful for support):
  Version: 5.0.0-alpha1.19470.7
  Commit:  c25b8b41ec

.NET Core SDKs installed:
  5.0.100-alpha1-014723 [C:\Program Files\dotnet\sdk]

.NET Core runtimes installed:

  Microsoft.AspNetCore.App 5.0.0-alpha1.19470.6 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.NETCore.App 5.0.0-alpha1.19470.7 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.WindowsDesktop.App 5.0.0-alpha1.19470.7 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]

The move towards requires the GetResponseCallback method to be made async to deal with the requirement of HttpClient to use async methods.
Default behavior of HttpClient is to cache all results. No changes were made to headers as this covers the task.

Microsoft Reviewers: Open in CodeFlow

dnfclas · 2019-09-22T00:14:48Z

All CLA requirements met.

codecov · 2019-09-22T00:23:33Z

Codecov Report

Merging #1952 into master will increase coverage by 0.00489%.
The diff coverage is 0%.

@@                Coverage Diff                 @@
##              master       #1952        +/-   ##
==================================================
+ Coverage   26.93408%   26.93898%   +0.0049%     
==================================================
  Files            864         864                
  Lines         266755      266751         -4     
  Branches       37915       37917         +2     
==================================================
+ Hits           71848       71860        +12     
+ Misses        189788      189765        -23     
- Partials        5119        5126         +7

Flag	Coverage Δ
#Debug	`26.93898% <0%> (+0.00489%)`	⬆️
#production	`26.93898% <0%> (+0.00489%)`	⬆️
#test	`100% <ø> (ø)`	⬆️

weltkante · 2019-09-22T06:44:48Z

src/System.Windows.Forms/src/System/Windows/Forms/PictureBox.cs

-            });
+            using HttpClient client = new HttpClient();
+
+            client.GetAsync(CalculateUri(_imageLocation))


The calling site should probably get a try/catch handler since the exception patterns likely have changed between what BeginGetResponse and GetAsync calls throw synchronously. Better be on the safe side instead of relying on that GetAsync behaves exactly as BeginGetResponse and never throws synchronously here.

weltkante · 2019-09-22T06:50:15Z

src/System.Windows.Forms/src/System/Windows/Forms/PictureBox.cs

                _totalBytesRead = 0;

-                Stream responseStream = webResponse.GetResponseStream();
+                Stream responseStream = await resp.Content.ReadAsStreamAsync();

                // Continue on with asynchronous reading.
                responseStream.BeginRead(


How about translating stream reading to async/await as well? Considering that you already are making the method async its probably not too hard. But if you want to keep those changes separated for clarity I won't object, the current diff is nicely isolated.

weltkante · 2019-09-22T06:56:21Z

src/System.Windows.Forms/src/System/Windows/Forms/PictureBox.cs

@@ -609,7 +607,7 @@ private void LoadCompletedDelegate(object arg)

        private void LoadProgressDelegate(object arg) => OnLoadProgressChanged((ProgressChangedEventArgs)arg);

-        private void GetResponseCallback(IAsyncResult result)
+        private async Task GetResponseCallback(IAsyncResult result)


You probably want to change the argument, ContinueWith passes a Task

Also by making the method async you are introducing a new problem, nobody will observe exceptions thrown here since the original ContinueWith used to invoke it isn't followed up by anything. Due to the try/catch in the method this is probably safe, but you should add a comment to the method for future editors that it must not throw, otherwise an unhandled async exception will default to terminating the application.

I agree completely. This is my first attempt at contributing to OSS so I was unclear if these guidelines implied not changing APM to TAP in the case of the callback. I originally made the change in that direction before I realized it might not be accepted due to divergence from the current pattern. I will change that and push code. That should take care of several of the other comments you have as well.

I'm not a team member so if there are open questions feel free to wait for official responses before adressing this.

I understood that, but it's still a valid concern/criticism.

weltkante · 2019-09-22T06:57:41Z

src/System.Windows.Forms/src/System/Windows/Forms/PictureBox.cs

@@ -619,13 +617,12 @@ private void GetResponseCallback(IAsyncResult result)

            try
            {
-                WebRequest req = (WebRequest)result.AsyncState;
-                WebResponse webResponse = req.EndGetResponse(result);
+                HttpResponseMessage resp = (HttpResponseMessage) result.AsyncState;


I think you want to use Task.Result for proper exception handling? Not sure how AsyncState works on a Task object, maybe it does the right thing, but its very nonstandard code and hard to read.

RussKie · 2019-09-24T13:20:28Z

src/System.Windows.Forms/src/System/Windows/Forms/PictureBox.cs

-                // Invoke BeginGetResponse on a threadpool thread, as it has unpredictable latency
-                req.BeginGetResponse(new AsyncCallback(GetResponseCallback), req);
-            });
+            using HttpClient client = new HttpClient();


❗️ This is an antipattern, HttpClient mustn't be used like this. This was a very unfortunate design decision made by the team.
More info:

https://aspnetmonsters.com/2016/08/2016-08-27-httpclientwrong/

https://stackoverflow.com/a/40707446/2338036

The linked article just explains a problem (socket exhaustion) but does not explain at all why calling Dispose prevents the sockets from being closed and why not calling Dispose would close them faster. It just says "not disposing is better" without saying why it makes a difference. I can believe that using a pooled HttpClient instead of many instances is more performant, but I don't see why using Dispose is an antipattern which breaks things.

Anyways, I just want to raise a word of caution making decisions based on that article alone, if you have technical background on the problem or know more about it, I'll not object changing this, but if its just based on this article it might be wise to actually ask someone from corefx for advice.

either way the current code looks broken because it disposes the HttpClient before its done with its request. I don't think the HttpClient should be disposed this early, if at all?

@RussKie Code has been updated and pushed.

For posterity, the using was not disposed of prior to doing work. The scope is implicit with C# 8.0

https://docs.microsoft.com/en-us/dotnet/csharp/whats-new/csharp-8#using-declarations

@neldridg not sure where C# 8 scopes make a difference, usually you await within the using scope to prevent the disposal happening before you are done. This was not the case here because the caller is not async and calling ContinueWith is not compatible with using scopes (regardless of what syntax they use) because it returns immediately. You'd be disposing the HttpClient while the GetAsync Task was still incomplete. (Which may or may not be a problem, I don't know the semantics of disposing HttpClient and the linked article doesn't go into details either, it just claims there are problems associated without explaining them)

Here's the official doc for HttpClient, specifically the remarks sections notes the following:

HttpClient is intended to be instantiated once and re-used throughout the life of an application. Instantiating an HttpClient class for every request will exhaust the number of sockets available under heavy loads. This will result in SocketException errors. Below is an example using HttpClient correctly.

So it would be logical to add an instance of HttpClient as a field to the PictureBox class, and then reuse it.
However an app is likely to have more than one instance of the PictureBox, and thus the instance of the HttpClient should be shared across all of them.

Yeah thats more like I was suspecting, the article is misleading. The dispose is irrelevant (and probably not implemented to dispose everything), so the antipattern is constructing multiple HttpClients in the first place. Static field is probably the way to go, couldn't find any other place in WinForms which wants to do web requests.

Adding more reference for anyone coming along this discussion. You definitely want to reuse HttpClient and not create/dispose them per request to avoid performance regressions: https://github.com/dotnet/corefx/issues/23401#issuecomment-561671803

RussKie · 2019-10-01T04:13:10Z

We will also need some tests.

A good place would be to start with manual verification tests and enhance WinformsControlsTest.PictureBoxes implementation to include several picture boxes, each one loading a sizable image, and then to cancel loading of an image in one of those picture boxes and verify that other picture boxes are unaffected.

neldridg added 4 commits September 21, 2019 18:17

Converted legacy WebRequest calls to HttpClient

b8f308d

Removed unnecessary using statement

5ae691a

Removed dead line of code and satisfied warning.

7e6d595

Changed async void to async Task

e8bc287

neldridg requested a review from a team as a code owner September 22, 2019 00:14

weltkante suggested changes Sep 22, 2019

View reviewed changes

ghost added the waiting-author-feedback The team requires more information from the author label Sep 22, 2019

weltkante reviewed Sep 22, 2019

View reviewed changes

neldridg removed the request for review from a team September 22, 2019 08:28

ghost removed the waiting-author-feedback The team requires more information from the author label Sep 22, 2019

RussKie assigned neldridg Sep 24, 2019

RussKie added the enhancement Product code improvement that does NOT require public API changes/additions label Sep 24, 2019

RussKie added this to the 5.0 milestone Sep 24, 2019

RussKie reviewed Sep 24, 2019

View reviewed changes

Removed Dispose pattern from HttpClient

01bea83

merriemcgaw requested a review from JeremyKuhne September 26, 2019 23:12

neldridg closed this Oct 1, 2019

RussKie removed this from the 5.0 milestone Oct 2, 2019

RussKie removed the enhancement Product code improvement that does NOT require public API changes/additions label Oct 2, 2019

RussKie mentioned this pull request Oct 16, 2019

PictureBox to use HttpClient instead of using WebRequest #1756

Closed

flerka mentioned this pull request Oct 18, 2019

use httpclient instead of webrequest #2124

Closed

neldridg removed their assignment Jul 11, 2021

ghost assigned neldridg Jul 11, 2021

neldridg removed their assignment Jul 12, 2021

ghost assigned neldridg Jul 12, 2021

ghost locked as resolved and limited conversation to collaborators Feb 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert PictureBox WebRequest methods to HttpClient #1952

Convert PictureBox WebRequest methods to HttpClient #1952

neldridg commented Sep 22, 2019 •

edited by ghost

Loading

dnfclas commented Sep 22, 2019 •

edited

Loading

codecov bot commented Sep 22, 2019 •

edited

Loading

weltkante Sep 22, 2019 •

edited

Loading

weltkante Sep 22, 2019 •

edited

Loading

weltkante Sep 22, 2019 •

edited

Loading

neldridg Sep 22, 2019 •

edited

Loading

weltkante Sep 22, 2019

neldridg Sep 22, 2019

weltkante Sep 22, 2019

RussKie Sep 24, 2019

weltkante Sep 24, 2019 •

edited

Loading

neldridg Sep 24, 2019

weltkante Sep 24, 2019 •

edited

Loading

RussKie Oct 1, 2019

weltkante Oct 1, 2019 •

edited

Loading

weltkante Dec 5, 2019

RussKie commented Oct 1, 2019

Convert PictureBox WebRequest methods to HttpClient #1952

Convert PictureBox WebRequest methods to HttpClient #1952

Conversation

neldridg commented Sep 22, 2019 • edited by ghost Loading

Proposed changes

Customer Impact

Regression?

Risk

Test methodology

Test environment(s)

Microsoft Reviewers: Open in CodeFlow

dnfclas commented Sep 22, 2019 • edited Loading

codecov bot commented Sep 22, 2019 • edited Loading

Codecov Report

weltkante Sep 22, 2019 • edited Loading

Choose a reason for hiding this comment

weltkante Sep 22, 2019 • edited Loading

Choose a reason for hiding this comment

weltkante Sep 22, 2019 • edited Loading

Choose a reason for hiding this comment

neldridg Sep 22, 2019 • edited Loading

Choose a reason for hiding this comment

weltkante Sep 22, 2019

Choose a reason for hiding this comment

neldridg Sep 22, 2019

Choose a reason for hiding this comment

weltkante Sep 22, 2019

Choose a reason for hiding this comment

RussKie Sep 24, 2019

Choose a reason for hiding this comment

weltkante Sep 24, 2019 • edited Loading

Choose a reason for hiding this comment

neldridg Sep 24, 2019

Choose a reason for hiding this comment

weltkante Sep 24, 2019 • edited Loading

Choose a reason for hiding this comment

RussKie Oct 1, 2019

Choose a reason for hiding this comment

weltkante Oct 1, 2019 • edited Loading

Choose a reason for hiding this comment

weltkante Dec 5, 2019

Choose a reason for hiding this comment

RussKie commented Oct 1, 2019

neldridg commented Sep 22, 2019 •

edited by ghost

Loading

dnfclas commented Sep 22, 2019 •

edited

Loading

codecov bot commented Sep 22, 2019 •

edited

Loading

weltkante Sep 22, 2019 •

edited

Loading

weltkante Sep 22, 2019 •

edited

Loading

weltkante Sep 22, 2019 •

edited

Loading

neldridg Sep 22, 2019 •

edited

Loading

weltkante Sep 24, 2019 •

edited

Loading

weltkante Sep 24, 2019 •

edited

Loading

weltkante Oct 1, 2019 •

edited

Loading