use httpclient instead of webrequest #2124

flerka · 2019-10-18T08:55:28Z

Fixes #1756
Refactor PictureBox to use HttpClient instead of using WebRequest
Before implementing read comments from this pr #1952
Introduced HttpClient field as suggested by @RussKie in #1952 (comment)

Proposed changes

introduce optional HttpClient parameter in PictureBox constructor
use HttpClient instead of using WebRequest
use CancellationToken to cancel request
update error handling due to HttpClient behavior

Customer Impact

may be a breaking change: introduce optional HttpClient parameter in PictureBox constructor

Regression?

Yes

Risk

Low/Medium

Test methodology

Run all tests in the solution
Manual testing and debugging with test project according to debugging guide for various cases (success, unsuccessful such as remote server fail, non-existed image etc.)

Test environment(s)


.NET Core SDK (reflecting any global.json):
 Version:   3.0.100
 Commit:    04339c3a26

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

Host (useful for support):
  Version: 3.0.0
  Commit:  7d57652f33

.NET Core SDKs installed:
  2.1.801 [C:\Program Files\dotnet\sdk]
  2.2.401 [C:\Program Files\dotnet\sdk]
  3.0.100 [C:\Program Files\dotnet\sdk]

.NET Core runtimes installed:
  Microsoft.AspNetCore.All 2.1.12 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
  Microsoft.AspNetCore.All 2.1.13 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
  Microsoft.AspNetCore.All 2.2.6 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
  Microsoft.AspNetCore.All 2.2.7 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
  Microsoft.AspNetCore.App 2.1.12 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 2.1.13 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 2.2.6 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 2.2.7 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 3.0.0 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.NETCore.App 2.1.12 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 2.1.13 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 2.2.6 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 2.2.7 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 3.0.0 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.WindowsDesktop.App 3.0.0 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]

To install additional .NET Core runtimes or SDKs:
  https://aka.ms/dotnet-download

Microsoft Reviewers: Open in CodeFlow

dnfclas · 2019-10-18T08:55:41Z

All CLA requirements met.

weltkante · 2019-10-18T10:42:00Z

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

@@ -101,8 +109,11 @@ public class PictureBox : Control, ISupportInitialize
        ///  Creates a new picture with all default properties and no Image. The default PictureBox.SizeMode
        ///  will be PictureBoxSizeMode.NORMAL.
        /// </summary>
-        public PictureBox()
+        /// <param name="httpClient">Http client instance that will be used to download the image.</param>
+        public PictureBox(HttpClient httpClient = null)


I'm not entirely sure designers will understand that constructor when generating/parsing Designer.cs files, did you test that?

Hello! How do you think maybe it's better to remove HttpClient from constructor to avoid possible problems with designer and add it as optional parameter to LoadAsync method?

We'll probably have to wait for repo members to comment on what they prefer, its new public API. My current preference would be adding an overload to LoadAsync that takes the HttpClient to use as a parameter.

Keeping it a constructor is possible but you'll probably have to split it instead of using a default-initialized parameter (I havent tried but I suspect it is handled badly in the designer, having multiple constructors is definitely ok though). But having it in the constructor also means almost all of the PictureBox users (everyone who uses a designer) won't be able to set it in the first place, so not my favorite.

