0

I have wpf app. I have 3 labels, 1 button , (Start/Stop). I have databinding. My MainWindowViewModel loooks like beneath:

namespace StartStop
{
    public class MainWindowViewModel : BindableBase
    {
        private Action<Action> RunOnUI;
        CancellationTokenSource source;
        public DelegateCommand StartCommand { get; set; }
        private string startButtonText = "START";
        public string StartButtonText
        {
            get { return startButtonText; }
            set { SetProperty(ref startButtonText, value); }
        }
        private string loop1Value ;
        public string Loop1Value
        {
            get { return loop1Value; }
            set { SetProperty(ref loop1Value, value); }
        }
        private string loop2Value;
        public string Loop2Value
        {
            get { return loop2Value; }
            set { SetProperty(ref loop2Value, value); }
        }
        private string loop3Value;
        public string Loop3Value
        {
            get { return loop3Value; }
            set { SetProperty(ref loop3Value, value); }
        }
        public MainWindowViewModel(Action<Action> runOnUI)
        {
            RunOnUI = runOnUI;
            StartCommand = new DelegateCommand(MainHandler);
        }

        async void MainHandler()
        {
            if (StartButtonText == "START")
            {
                StartButtonText = "STOP";

                try
                {
                    source = new CancellationTokenSource();
                    await Task.Run((Action)Loop1);
                }
                catch (OperationCanceledException)
                {
                    MessageBox.Show("Test was aborted by user");
                }
                finally
                {
                    StartButtonText = "START";
                    source = null;
                }
            }
            else
            {
                source = null;
                StartButtonText = "START";
            }
        }

        private void Loop1()
        {
            for(int i=0;i<100;i++)
            {
                Loop1Value = "Loop1: " + i.ToString();
                Sleep(1000, source.Token);
                Loop2();
            }
        }

        private void Loop2()
        {
            for (int i = 0; i < 100; i++)
            {
                Loop2Value = "Loop2: " + i.ToString();
                Sleep(1000, source.Token);
                Loop3();
            }
        }

        private void Loop3()
        {
            for (int i = 0; i < 100; i++)
            {
                Loop3Value = "Loop3: " + i.ToString();
                Sleep(1000, source.Token);
            }
        }
        private static void Sleep(int millisecondsTimeout, CancellationToken cancellationToken)
        {
            Task.WaitAny(Task.Delay(millisecondsTimeout, cancellationToken));
        }
    }
}

Normally I have more advanced code and in Loops i have serial communication (rs232), I simplified code to show my problem. When I press Stop button I want the thread stop (exit all loops). I tried to throw exception when Stop is pressed, buy setting source=null, but code doesn't go to catch. Why?

