crc-org / tray-windows

CodeReady Containers tray for windows
Apache License 2.0
2 stars 4 forks source link

Issue #92 Using Exception around each call to determine success #94

Closed gbraad closed 3 years ago

gbraad commented 3 years ago

This uses try/catch to handle issues from the requests made. AggregateException needs to be handle to pick the known ones which complicate the codebase, as this is required by the async pattern if not using the result messages.

gbraad commented 3 years ago

@anjannath do you have 'time' to have a quick glance at this? I only tested a quick start, delete and status. It should be OK, but when you run in debug, you HAVE to disable the 'unhandled exceptions' else you will be flooded with CrcApiException and AggregateExceptions in the getResponse. This can't be easily solved as this the .Result being read from the task.

gbraad commented 3 years ago

FYI, already had Start throw a TaskCancelledException once by timing out. This means that the AggregateException in the case of start also needs to check and handle the TaskCancelled/Timeout by 'ignoring' it. Or we set the timeout value of this request extremely long? :-/ Would rather detect this Started state by the StatusChanged event, but that would also mean that none of the request should actually provide their own notifications as you do not want to see duplicates.

gbraad commented 3 years ago

Set and Unset config still use an Error to determine if the settings were applied or not. This needs a backend change

anjannath commented 3 years ago

One question, why is there an AggregateException only in some cases and not all?

gbraad commented 3 years ago

Some refactoring can be done to get rid of the try/catch pattern:

            try
            {
                await Task.Run(Func<T>);
                TrayIcon.NotifyInfo(@"Task ended successfully");
            }
            catch (AggregateException ae)
            {
                ae.Handle((x) =>
                {
                    if (x is APIException) // This we know how to handle.
                    {
                        TrayIcon.NotifyError(
$@"Error returned.
{x.Message}");
                        return true;
                    }

                    if (x is TaskCanceledException)
                    {
                        TrayIcon.NotifyInfo(@"Task canceled");
                        return true;
                    }

                    return false;

                });
            }
gbraad commented 3 years ago

One question, why is there an AggregateException only in some cases and not all?

Since not in all case this works yet... Some of the return values are interpreted, while others rely on the 500 status. In a refactoring procesd for this.

gbraad commented 3 years ago

Added a big refactor, but unable to test properly now. I do not get any notifications at the moment ... feels like a glitch in the OS.

anjannath commented 3 years ago

I get an unhandled exception at the end of Start, this is the release build. Its from the httpOverStream library it seems

An unhandled exception of type 'System.Reflection.TargetInvocationException' occurred in mscorlib.dll
Exception has been thrown by the target of an invocation.

image

gbraad commented 3 years ago

Will check what might cause this... not checking for nulls anymore but exception(s). Perhaps this is actually caused by the timeout/canceled call (which explains the HttpOverStream) and pops an empty message