The alternative of exposing it as a property has various problems (which I can expand on if someone should think its a better idea, but for now I'll wait for more opinions).

But having it in the constructor also means almost all of the PictureBox users (everyone who uses a designer) won't be able to set it in the first place, so not my favorite.

Good point.

@RussKie could you please comment on this?

Modifying the constructor, whilst is an option in theory, won't play nicely with the Designer. Nor it would work for all the existing consumers. So we can't accept this.
I don't think it is a good idea to expose the HttpClient outside the control as we would be leaking implementation details, and won't be able to replace it, if necessary.

For the purpose of tests we can access the instance of HttpClient via reflection.

weltkante · 2019-10-18T10:42:46Z

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

        {
+            _httpClient = httpClient ?? new HttpClient();


The common case of placing the PictureBox in the designer will create multiple HttpClient instances, which (in the other PR) has been identified as something we do not want to do

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

weltkante · 2019-10-18T10:47:01Z

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

+                    HttpResponseMessage responseMessage = await _httpClient.GetAsync(CalculateUri(_imageLocation), _cancellationTokenSource.Token);
+                    if (!responseMessage.IsSuccessStatusCode)
+                    {
+                        PostCompleted(new Exception($"HttpClient received non-successful status code: {responseMessage.StatusCode}"), false);


Other exceptions are localized, you probably want to stick to the same pattern

I didn't understand how to localize it properly so I changed implementation a bit to this:
new Exception($"{responseMessage.ReasonPhrase}: {responseMessage.StatusCode}"). Is it Ok?

No, localization is done through SR members, refer to the other exceptions which already existed in the file to see how it looks like. I don't know either how to add new entries so maybe wait for a repo member to point you into the right direction.

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

codecov · 2019-10-22T14:59:07Z

Codecov Report

Merging #2124 into master will increase coverage by 0.27317%.
The diff coverage is 7.69231%.

@@                 Coverage Diff                 @@
##              master       #2124         +/-   ##
===================================================
+ Coverage   28.15221%   28.42538%   +0.27317%     
===================================================
  Files            910         915          +5     
  Lines         266718      266744         +26     
  Branches       37942       37949          +7     
===================================================
+ Hits           75087       75823        +736     
+ Misses        186400      185693        -707     
+ Partials        5231        5228          -3

Flag	Coverage Δ
#Debug	`28.42538% <7.69231%> (+0.27317%)`	⬆️
#production	`28.42538% <7.69231%> (+0.27317%)`	⬆️
#test	`100% <ø> (ø)`	⬆️

weltkante · 2019-10-22T15:31:36Z

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

+                        return;
+                    }
+
+                    _contentLength = (int)responseMessage.Content.Headers.ContentLength.GetValueOrDefault();


Having read through the rest of the file I think the old code used -1 for unknown content length, it contains explicit checks against that value - leaving it at zero may cause a division by zero during the progress calculation (didn't test it but it looks like it)

Suggested change

_contentLength = (int)responseMessage.Content.Headers.ContentLength.GetValueOrDefault();

_contentLength = (int)(responseMessage.Content.Headers.ContentLength ?? -1);

We should effort to reference a const for -1 perhaps const int UnknownContentLengthSentinal = -1 or something.

Agree with Zach, let's have a const instead of a magic number.

RussKie

Apologies, your PR has totally fallen off my radar.

I think you're moving in the right direction but I feel more work is required.
We have manual tests, but we will also need automated tests to confirm the correctness.

RussKie · 2020-01-31T04:39:34Z

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

@@ -101,8 +109,11 @@ public class PictureBox : Control, ISupportInitialize
        ///  Creates a new picture with all default properties and no Image. The default PictureBox.SizeMode
        ///  will be PictureBoxSizeMode.NORMAL.
        /// </summary>
-        public PictureBox()
+        /// <param name="httpClient">Http client instance that will be used to download the image.</param>
+        public PictureBox(HttpClient httpClient = null)


Modifying the constructor, whilst is an option in theory, won't play nicely with the Designer. Nor it would work for all the existing consumers. So we can't accept this.
I don't think it is a good idea to expose the HttpClient outside the control as we would be leaking implementation details, and won't be able to replace it, if necessary.

For the purpose of tests we can access the instance of HttpClient via reflection.

RussKie · 2020-01-31T04:44:01Z

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

+        /// <summary>
+        ///  Http client instance.
+        /// </summary>
+        private readonly HttpClient _httpClient;


This must be made static to it is shared amongst all instances of PictureBox an app may have.
The HttpClient's API are confusing at best, and that causes a serious confusion amongst developers. There are many articles explaining how to use it correctly, e.g I may find this article useful.

RussKie · 2020-01-31T05:03:19Z

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

+                        return;
+                    }
+
+                    _contentLength = (int)responseMessage.Content.Headers.ContentLength.GetValueOrDefault();


Agree with Zach, let's have a const instead of a magic number.

RussKie · 2020-01-31T05:18:56Z

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

+                    responseStream.BeginRead(
+                        _readBuffer,
+                        0,
+                        ReadBlockSize,
+                        new AsyncCallback(ReadCallBack),
+                        responseStream);


I think this should be replaced with the new async API as well. Something like (pseudo code):

do { var bytesRead = await responseStream.ReadAsync(_readBuffer, 0, ReadBlockSize); if (bytesRead == 0) { isMoreToRead = false; _currentAsyncLoadOperation?.Post(_loadProgressDelegate, new ProgressChangedEventArgs(100, null)); continue; } await _tempDownloadStream.WriteAsync(_readBuffer, 0, bytesRead); _totalBytesRead += bytesRead; if (_contentLength != -1) { int progress = (int)(100 * (((float)_totalBytesRead) / ((float)_contentLength))); _currentAsyncLoadOperation?.Post(_loadProgressDelegate, new ProgressChangedEventArgs(progress, null)); } } while (isMoreToRead);

I looked up the idea here.

ghost · 2020-02-07T08:00:22Z

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 7 days of this comment.

use httpclient instead of webrequest

dea5fad

flerka requested a review from a team as a code owner October 18, 2019 08:55

weltkante suggested changes Oct 18, 2019

View reviewed changes

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

fix code review comments

63ff8cd

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

update exception type due code review

fb9c710

weltkante reviewed Oct 22, 2019

View reviewed changes

RussKie suggested changes Jan 31, 2020

View reviewed changes

ghost added the waiting-author-feedback The team requires more information from the author label Jan 31, 2020

ghost added the no-recent-activity label Feb 7, 2020

ghost closed this Feb 14, 2020

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

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use httpclient instead of webrequest #2124

use httpclient instead of webrequest #2124

flerka commented Oct 18, 2019 •

edited by ghost

Loading

dnfclas commented Oct 18, 2019 •

edited

Loading

weltkante Oct 18, 2019

flerka Oct 22, 2019

weltkante Oct 22, 2019 •

edited

Loading

flerka Oct 22, 2019

RussKie Jan 31, 2020

weltkante Oct 18, 2019 •

edited

Loading

weltkante Oct 18, 2019

flerka Oct 22, 2019

weltkante Oct 22, 2019

codecov bot commented Oct 22, 2019 •

edited

Loading

weltkante Oct 22, 2019 •

edited

Loading

zsd4yr Oct 23, 2019

RussKie Jan 31, 2020

RussKie left a comment

RussKie Jan 31, 2020

RussKie Jan 31, 2020

RussKie Jan 31, 2020

RussKie Jan 31, 2020

ghost commented Feb 7, 2020

	_contentLength = (int)responseMessage.Content.Headers.ContentLength.GetValueOrDefault();
	_contentLength = (int)(responseMessage.Content.Headers.ContentLength ?? -1);

use httpclient instead of webrequest #2124

use httpclient instead of webrequest #2124

Conversation

flerka commented Oct 18, 2019 • edited by ghost Loading

Proposed changes

Customer Impact

Regression?

Risk

Test methodology

Test environment(s)

Microsoft Reviewers: Open in CodeFlow

dnfclas commented Oct 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weltkante Oct 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weltkante Oct 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 22, 2019 • edited Loading

Codecov Report

weltkante Oct 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RussKie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Feb 7, 2020

flerka commented Oct 18, 2019 •

edited by ghost

Loading

dnfclas commented Oct 18, 2019 •

edited

Loading

weltkante Oct 22, 2019 •

edited

Loading

weltkante Oct 18, 2019 •

edited

Loading

codecov bot commented Oct 22, 2019 •

edited

Loading

weltkante Oct 22, 2019 •

edited

Loading