fix: `PictureBox.LoadAsync` throws `PlatformNotSupportedException` #1696

RussKie · 2019-08-26T19:29:06Z

Proposed changes

Async delegates have been deprecated in .NET Core (see https://github.com/dotnet/corefx/issues/5940) for reasons such as:

Async delegates use deprecated IAsyncResult-based async pattern. This pattern is generally not supported throughout .NET Core base libraries.
Async delegates depend on remoting (System.Runtime.Remoting) under the hood. Remoting is not supported in .NET Core.

For more detailed reasons and migration strategies please refer to https://devblogs.microsoft.com/dotnet/migrating-delegate-begininvoke-calls-for-net-core/

In case of WinForms, async pattern predates the Task-based Asynchronous Pattern and it does not return awaitables.
In order to minimise the change to the public API surface we continue to expose the existing API and just re-route the image loading routine to a Task runner under the covers.

Customer Impact

Customers can continue loading images using the existing API

Regression?

Yes

Risk

Low, customers will be able to load image asynchronously again.

Test methodology

manual via the integration app; the following use cases were considered
- load an image that results in an error (e.g. https://publicdomainarchive.com/wp-content/uploads/2014/03/public-domain-images-free-stock-photos-high-quality-resolution-downloads-around-the-house-1.jpg - SSL error)
- load a large image (e.g. https://initiate.alphacoders.com/download/wallpaper/685514/images5/jpg/228368184713846/97229) and observe a progress reported, as well as a UI responsiveness
- cancel during a large image loading process and observe events correctly fired, no further progress reported, GC collecting memory

Microsoft Reviewers: Open in CodeFlow

weltkante

Doesn't look correct to me, multithreading is hard :-/

RussKie · 2019-08-26T20:14:10Z

Doesn't look correct to me

What doesn't look correcT?

weltkante · 2019-08-26T20:14:59Z

What doesn't look correcT?

Sorry, github didn't take my comments, I'll review again. Might have already adressed some of them ;-)

RussKie · 2019-08-26T20:17:21Z

I'll review again. Might have already adressed some of them ;-)

No worries... I'm done for the day, no more force-pushes from me until tomorrow ;)

weltkante

First review didn't take my comments, trying again. Also had comments about reusability of the cancellation token but that seems to have been adressed already.

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

weltkante · 2019-08-29T19:26:08Z

Looks better to me now. Having read through it a few times I'm starting to feel a bit iffy on the cancellation. I think the old Delegate.BeginInvoke call wouldn't have been subject to cancellation, so if you now allow the Task.Run to be canceled wouldn't that introduce the possibility that nobody ever resets the control states?

I wonder if its possible to get the PictureBox stuck in a canceled loading state:

call LoadAsync
immediately call CancelAsync
if the thread pool is under load it may not yet have scheduled the Task.Run call and cancel it
who is now responsible to clean up the control states?

Considering that the only point of the CancellationTokenSource is to stop the Task before it runs, it might be reasonable to get rid of the CTS entirely and just leave it at the preexisting cancellation logic to break out of the partial image download? Not great, but shouldn't be worse than the old implementation.

You'll probably want to take someone elses opinion on this, multithreading is indeed hard :-(

Async delegates have been deprecated in .NET Core (see https://github.com/dotnet/corefx/issues/5940) for reasons such as: * Async delegates use deprecated IAsyncResult-based async pattern. This pattern is generally not supported throughout .NET Core base libraries. * Async delegates depend on remoting (System.Runtime.Remoting) under the hood. Remoting is not supported in .NET Core. For more detailed reasons and migration strategies please refer to https://devblogs.microsoft.com/dotnet/migrating-delegate-begininvoke-calls-for-net-core/ In case of WinForms, async pattern predates the Task-based Asynchronous Pattern and it does not return awaitables. In order to minimise the change to the public API surface we continue to expose the existing API and just re-route the image loading routine to a `Task` runner under the covers. Fixes dotnet#242 Fixes dotnet#1548

codecov · 2019-08-30T15:16:54Z

Codecov Report

Merging #1696 into release/3.0 will decrease coverage by 0.00974%.
The diff coverage is 0%.

@@                 Coverage Diff                  @@
##           release/3.0      #1696         +/-   ##
====================================================
- Coverage     25.95534%   25.9456%   -0.00974%     
====================================================
  Files              804        804                 
  Lines           267706     267714          +8     
  Branches         37950      37951          +1     
====================================================
- Hits             69484      69460         -24     
- Misses          193295     193327         +32     
  Partials          4927       4927

Flag	Coverage Δ
#Debug	`25.9456% <0%> (-0.00975%)`	⬇️
#production	`25.9456% <0%> (-0.00975%)`	⬇️
#test	`100% <ø> (ø)`	⬆️

RussKie · 2019-08-30T15:24:44Z

Your multi-threading kung-fu is better than mine and reasoning is more sound 😄
I think your assessment is correct - if a user cancels before a task is run, the picturebox may get locked in a state that would no longer load any images, and safe-guarding against that would require more code and plumbing duplicating the infra the pcturebox already has...

RussKie · 2019-09-03T08:17:18Z

The fix is signed off by CTI, no regressions found.

leecow · 2019-09-03T17:34:53Z

Tactics approved after final code reviews.

zsd4yr · 2019-09-03T18:40:19Z

@RussKie this is good to go. Please merge at your leisure

sharwell · 2019-09-03T22:17:00Z

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);
+            });


@stephentoub Can con confirm this is the expected translation for a BeginInvoke usage like this?

This is reasonable.

(Though I don't think the comment is relevant on .NET Core, so it's not clear how relevant this queueing is anymore. Plus, it'd be better to switch to a cached HttpClient instead of using WebRequest.)

Plus, it'd be better to switch to a cached HttpClient instead of using WebRequest

Certainly, Sam has already flagged it offline.
We will replace WebRequest in .NET 5.0

RussKie added the waiting-for-testing The PR is awaiting manual testing by the primary team; no action is yet required from the author(s) label Aug 26, 2019

RussKie requested review from sharwell, JeremyKuhne, ericstj and a team August 26, 2019 19:29

RussKie self-assigned this Aug 26, 2019

RussKie added this to the 3.0-GA milestone Aug 26, 2019

RussKie force-pushed the fix_1548_PictureBox.LoadAsync_throws_PNSE branch 3 times, most recently from 6a57e2b to a3bb61b Compare August 26, 2019 20:12

weltkante suggested changes Aug 26, 2019

View reviewed changes

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

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

weltkante suggested changes Aug 26, 2019

View reviewed changes

src/System.Windows.Forms/src/System/Windows/Forms/PictureBox.cs Outdated Show resolved Hide resolved

src/System.Windows.Forms/src/System/Windows/Forms/PictureBox.cs Outdated Show resolved Hide resolved

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

RussKie force-pushed the fix_1548_PictureBox.LoadAsync_throws_PNSE branch from a3bb61b to bd1d8f3 Compare August 29, 2019 17:13

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

RussKie force-pushed the fix_1548_PictureBox.LoadAsync_throws_PNSE branch from bd1d8f3 to e8bb804 Compare August 30, 2019 15:16

RussKie added ask-mode and removed waiting-for-testing The PR is awaiting manual testing by the primary team; no action is yet required from the author(s) labels Sep 3, 2019

RussKie assigned zsd4yr Sep 3, 2019

JeremyKuhne approved these changes Sep 3, 2019

View reviewed changes

RussKie merged commit dcc9ebf into dotnet:release/3.0 Sep 3, 2019

RussKie deleted the fix_1548_PictureBox.LoadAsync_throws_PNSE branch September 3, 2019 21:43

RussKie removed the waiting-approval label Sep 3, 2019

sharwell reviewed Sep 3, 2019

View reviewed changes

RussKie mentioned this pull request Sep 4, 2019

PictureBox to use HttpClient instead of using WebRequest #1756

Closed

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

fix: `PictureBox.LoadAsync` throws `PlatformNotSupportedException` #1696

fix: `PictureBox.LoadAsync` throws `PlatformNotSupportedException` #1696

RussKie commented Aug 26, 2019 •

edited

Loading

weltkante left a comment

RussKie commented Aug 26, 2019

weltkante commented Aug 26, 2019

RussKie commented Aug 26, 2019

weltkante left a comment •

edited

Loading

weltkante commented Aug 29, 2019 •

edited

Loading

codecov bot commented Aug 30, 2019

RussKie commented Aug 30, 2019

RussKie commented Sep 3, 2019

leecow commented Sep 3, 2019

zsd4yr commented Sep 3, 2019

sharwell Sep 3, 2019

stephentoub Sep 4, 2019

RussKie Sep 4, 2019

RussKie Sep 4, 2019

fix: PictureBox.LoadAsync throws PlatformNotSupportedException #1696

fix: PictureBox.LoadAsync throws PlatformNotSupportedException #1696

Conversation

RussKie commented Aug 26, 2019 • edited Loading

Proposed changes

Customer Impact

Regression?

Risk

Test methodology

Microsoft Reviewers: Open in CodeFlow

weltkante left a comment

Choose a reason for hiding this comment

RussKie commented Aug 26, 2019

weltkante commented Aug 26, 2019

RussKie commented Aug 26, 2019

weltkante left a comment • edited Loading

Choose a reason for hiding this comment

weltkante commented Aug 29, 2019 • edited Loading

codecov bot commented Aug 30, 2019

Codecov Report

RussKie commented Aug 30, 2019

RussKie commented Sep 3, 2019

leecow commented Sep 3, 2019

zsd4yr commented Sep 3, 2019

sharwell Sep 3, 2019

Choose a reason for hiding this comment

stephentoub Sep 4, 2019

Choose a reason for hiding this comment

RussKie Sep 4, 2019

Choose a reason for hiding this comment

RussKie Sep 4, 2019

Choose a reason for hiding this comment

fix: `PictureBox.LoadAsync` throws `PlatformNotSupportedException` #1696

fix: `PictureBox.LoadAsync` throws `PlatformNotSupportedException` #1696

RussKie commented Aug 26, 2019 •

edited

Loading

weltkante left a comment •

edited

Loading

weltkante commented Aug 29, 2019 •

edited

Loading