Skip to content
This repository has been archived by the owner on Oct 18, 2023. It is now read-only.

Should ShowDialog be async or Exception handling be moved? #274

Open
mongosr2 opened this issue Jul 5, 2022 · 4 comments
Open

Should ShowDialog be async or Exception handling be moved? #274

mongosr2 opened this issue Jul 5, 2022 · 4 comments

Comments

@mongosr2
Copy link

mongosr2 commented Jul 5, 2022

I am wondering if the method, Prism.Services.Dialogs.Popups.PopupDialogService::ShowDialog, should be async ( maybe deprecated for the async version? ). We are seeing 2 behaviors that seem to be due to a call to an async method from this non-async method. In ShowDialog there is a call to PushPopupPage inside a try catch. When an await is hit in PushPopupPage, the call stack to show dialog unwinds as there is no awaiter in ShowDialog's call to PushPopupPage. We are seeing stack traces that start in PushPopupPage and missing the calls that got us there (see below). Also, we are seeing uncaught exceptions because the try catch block containing the call to PushPopupPage in ShowDialog is never called. That call stack is unwound upon the first await call in PushPopupPage and the on below is started. Since the call stack doesn't unwind into ShowDialogs try / catch, any exception thrown by the call await _popupNavigation.PushAsync(popupPage) in PushPopupPage is an uncaught exception.

However, I do see the interface, IDialogService, from another library that enforces a non-async ShowDialog method. Therefore, if moving to an async version is not the right approach now, then can a try catch be placed around the await in PushPopupPage?

I have attached an example that reproduces what we are seeing. It generates the same stack trace below forcefully. This repro project is strictly meant to illustrate what I am describing above in a crude manner. As such, this is not what we would do in our code. I have put my name, Mike Faltys, in the code where I throw the exception and also around areas that give examples of an additional try/catch to also illustrate the uncaught exception being caught
PrismPopupPlayground.zip
.

System.NullReferenceException: Object reference not set to an instance of an object
at Rg.Plugins.Popup.IOS.Impl.PopupPlatformIos.AddAsync (Rg.Plugins.Popup.Pages.PopupPage page) <0x10b7a29a0 + 0x00178> in <d3bb7c165f354f2f85b824d5211f088c#abba9788d3d687cc747baac2bdd631a1>:0
at Rg.Plugins.Popup.Services.PopupNavigationImpl.AddAsync (Rg.Plugins.Popup.Pages.PopupPage page) <0x10b339850 + 0x0019f> in <5ab9a1a9dc2a49aa9bac5b2ef3232ecc#abba9788d3d687cc747baac2bdd631a1>:0
at Rg.Plugins.Popup.Services.PopupNavigationImpl+<>c__DisplayClass20_0.b__0 () <0x10b338920 + 0x001c7> in <5ab9a1a9dc2a49aa9bac5b2ef3232ecc#abba9788d3d687cc747baac2bdd631a1>:0
at Rg.Plugins.Popup.Services.PopupNavigationImpl+<>c__DisplayClass28_0.b__0 () <0x10b339e40 + 0x0018b> in <5ab9a1a9dc2a49aa9bac5b2ef3232ecc#abba9788d3d687cc747baac2bdd631a1>:0
at Prism.Services.Dialogs.Popups.PopupDialogService.PushPopupPage (Rg.Plugins.Popup.Pages.PopupPage popupPage, Xamarin.Forms.View dialogView) <0x10b3b60e0 + 0x00687> in <6e98ff2754de40538b4d1fcdd1cc93dc#abba9788d3d687cc747baac2bdd631a1>:0
at System.Runtime.CompilerServices.AsyncMethodBuilderCore+<>c.b__7_0 (System.Object state) <0x104b9ed00 + 0x00053> in <25bf495f7d6b4944aa395b3ab5293479#abba9788d3d687cc747baac2bdd631a1>:0
at Foundation.NSAsyncSynchronizationContextDispatcher.Apply () <0x105a8fff0 + 0x0002b> in <5b08e01cb6df409eb2fea153e6177e5d#abba9788d3d687cc747baac2bdd631a1>:0
at (wrapper managed-to-native) UIKit.UIApplication.UIApplicationMain(int,string[],intptr,intptr)
at UIKit.UIApplication.Main (System.String[] args, System.String principalClassName, System.String delegateClassName) <0x105ad1170 + 0x0005f> in <5b08e01cb6df409eb2fea153e6177e5d#abba9788d3d687cc747baac2bdd631a1>:0
at SalesHub.iOS.Application.Main (System.String[] args) <0x10492b9a0 + 0x00177> in <0d055cf5c4e34ca1a50ee4f43f973afc#abba9788d3d687cc747baac2bdd631a1>:0

@dansiegel
Copy link
Owner

First of all the interface comes from Prism.Forms so this wouldn't be an API decision in the Popup Plugin. Second of all the design decision to use callbacks instead of an Async First API was intentional.

@mongosr2
Copy link
Author

mongosr2 commented Jul 6, 2022

Understood, but could we get something in place that stops the uncaught exceptions? I have attached a repro case that illustrates how the uncaught exception is ocurring.

@dansiegel
Copy link
Owner

I don't have the time to look at it right now. This is an open source library and I do take Pull Requests. So if you would like to submit a bug fix I'd be more than happy to review your pr and merge it.

@mongosr2
Copy link
Author

mongosr2 commented Jul 6, 2022

I appreciate that, and completely understand. Unfortunately, I am not able to open PRs for open source due to restrictions from my employer. I did put what I think would be a fix in the sample application for whenever you get a chance to work on this. It is commented out with my name, Mike Faltys, above it.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants