IsRunning = True even if errors occur

[JoE-Seeve]

If the source directory does not exist during a copy process, IsRunnung is set to True beforehand. Since the source directory is only checked after this, IsRunning must be set to False again in the event of an error. Since this is missing, IsRunning remains true even after the copy attempt has been completed.

[PCAssistSoftware]

@JoE-Seeve - should be resolved by #107 - please confirm okay and will then close - thanks

[JoE-Seeve]

I tested the changes with a non-existent source and target directory and found that IsRunning is not set to FALSE if the command is aborted due to an error.

In the original code (with testing source directory, and not testing the target directory), I just added one line here and it worked. But since the handling of IsRunning has changed, I can no longer really understand when IsRunning is set to FALSE.

        // make sure source path is valid
        if (!Directory.Exists(CopyOptions.Source))
        {
            Debugger.Instance.DebugMessage("The Source directory does not exist.");
            hasError = true;
            OnCommandError?.Invoke(this, new CommandErrorEventArgs("The Source directory does not exist."));
            Debugger.Instance.DebugMessage("RoboCommand execution stopped due to error.");
            tokenSource.Cancel(true);

            // Added by JoE - Running was set even if the process wasn't started due to file error
            isRunning = false;
            //
        }

[PCAssistSoftware]

Thanks @JoE-Seeve - will noted - think we jsut need to add isRunning = false; after line 273 and 289 but will wait for others to comment in case changes to handling of isRunning I have missed

[JoE-Seeve]

OK, but please have in mind that the code for testing source and target has changed and it seems like that additional lines with "IsRunning = False" are necessary.

[PCAssistSoftware]

Yes I am fully aware, hence waiting for others to comment first

[PCAssistSoftware]

@RFBomb - thoughts on this please?

[RFBomb]

In #107, I had made a change to the STOP() function in order to more reliably set the isRunning field. I had thought this would take care of this issue, but I now see it didn't (hence my note for needing testing).

What I hadn't noticed is the cancellationToken.ThrowIfCancellationRequested(); call in the backup task factory section. Since this occurs before generating the backup task, the Stop() method never actually runs.

I think the cleanest way to ensure that isRunning is set when the cancellation token is requested here is putting this line above the ThrowIfCancellation line:

isRunning = !cancellationToken.IsCancellationRequested;

All of the checks prior to this point are setting the cancellation token during failure, so this will set the bool prior to throwing the cancellation (which is why the stop() method was never called - it was thrown prior to starting the task that calls stop() )

I'll put in a PR to fix that if you want

[PCAssistSoftware]

Thanks @RFBomb

So just to confirm you are referring to line 328 of RoboCommand.cs and changing it from

cancellationToken.ThrowIfCancellationRequested();
process = new Process();

to

isRunning = !cancellationToken.IsCancellationRequested;
cancellationToken.ThrowIfCancellationRequested();
process = new Process();

@JoE-Seeve - can you manually make this change your end and test it works for you as expected and let me know?

[RFBomb]

That was the line I was referring to, yes
With that in place, we shouldn't have to worry about setting the isRunning=false flag elsewhere in the start() method.
(Prior to this, its set true and shouldn't exit until that line. After that line, I believe the stop() function will be called by the resulting process/task)

[PCAssistSoftware]

That make sense to me, will let @JoE-Seeve test it as he reported it and then get it implemented if okay :)

You can put a PR if you want - or if not I will make the change to the code manually and reference it to this discussion

[JoE-Seeve]

I've made the changes described, but it doesn't seem to work. RoboSharp continues to report IsRunning = True if errors occur in the source or target directories.

        backupTask = Task.Factory.StartNew(() =>
        {
            //cancellationToken.ThrowIfCancellationRequested();
            //process = new Process();
            isRunning = !cancellationToken.IsCancellationRequested;
            cancellationToken.ThrowIfCancellationRequested();
            process = new Process();

[PCAssistSoftware]

Thanks, I'll wait for @RFBomb to come back on this, but from my quick look an error occurring with source or target directories doesn't throw cancellation request as it is an exception rather than a cancellation.

[RFBomb]

Only the CreateDirectory portion should be throwing the error, Directory.Exists and DirectoryInfo.Exists shouldn't be throwing errors. Even then the Error is caught and reacted to, so I would imagine it should process it just fine.
My suggestion was based on the tokenSource.Cancel(true); commands in the program.

I'll do a quick test to see if I can get it to fail and react.

Sidenote, this is my first time testing using the BackupApp. Why does it need VS running in admin mode to debug?

[RFBomb]

@JoE-Seeve this is definitely an odd bug. I was able to reproduce and catch it though by modifying the 'Backup' button's code for the backup app.

            copy.Start();
            while (copy.IsRunning)
            {
                System.Threading.Thread.Sleep(100);
                DoEvents();
            }
        }

        public void DoEvents()
        {
            System.Windows.Threading.DispatcherFrame frame = new System.Windows.Threading.DispatcherFrame();
            System.Windows.Threading.Dispatcher.CurrentDispatcher.BeginInvoke(System.Windows.Threading.DispatcherPriority.Background,
                new System.Windows.Threading.DispatcherOperationCallback(ExitFrame), frame);
            System.Windows.Threading.Dispatcher.PushFrame(frame);
        }

        public object ExitFrame(object f)
        {
            ((System.Windows.Threading.DispatcherFrame)f).Continue = false;

            return null;
        }

This will allow the error windows to show, but it never actually leaves the loop. Oddly enough, I also can't seem, to hit the breakpoints I set within the STOP() function, not the breakpoint for the IsRunning check. Even without the while-loop, it never stopped on those breakpoints.

But I found out why once I created a valid test. Since the cancellation token is requesting a cancel before the process attached to it even starts, NONE OF THE CODE IS HIT. As in, the TaskFactory never even starts processing the task. If the cancel is not requested, I hit all my breakpoints just fine, including the cancellationToken.ThrowIfCancellationRequested(); breakpoint.

So it appears that my suggestion didn't work because the task factory saw the cancel request and basically just didn't do anything. As you can see below, it returned a null method due to cancellation request. This in turn forced the continueWithTask to be a null method. This is why Stop() is never triggering.

Simple fix: move
isRunning = !cancellationToken.IsCancellationRequested;
above
backupTask = Task.Factory.StartNew(() =>

(I'm glad I looked into this, because it may reduce some headache when troubleshooting my stuff down the road - thats actually an interesting optimization the compiler does by returning a null method)

[PCAssistSoftware]

Sidenote, this is my first time testing using the BackupApp. Why does it need VS running in admin mode to debug?

To avoid file permission errors I suppose, I always run my GUI for RoboSharp with admin rights for that reason. Can easily be turned off by editing app.manifest and replacing "requireAdministrator" with "asInvoker"