View Revisions: Issue #1644

Summary 0001644: Dispatcher issues
Revision 2020-03-24 14:44 by ai_enabled
Description Hi guys,

first, this operation.Invoke() call https://github.com/Noesis/Managed/blob/782acc60bf2f2befbab0d701a3e3d5bd467859ab/Src/Noesis/Core/Src/Core/Dispatcher.cs#L137 will crash the process if there is an exception because it's not handled.
I suggest changing it to:
                    try
                    {
                        operation.Invoke();
                    }
                    catch (OperationCanceledException)
                    {
                        // that's fine
                    }
                    catch (Exception)
                    {
                        // TODO: add logging
                    }

(the issue happened to me)

Second, NoesisGUI C# SDK 2.2.6 setting up the synchronization context here https://github.com/Noesis/Managed/blob/ce47687f17cb6c66c93b1151154d906ff9cf8d23/Src/Noesis/Core/Src/Core/NoesisGUI.cs#L58
this is conflicting with our own "main thread" SynchronizationContext . I'm not sure if you actually intended to set your SynchronizationContext there. For my branch, I've just removed this line, but I think you need to pay extra attention as it might lead to unexpected consequences for your customers. You cannot make an assumption that the main thread (with its SynchronizationContext) completely belongs to NoesisGUI. Setting your SynchronizationContext temporary is fine but implicitly enforcing it is not a good idea from my experience.

Regards!
Revision 2020-03-24 14:44 by ai_enabled
Description Hi guys,

first, this operation.Invoke() call https://github.com/Noesis/Managed/blob/782acc60bf2f2befbab0d701a3e3d5bd467859ab/Src/Noesis/Core/Src/Core/Dispatcher.cs#L137 will crash the process if there is an exception because it's not handled.
I suggest changing it to:
                    try
                    {
                        operation.Invoke();
                    }
                    catch (OperationCanceledException)
                    {
                        // that's fine
                    }
                    catch (Exception)
                    {
                        // TODO: add logging
                    }

(the issue happened to me)

Second, NoesisGUI C# SDK in 2.2.6 setting up the synchronization context here https://github.com/Noesis/Managed/blob/ce47687f17cb6c66c93b1151154d906ff9cf8d23/Src/Noesis/Core/Src/Core/NoesisGUI.cs#L58
this is conflicting with our own "main thread" SynchronizationContext . I'm not sure if you actually intended to set your SynchronizationContext there. For my branch, I've just removed this line, but I think you need to pay extra attention as it might lead to unexpected consequences for your customers. You cannot make an assumption that the main thread (with its SynchronizationContext) completely belongs to NoesisGUI. Setting your SynchronizationContext temporary is fine but implicitly enforcing it is not a good idea from my experience.

Regards!
Revision 2020-03-24 14:43 by ai_enabled
Description Hi guys,

first, this operation.Invoke() call https://github.com/Noesis/Managed/blob/782acc60bf2f2befbab0d701a3e3d5bd467859ab/Src/Noesis/Core/Src/Core/Dispatcher.cs#L137 will crash the process if there is an exception because it's not handled.
I suggest changing it to:
                    try
                    {
                        operation.Invoke();
                    }
                    catch (OperationCanceledException)
                    {
                        // that's fine
                    }
                    catch (Exception)
                    {
                        // TODO: add logging
                    }

(the issue happened to me)

Second, NoesisGUI C# SDK in 2.2.6 setting up the synchronization context here https://github.com/Noesis/Managed/blob/ce47687f17cb6c66c93b1151154d906ff9cf8d23/Src/Noesis/Core/Src/Core/NoesisGUI.cs#L58
this is conflicting with our own "main thread" dispatcher. I'm not sure if you actually intended to set your SynchronizationContext there. For my branch, I've just removed this line, but I think you need to pay extra attention as it might lead to unexpected consequences for your customers. You cannot make an assumption that the main thread (with its SynchronizationContext) completely belongs to NoesisGUI. Setting your SynchronizationContext temporary is fine but enforcing it is not a good idea from my experience.

Regards!