redth-org / BTProgressHUD

Port to Xamarin.iOS of the SVProgressHUD
Other
118 stars 64 forks source link

BTProgressHUD.Show() from background thread causes UIKitThreadAccessException in debug builds #37

Closed IlSocio closed 9 years ago

IlSocio commented 9 years ago

I just got the exception below:

at Reply.CnhApp.Gui.ViewModels.BaseViewModel.ShowProgressDialog () [0x00018] in /Users/danieleardissone/Documents/XamarinProjects/Cnh/IlSocio/cnh_plm_mobile.git/Gui/Core/ViewModels/BaseViewModel.cs:254 
  at Reply.CnhApp.Gui.ViewModels.BaseViewModel+<ShowLoadingForLongOperationAsync>c__async0`1[System.Boolean].MoveNext () [0x000ad] in /Users/danieleardissone/Documents/XamarinProjects/Cnh/IlSocio/cnh_plm_mobile.git/Gui/Core/ViewModels/BaseViewModel.cs:283 
UIKit.UIKitThreadAccessException: UIKit Consistency error: you are calling a UIKit method that can only be invoked from the UI thread.
  at UIKit.UIApplication.EnsureUIThread () [0x00023] in /Developer/MonoTouch/Source/monotouch/src/UIKit/UIApplication.cs:70 
  at UIKit.UIScreen.get_MainScreen () [0x00000] in /Developer/MonoTouch/Source/monotouch/src/build/native/UIKit/UIScreen.g.cs:318 
  at BigTed.ProgressHUD.get_Shared () [0x00000] in <filename unknown>:0 
  at BigTed.BTProgressHUD.Show (System.String status, Single progress, MaskType maskType) [0x00000] in <filename unknown>:0 

And it seems to be caused by this line of code, which tries to get the MainScreen instance from a background thread: https://github.com/nicwise/BTProgressHUD/blob/master/BTProgressHUD/ProgressHUD.cs#L189

Despite it could be easily fixed by wrapping it around an InvokeOnMainThread(), IMHO, ALL the InvokeOnMainThread() should be definitely removed from this component and replaced with just a UIApplication.EnsureUIThread() check... as suggested here: http://tirania.org/monomac/archive/2012/Sep-10.html it would improve both consistency and efficiency.

When working with gui components, developers are already used to call the InvokeOnMainThread() when they're working in a background thread. In this way, with a single InvokeOnMainThread() call we can perfom all the gui related operations on all different gui component... eg. show the progressdialog, update a label, populate a combo...

[background thread]
InvokeOnMainThread()
{
   [main thread]
   ProgressHUD.Show();
   TextField.Update();
   Combo.Populate();
   ProgressHUD.Hide();
}
[background thread]

While, delegating the call of the InvokeOnMainThread() to each gui component, would be overkill due to the multiple switches from background thread and main thread...

[background thread]
ProgressHUD.Show();  
    InvokeOnMainThread()
    [main thread]

[background thread]
TextField.Update();
    InvokeOnMainThread()
    [main thread]

[background thread]
Combo.Populate();
    InvokeOnMainThread()
    [main thread]

[background thread]
ProgressHUD.Hide();  
    InvokeOnMainThread()
    [main thread]

[background thread]
nicwise commented 9 years ago

Hi @IlSocio

Thanks for this - you are right. The get on Shared DOES need to be run on the UI thread the first time.

So: quick fix is to do that the first time (just PRogressHUD.Shared; should do it?) on the UI thread, then use as normal. I'll fix this shortly and put it into the component store.

Thanks!

IlSocio commented 9 years ago

It would fix the issue for sure, but, actually the "issue" exists just in terms of how BTProgressHUD control has been designed, I'm referring to the fact that this control takes care of switching to the main ui thread by its own in it's implementation.

Imho, a much better approach would be to remove all the InvokeOnMainThread / RunOnUiThread calls from BTProgressHUD and let the developers to take care of switching to the main ui thread when using BTProgressHUD control, like we already do when using any other gui controls provided by the framework. Doing it inside BTProgressHUD, kills the performances and it's not needed, because in any case, we will have to switch on the main thread by our own just to update a simple label...

nicwise commented 9 years ago

Thanks. Mostly fixed (the exception is thrown anyway) in https://github.com/nicwise/BTProgressHUD/commit/b46bd9e6a2658cb0668786279d33c629ed6aad6a