Skip to content

Catch all the exception in touch process This MR do not expect to merge#936

Closed
lindexi wants to merge 1 commit intodotnet:masterfrom
dotnet-campus:t/lindexi/CefearkearkurWhewearlurhiwheejawqar
Closed

Catch all the exception in touch process This MR do not expect to merge#936
lindexi wants to merge 1 commit intodotnet:masterfrom
dotnet-campus:t/lindexi/CefearkearkurWhewearlurhiwheejawqar

Conversation

@lindexi
Copy link
Member

@lindexi lindexi commented Jun 13, 2019

Because we can not use any way to repair it when the code throw the exception and the Stylus Input thread will exit

Maybe it can fix #935

It is an evil code

Because we can not use any way to repair it when the code throw the exception and the Stylus Input thread will exitt
@ghost ghost added the PR metadata: Label to tag PRs, to facilitate with triage label Jun 13, 2019
@ghost ghost requested review from rladuca, ryalanms, stevenbrix and vatsan-madhavan June 13, 2019 03:28
@weltkante
Copy link

weltkante commented Jun 13, 2019

I'd prefer bugs being fixed instead of being ignored. If you ignore the exceptions they will never get fixed. It just leads to problems of the type "my touch input stops responding" because the thread may be in a state it can't recover from and just keeps throwing and ignoring exceptions.

If the WPF team really wants a catch-all handler it should have associated diagnostics and not just ignore the exception.

Personally I have my own top level exception handlers on AppDomain, Dispatcher, Application etc. to report (and potentially ignore) unhandled exceptions. I believe the option to ignore exceptions from which it cannot recover should be left to the application programmer, not be made at the framework level. For example WinForms has Application.SetUnhandledExceptionMode to assist with this problem.

}
catch (Exception)
{
// If the code above throw an exception that the Stylus Input thread will be break. And we can not use any way to repair it.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some grammar issues.

// If the code above throws an exception the Stylus Input thread will break and there is nothing we can do to repair it.

catch (Exception)
{
// If the code above throw an exception that the Stylus Input thread will be break. And we can not use any way to repair it.
// Maybe only some touch device be crash but other touch device can run still.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only some of the touch devices may crash and the others can still run normally.

{
// If the code above throw an exception that the Stylus Input thread will be break. And we can not use any way to repair it.
// Maybe only some touch device be crash but other touch device can run still.
// To catch all the exception to make the application can run when the code throw an exception.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We catch all the exceptions so that the application can still be alive.

@lindexi
Copy link
Member Author

lindexi commented Jun 13, 2019

@weltkante I think the better way is to offer some way to restart the Stylus Input thread or some way to reset the status

@walterlv
Copy link
Contributor

walterlv commented Jun 13, 2019

@weltkante You mean that we'd better unload the touch module and then initialize it again during the catch statement?

We have some other similar situations that reinitializing can fix stylus issues such as these below:

The codes that cause the issues are different but reinitializing the whole stylus module can fix them.

@lindexi
Copy link
Member Author

lindexi commented Jun 13, 2019

It just leads to problems of the type "my touch input stops responding"

@weltkante The user can only fix it by reboot the device when the fullscreen application running in the device which only supports touch input. The application stops responding to touch input is an important crash for the touch application.

But I must admit that this is not a good way and it is an evil code. We should not ignore the exception.

@weltkante
Copy link

In my opinion WPF needs to deal gracefully with hardware devices being (un)plugged or becoming unusable. If it can't do that I prefer an application crash (usually surfaced via unhandled exceptions) over stopping to respond, so I can bring up a diagnostic interface to report the problem to our support. Our customers usually have support contracts and if they have faulty devices preventing proper usage of our software we may be able to root cause the problem this way. If the input just stops working this is much harder to diagnose on our side and the customer isn't any more happy either.

I undestand not everyone wants that, thats why I brought up the option of configurable top level exception handling, like WinForms allows in Application.SetUnhandledExceptionMode. I don't know if WPF already has a similar setting. If you want to ignore exceptions you can't recover from, you'll have to give the application the chance to abort (or restart) the whole program.

You mean that we'd better unload the touch module and then initialize it again during the catch statement?

If this is guaranteed to fix the problem you can do that. But what if the device is broken and you end up an infinite loop of ignoring exceptions? I don't want to burn a CPU core at 100% just repeatedly ignoring the same exception.

@lindexi
Copy link
Member Author

lindexi commented Jun 13, 2019

This MR may fix the problem that some friend throws an exception in StylusPlugIn.

Just like this code. The code will break the Stylus Input thread.

public partial class MainWindow : Window
{
    public MainWindow()
    {
        InitializeComponent();
        StylusPlugIns.Add(new Foo());
    }
}

public class Foo : StylusPlugIn
{
    /// <inheritdoc />
    protected override void OnStylusDown(RawStylusInput rawStylusInput)
    {
        throw new Exception();
    }
}

The OnStylusDown running in Stylus Input thread and if some friend throws any exceptions that will break the thread. And the application will stop responding touch. Of course, the actual situation may be more complicated. Maybe some of my friends just didn't realize that the code he wrote was unstable. But add try catch to all the method is an evil code.

But I do not think we can throw any unintended exceptions in StylusPlugIn.

See StylusPlugIn Class (System.Windows.Input.StylusPlugIns)

If you use a StylusPlugIn inside a control, you should test the plug-in and control extensively to make sure they do not throw any unintended exceptions.

See Intercepting Input from the Stylus

If a StylusPlugIn throws or causes an exception, the application will close. You should thoroughly test controls that consume a StylusPlugIn and only use a control if you are certain the StylusPlugIn will not throw an exception.

@stevenbrix
Copy link
Contributor

@weltkante @walterlv @lindexi thank you for the great discussion on this PR!

My personal feeling on this issue, and I think there is consensus on this thread, is that catching all exceptions and ignoring them is not the right fix for this issue.

As for something analogous to the Application.SetUnhandledExceptionMode in WinForms, does using Application.Current.DispatcherUnhandledException solve your issue, or is there something fundamentally lacking?

@lindexi
Copy link
Member Author

lindexi commented Oct 4, 2019

Thank you @stevenbrix . The core issue is how to fix the touch break when some core throw an exception in the stylus plugin method. Maybe it is the WPF StylusPlugIn API lack a little design. But I do not know how to fix the design.

It will break the stylus input thread when we do not capture the StylusPlugIn exception, so I do not think we can use Application.Current.DispatcherUnhandledExceptio can solve it.

Do you mean to use ExceptionDispatchInfo to capture the exception and rethrow? See Catch and rethrow the exception in StylusPlugIn by lindexi · Pull Request #945 · dotnet/wpf

@stevenbrix
Copy link
Contributor

It will break the stylus input thread when we do not capture the StylusPlugIn exception, so I do not think we can use Application.Current.DispatcherUnhandledException can solve it.

Makes sense since that only catches exceptions on the UI thread.

Do you mean to use ExceptionDispatchInfo to capture the exception and rethrow? See Catch and rethrow the exception in StylusPlugIn by lindexi · Pull Request #945 · dotnet/wpf

@lindexi, that is definitely a better overall solution. Let's continue the discussion on that PR. Do you mind if close out this PR?

@stevenbrix
Copy link
Contributor

@lindexi I'm just going to close this so it doesn't linger. Discussion has hopped over to the original issue you opened #1037

@stevenbrix stevenbrix closed this Oct 4, 2019
@ghost ghost locked as resolved and limited conversation to collaborators Apr 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

PR metadata: Label to tag PRs, to facilitate with triage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Throw IndexOutOfRangeException in WispLogic.CoalesceAndQueueStylusEvent

4 participants