Avoid using LoadAsync() method of PictureBox control

[KlausLoeffelmann]

Steps to reproduce:

1. Create a new WinForms Core App.
2. Add the following Code to the Form's constructor.

public Form1()
        {
            InitializeComponent();
            System.Windows.Forms.PictureBox pb = new PictureBox();
            pb.ImageLocation = @"\\winformssrvvm01\BAQAFiles\v-mikou\OGF Doc\picture\myLargeImage1.bmp";
            pb.LoadAsync();
        }

3. Run the app.

Expected Result: LoadAsync should be executed. (NOTE: Although LoadAsync implies this, the method does not return Task but void, so the usage here is correct with regards to async/await.)

Actual Result: An exception pops up, please see following screenshot and exception log.

Exception log:

System.PlatformNotSupportedException
  HResult=0x80131539
  Message=Operation is not supported on this platform.
  Source=System.Private.CoreLib
  StackTrace:
   at System.Threading.WaitCallback.BeginInvoke(Object state, AsyncCallback callback, Object object)
   at System.Windows.Forms.PictureBox.LoadAsync()
   at XpictureIssue.Form1..ctor() in C:\Users\v-zheshi\source\winforms\XpictureIssue\Form1.cs:line 20
   at XpictureIssue.Program.Main() in C:\Users\v-zheshi\source\winforms\XpictureIssue\Program.cs:line 19

[jnm2]

The BeginInvoke call is something I think https://docs.microsoft.com/dotnet/standard/analyzers/api-analyzer would have caught at compile time, IIRC.

[KlausLoeffelmann]

@ericstj , could you take a look?

[KlausLoeffelmann]

In addition: What do folks think about making this method Obsolete, and come up with a new one which does keep what its promising (namely returning Task and not void)?

[weltkante]

It doesn't promise returning a Task, that's some naming convention which was invented way later than WinForms. Errors are reported through LoadCompleted event, would you obsolete that as well or expose the same error twice through different means?

Without having a proper infrastructure for async support in the WinForms framework I see little value in adding Task support for this particular method. How would existing callers be migrated to await the Task? Existing callers are most likely not in an async method so awaiting for purpose of observing potential errors is trouble most callers probably discard (and then get killed by unhandled exceptions if an error actually happens and they didn't observe it, I see that kind of lazy programming crashes constantly in UWP apps where async is forced onto the UI and people ignore error handling because the framework provides no easy way to do it correctly).

Alltogether, I agree WinForms needs to think about async support, but this is probably not a good place to start with.

[KlausLoeffelmann]

Alltogether, I agree WinForms needs to think about async support, but this is probably not a good place to start with.

I think we just have! 😊

Think about the target audience for a second that WinForms has - this is quite different from what you normally have in Core. So, I think, this might be even a great opportunity to start, because the use case is simple and straight forward. On top I expect the actual usage of this method extremely low. And at the same time, I expect that the typical Windows audience expects the usage of this method extremely differently: If a Dev sees this method ending with Async, they will be rather surprised not getting back a Task or some Async operation/result.

So, obviously it's not my intension to break existing apps. What's in is in and no one wants to take that out. But I don't see any reason to tag both methods with Obsolete and advertise an new way with "Hey, there's a better way to do this - here is how it's done:" and refer to an alternative implementation?

[asmaaswt]

البرمجة بالعربية غير مدعومة

Automated translation: Programming in Arabic is not supported

[karelz]

@asmaaswt we use English in our repos - we not have capability and/or capacity to use other languages. English is treated as most common language for most developers world-wide.

I used automated translation (updated above), but I am not sure if it is correct, or how it relates to this specific issue. Can you please explain in more details what you meant and how it relates to this issue? Thanks!

[kevingosse]

The BeginInvoke call is something I think https://docs.microsoft.com/dotnet/standard/analyzers/api-analyzer would have caught at compile time, IIRC.

I gave it a try, to my surprise Delegate.BeginInvoke is not detected by the analyzer.

[ericstj]

Just to confirm, I believe this is intentional per
https://github.com/dotnet/corefx/issues/5940#issuecomment-182107557 /cc @jkotas. You'll need to make a fix to WinForms here to avoid this API. The comment here may not even apply any more on Core so perhaps you can just call BeginGetResponse

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

Lines 671 to 674 in 408a31d

	// Invoke BeginGetResponse on a threadpool thread, as it has
	// unpredictable latency, since, on first call, it may load in the
	// configuration system (this is NCL
	(new WaitCallback(BeginGetResponseDelegate)).BeginInvoke(req, null, null);

Alternatively just use the Task infrastructure to run the code on a background thread.

In addition: What do folks think about making this method Obsolete, and come up with a new one which does keep what its promising (namely returning Task and not void)?

We had similar goals back in .NETCore1.0 but dialed it back. In most cases it's not worth breaking someone over just having a newer API. There should be some really good reason to force a dev to think about the change. "New" is not a good enough reason. Thus we still have all the old event-based and APM based API next to task based API.

It's a reasonable goal to plumb task-based async API through WinForms, but that should be done in a wave impacting all async winforms APIs. That way you aren't forcing people to learn two different async programming models. I can imagine that being a pretty huge task given that WinForms relies heavily on the event-based model.