Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature request: Exception callbacks #1571

Open
hez2010 opened this issue Apr 15, 2024 · 5 comments · May be fixed by #1578
Open

Feature request: Exception callbacks #1571

hez2010 opened this issue Apr 15, 2024 · 5 comments · May be fixed by #1578
Labels
enhancement New feature or request

Comments

@hez2010
Copy link
Contributor

hez2010 commented Apr 15, 2024

Proposal: Exception callbacks

Summary

In out-of-proc scenario, we can only see COMException without any message from the client if any unhandled exception happens on the server (see #1562).
We cannot intercept it without wrapping every API with a try-catch block. This makes it difficult to do unified unhandled exception handling and logging.
I propose to add the support for exception callbacks which allows propagate the exception to a user specified handler to handle unhandled exceptions:

namespace WinRT;

public static class ExceptionHelpers
{
        /// <summary>
        /// Unhandled WinRT server exception event.
        /// </summary>
        public static event EventHandler<UnhandledWinRTServerExceptionEventArgs> UnhandledWinRTServerException;

        public class UnhandledWinRTServerExceptionEventArgs : EventArgs
        {
            public Exception Exception { get; }
            public bool Handled { get; set; }

            public UnhandledWinRTServerExceptionEventArgs(Exception exception)
            {
                Exception = exception;
            }
        }

        public static bool TryHandleWinRTServerException(object sender, Exception ex)
        {
            EventHandler<UnhandledWinRTServerExceptionEventArgs> handler = UnhandledWinRTServerException;
            if (handler != null)
            {
                UnhandledWinRTServerExceptionEventArgs args = new UnhandledWinRTServerExceptionEventArgs(ex);
                handler.Invoke(sender, args);
                return args.Handled;
            }
        }
}

WinRT codegen

[UnmanagedCallersOnly(CallConvs = new[] { typeof(CallConvStdcall) })]
private static unsafe int Do_Abi_GetPoint_2(IntPtr thisPtr, global::Windows.Foundation.Point* __return_value__)
{
    global::AuthoringTest.BasicClass __this__ = default;
    global::Windows.Foundation.Point ____return_value__ = default;
    *__return_value__ = default;
    try
    {
        ____return_value__ = (__this__ = global::WinRT.ComWrappersSupport.FindObject<global::AuthoringTest.BasicClass>(thisPtr)).GetPoint();
        *__return_value__ = ____return_value__;

    }
    catch (Exception __exception__)
    {
        if (global::WinRT.ExceptionHelpers.TryHandleWinRTServerException(__this__, __exception__))
        {
            return 0;
        }
        global::WinRT.ExceptionHelpers.SetErrorInfo(__exception__);
        return global::WinRT.ExceptionHelpers.GetHRForException(__exception__);
    }
    return 0;
}

Usage

WinRT.ExceptionHelpers.UnhandledWinRTServerException += (sender, args) =>
{
    // ...
    if (this_exception_has_been_handled_correctly) args.Handled = true;
}
@hez2010 hez2010 added the enhancement New feature or request label Apr 15, 2024
@hez2010 hez2010 linked a pull request Apr 19, 2024 that will close this issue
@manodasanW
Copy link
Member

If the mentioned bug was to get fixed for most scenarios to possibly propagate the error message, would we still want this custom first chance Unhandled exception handler?

@jlaanstra
Copy link
Collaborator

jlaanstra commented Apr 19, 2024

This event would be raised in many non-server scenarios, for example if native code calls in-proc into managed. This also seems trivial to implement for specific classes that are used out-of-proc by adding the try/catch yourself.

@Sergio0694
Copy link
Member

Sergio0694 commented Apr 19, 2024

I also don't really like this change. It's not pay for play and requires extra checks on every single handled exception for every WinRT type ever, even though 99.999% of them will never actually need this. It also has a slight binary size increase due to all that additional code in every single generated marshalling stub. Especially if there's a workaround, I'm not a fan of this.

@hez2010
Copy link
Contributor Author

hez2010 commented Apr 20, 2024

If the mentioned bug was to get fixed for most scenarios to possibly propagate the error message, would we still want this custom first chance Unhandled exception handler?

If #1562 gets fixed, I think we may not need this so urgently. The inability to intercept unhandled exceptions on the server in managed code means that we are not able to capture the stacktrace.

@hez2010
Copy link
Contributor Author

hez2010 commented Apr 20, 2024

requires extra checks on every single handled exception for every WinRT type ever

The check only happens when an exception on WinRT server is unhandled from managed code.
Even #1562 gets fixed in the future, we still lose the stacktrace of the server.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants