Clarify should authgear wrap platform's web view cancelled exception

[roxk]

Throw Authgear's cancel exception.

[roxk]

@louischan-oursky The exception thrown by xamarin essential is the platform's TaskCancelledException. You can think of it like PromiseCancelledError in js I suppose (task is C#'s promise). So for idiomatic C# it seems like we should ditch the custom Authgear.CancelException and just use TaskCancelledException everywhere 🤔

cc @kiootic

[louischan-oursky]

As long as both platform, iOS and Android, throw the same TaskCancelledException, then it is fine to use it instead of inventing our own.

[kiootic]

Since I'm tagged, here's my 2 cents:

A quick look of Authgear.CancelException seems it's related to domain logic of authentication (i.e. user cancelled/rejected the authentication flow), while TaskCanceledException is mainly used by the Task infrastructure to signal cancellation when trying to get result (note the Task property there).

IMO Authgear.CancelException should probably inherits from OperationCanceledException.

[louischan-oursky]

The baseline is e is TaskCanceledException and e is OperationCanceledException should be true.

[roxk]

I think you meant or instead of and?

IMO both work, and practically all operation that has a notion of cancellation is long running, and should be modelled as task in modern C#. I checked the doc for OperationCanceledException and it says "operation in a thread", which is a bit weird. But!

public class TaskCanceledException : OperationCanceledException

So TaskCanceledException inherit OperationCanceledException. Doc.

So using either would satisfy louis's baseline.

In the spirit of TaskCanceledException, I opt for having distinct class inheriting OperationCanceledException. This way, developers who want to know exactly what was cancelled can check the derived exception, and those who don't care just check the base. This I think can be called good design and error handling.

There is currently only one place where CancelException is used, and that is biometric. I would create a BiometricCanceledException : OperationCanceledException then.

[louischan-oursky]

Why don't we do CancelException : TaskCanceledException? BiometricCanceledException is too narrow IMO. e is TaskCanceledException returns false would be surprising to the developer.

CancelException would also be used in opening settings page via webview.

[roxk]

BiometricCanceledException is too narrow IMO. e is TaskCanceledException returns false would be surprising to the developer.

1. The base for all C#'s cancellation is OperationCanceledException (even TaskCanceledException inherits from it) so it shouldn't be surprising
2. e is OperationCanceledException would be true, e is BiometricCanceledException would also be true. Developers might have a single display error handler that check for each narrower type and show different error dialog message. This gives devs control.

CancelException would also be used in opening settings page via webview.

Seems not in android sdk 🤔. Also IMO it shouldn't be an exception as going back is the expected thing to do. Or do you mean elsewhere? If you mean OpenAuthorizationUrl, this is handled by a AuthenticationCanceledException (I just created lol).

[louischan-oursky]

I am calling a function that returns a Task, so it follows naturally that I catch TaskCanceledException to detect cancellation. If our cancel exception does not inherit from TaskCanceledException, it would be surprising. Does this make sense?

Splitting the cancel exception into very fine grained classes does not have much benefits. Most of the time the developer would just call one method, and all they care about is whether it is canceled or not. It is cognitive burden on the developer to ask them to catch different cancel exception class for each method.

try {
  await authgear.Authenticate(...);
} catch (CancelException e) {
  // Catch CancelException consistently to detect cancellation
}

try {
  await authgear.AuthenticateBiometric(...);
} catch (CancelException e) {
  // Catch CancelException consistently to detect cancellation
}

[kiootic]

I am calling a function that returns a Task, so it follows naturally that I catch TaskCanceledException to detect cancellation.

Note that TaskCanceledException is actually usually generated from Task infrastructure when the task is marked as cancelled instead of thrown from user code, which is why I think throwing/inheriting it is not ideal: https://github.com/dotnet/runtime/blob/57bfe474518ab5b7cfe6bf7424a79ce3af9d6657/src/libraries/System.Private.CoreLib/src/System/Threading/Tasks/Task.cs#L1820-L1830

Also note that Task would be marked as cancelled when OperationCanceledException is thrown: https://github.com/dotnet/runtime/blob/57bfe474518ab5b7cfe6bf7424a79ce3af9d6657/src/libraries/System.Private.CoreLib/src/System/Threading/Tasks/Task.cs#L58-L62

[roxk]

It is cognitive burden on the developer to ask them to catch different cancel exception class for each method.

They don't have to. They can still catch the base and it would still work. Let's say we use CancelException as base, adding BiometricCanceledException : CancelException and throw it, your above code would still work. So its adding more control to devs who need it without complicating the "naive/lazy" approach.