gregor
  • 1
  • 2
  • 1
    End the loops when `source.Token.IsCancellationRequested` is true? Besides that, you should be using `await` to await any awaitable method call. – Clemens Jan 20 '23 at 09:53
  • 1
    have you tried calling `source.Cancel();` rather than setting `source=null` ? note, however, that the "sync-over-async" sleep is a very ... unusual (and inefficient) way of doing that – Marc Gravell Jan 20 '23 at 09:54
  • when i set source.Cancel(), there is no error i app ends fast, because i app didn't wait in Sleep function. But when I have communication RS232, occurs errors. The best will be to go to catch (OperationCanceledException). – gregor Jan 20 '23 at 09:59
  • 1
    Sounds like [x-y](https://meta.stackexchange.com/questions/66377/what-is-the-xy-problem) to me. What are you _actually_ trying to do with this? – Fildor Jan 20 '23 at 10:11

1 Answers1

1

Change your loop method to

private void Loop1(CancellationToken token)
{
     for(int i=0;i<100;i++)
     {
          token.ThrowIfCancellationIsRequested();
          Loop1Value = "Loop1: " + i.ToString();
          Sleep(1000, token.Token);
          Loop2(token);
     }
}

Do the same with all other loops. Also change your code for stopping to source.Cancel()

I would assume the Thread.Sleep is a stand in for reading a serial-port. Unfortunately SerialPort.Read does not seem to provide a overload with cancellation support. I'm not really faimilliar with serial ports, so yo might want to read Async Serial Port with CancellationToken and ReadTimeout. Or search for better libraries. Or if nothing else works, set a low timeout and catch the timeout exceptions.

I would also recommend to avoid mutable shared state when dealing with multiple threads. In this case it would mean to take the cancellation token and any other inputs as method parameters, and return some result. Avoid using any mutable fields. Updating the Loop1Value from a background thread looks like a very likely bug to me.

If you do not care if the operation was canceled or completed you could instead check the IsCancellationRequested property and simply return.

I want the thread stop

You need to use cooperative cancellation. Trying to stop a thread non-cooperativly is just not a good idea. See What's wrong with using Thread.Abort() for details.

JonasH
  • 28,608
  • 2
  • 10
  • 23
  • Why throw when you're in a loop that you can gracefully exit? – Enigmativity Jan 20 '23 at 10:21
  • 1
    @Enigmativity For example if you write library code you don't know how the callers (up the call stack) handles cancellation. You throw to allow to notify everybody about the cancellation event. – BionicCode Jan 20 '23 at 10:32
  • 1
    @Enigmativity because otherwise you might not be able to tell if the loop completed the work it was supposed to do, or if it was cancelled prematurely. [This article](https://blog.stephencleary.com/2022/03/cancellation-4-polling.html) should explain it in greater detail. – JonasH Jan 20 '23 at 10:37
  • @JonasH - It also says "I only do this when testing indicates that the code does not respect cancellation". I feel that using exceptions for code control is a last resort. [This article from Eric Lippert](https://ericlippert.com/2008/09/10/vexing-exceptions/) gives a great discussion on when to use exceptions. – Enigmativity Jan 20 '23 at 10:50
  • It's not "to notify everybody". It's more like "to notify anybody". If you're not catching your own exception then anybody can and you have no idea how they handle them. – Enigmativity Jan 20 '23 at 10:51
  • @Enigmativity there is no need to know how the caller will handle the exception. In fact if you knew that, you would handle it yourself. When you don't know how to handle an exception, you should propagate it to the caller, and let the caller deal with it. The cancellation is communicated as an exception in .NET since the introduction of the `CancellationToken` at 2010 (.NET Framework 4.0). Now it's too late to change it. – Theodor Zoulias Jan 20 '23 at 11:12
  • @JonasH In your solution when I press Stop, appears Exception: System.OperationCanceledException: 'The operation was canceled., and code stops here. How to handle that exception? – gregor Jan 20 '23 at 11:27
  • @TheodorZoulias - "In fact if you knew that, you would handle it yourself." - that's hyperbole. – Enigmativity Jan 20 '23 at 11:33
  • @TheodorZoulias - You can just check if cancel is requested and exit gracefully. No need to throw. And then there's no need to know how to handle the exception. It becomes moot. – Enigmativity Jan 20 '23 at 11:35
  • @JonasH Instead of `token.ThrowIfCancellationRequested();` I inserted `if token.IsCancellationRequested) return;` , because I don't know how to handle exception. – gregor Jan 20 '23 at 11:46
  • 1
    @Gregor I would expect the exception to be caught by the `catch (OperationCanceledException)`. Is this not the case? If running in the debugger you should probably turn off break on exception for that type. – JonasH Jan 20 '23 at 11:57
  • 1
    @Enigmativity "I only do this when testing indicates that the code does not respect cancellation" is specifically for *when* to poll, the advice is still to use `ThrowIfCancellationRequested` to poll. I completely agree using exceptions for flow control in this way is counter to most advice, but this is the pattern Microsoft selected. And it is probably easier to follow the common pattern instead of inventing something new. Again, assuming you actually care if the method was cancelled or not. – JonasH Jan 20 '23 at 12:19
  • @Enigmativity *"You can just check if cancel is requested and exit gracefully. "* -- So you say that all these thousands of APIs that accept cancellation tokens and throw exceptions are not exiting gracefully? If so what will you do about it? Are you going to fork the .NET runtime, and fix all these APIs? Btw [graceful exit](https://en.wikipedia.org/wiki/Graceful_exit) doesn't mean no exceptions. It means exiting in a controlled manner. Exiting with an error message is perfectly graceful. Non-graceful means exiting without providing info about whether the work completed successfully or not. – Theodor Zoulias Jan 20 '23 at 12:34
  • @JonasH You was right. When I run app (exe). It works ok. MessageBox apppears and show error. But how to turn off break on exception for that type? – gregor Jan 20 '23 at 12:57
  • @gregor in Visual studio there should be a checkbox in the popup dialog that you can uncheck, "break when this exception type is thrown". You can also open the 'Exception Settings' window from the debug/window menu, and search for the exception type and uncheck it, or just uncheck all exceptions. – JonasH Jan 20 '23 at 13:27
  • @gregor you can also run the solution without the debugger attached, with the shortcut Ctrl+F5. This is my favorite shortcut. – Theodor Zoulias Jan 20 '23 at 15:19