(Not to mention "all they care about is whether it is canceled or not" isn't necessarily true 😅)

If we agree on this, then the only disagreement is which is the base. I personally don't mind using TaskCanceledException as base. Although that would couple the operation in question to task unnecessarily.

[louischan-oursky]

I suggest using TaskCanceledException because xamarin uses it, so I guess the developer should be familiar with it.

As I said in my first comment, if the platform already provides an exception to represent cancellation, I prefer we just use it. I can also argue why WebAuthenticator.AuthenticateAsync does not throw AuthenticateCanceledException: TaskCanceledException. It simply does not provide much value to introduce a new class to represent the very same thing.

[roxk]

I suggest using TaskCanceledException because xamarin uses it

Oh, I see where you are coming from now. So Xamarin actually didn't explicitly use/throw it, it called TaskCompletionSource.TrySetCanceled(): https://github.com/xamarin/Essentials/blob/main/Xamarin.Essentials/WebAuthenticator/WebAuthenticator.android.cs#L25 (where TaskCompleteSource is just the "emitter" for a task)

I can also argue why WebAuthenticator.AuthenticateAsync does not throw AuthenticateCanceledException

Putting aside the fact that IMO it should, by "to introduce a new class to represent the very same thing", do you mean BiometricCanceledException? IMO it's not the same class as it allows dev to differentiate between errors when they need it. It's very common in enterprise app to have more detailed errors for each case instead of just a general "Something went wrong".

Your previous worries that the "base" case is also solved as exception catching is polymorphic. I just tested and it works as expected :) Devs who don't it can just catch the base and the catch block would reach even though the in-flight exception is a derived exception. So, again, the design take very minimal effort (just declare 2 class, use them appropriate), doesn't block default/easy usage, and allows more controls. I think it's providing good value.

There is implementation motivation as well: currently the biometric cancellation is checked inside wrapException, which itself knows nothing about whether the exception would be thrown inside a task or not 🤔

[louischan-oursky]

It's very common in enterprise app to have more detailed errors for each case instead of just a general "Something went wrong".

I agree with this. This is the reason why we have BiometricLockoutException and friends. But cancel really just means cancel, no much more details we can attach to it. I would agree to introduce a cancel exception class if the new class can provide new information (e.g. a new field telling me why it was canceled, for example)

Let me sum up my concerns here.

The downside of having very fine grained cancel exception classes

• Show up as an extra entry in the API reference documentation. Keeping the API small and simple is important.
• Need to document them separately
• May confuse the developer which one to catch
• Not consistently with other SDKs

There is implementation motivation as well: currently the biometric cancellation is checked inside wrapException, which itself knows nothing about whether the exception would be thrown inside a task or not

The implementation checks against API like BiometricManager.BiometricErrorNoHardware, which probably is the result of calling some function returning Task. So assuming task is involved is valid to me.

[roxk]

Show up as an extra entry in the API reference documentation. Keeping the API small and simple is important.

I think we can afford to add 2 more errors given the myriad of existing errors. Devs who don't it won't read/won't be affected by it anyways 🤔

May confuse the developer which one to catch

Devs with basic knowledge on polymorphism would know which to catch since our doc would document they are derived from the same base. Also, while using only one exception works and would not confuse dev, it's taking away controls from devs who need it.

Not consistent with other SDKs

That is a chicken and egg problem IMO. If we agree this is good we should update other SDKs instead of using it as a reason for not doing it. Now of course, if we can't afford the time to do that, then it's a valid reason to stick to the status quo.

The implementation checks against API like BiometricManager.BiometricErrorNoHardware, which probably is the result of calling some function returning Task

The native java API is callback based and there is no task. Current effort to centralize error checking (wrapException) also have no knowledge it's from a task. Although admittedly the returned exception from wrapException is throw using TaskCompletionSource. But then the function itself IMO shouldn't assume how the result is used.

Also just found a blogpost showing example code, and all of them are catching OperationCanceledException, even inside a task/awaiting a task 🤔

---

This is getting long and I understand there are other more important things than aligning this bit of the SDK. So, given the above piece of blog (which I think is quite a persuasive evidence), I will suggest the following compromise: is replacing CancelException with just OperationCanceledException acceptable? So no more fine grained exception, just the system one. But we are using OperationCanceledException instead of TaskCanceledException.

[louischan-oursky]

is replacing CancelException with just OperationCanceledException acceptable?

Yes. Just to make sure OperationCanceledException is documented to be thrown by some of our methods.