Implement SynchronizationContext for GLib #856
Conversation
|
I did not yet take a look at your code will do later. To fix the build pipeline please run |
|
Please do not merge current |
|
I'll comeback later to add unit tests so it is not ready for review yet. |
|
Thanks for updating the PR. Please:
I will do some in depth testing later. |
|
Strange. It was built successfully on my machine. |
|
I found the reason now. I did not run |
|
As this is a fairly new topic for me please be patient with me, while I try to familiarize with the topic and your work, as I'm going to ask questions and having comments.
I'm not sure if the unit test you wrote is correct. I did not take a look into the internals but just thought that your implementation fixes the use case you described in your unit test. So I went ahead and removed the line In regard to code style I would expect at least a comment if you call an |
I also don't have any idea to test this without invoking it directly. Let me know if you want this test.
Same answer as the above.
I'm using Arch Linux. Here is what happens on my machine when I removed the |
|
It also fails reliably on my machine if I remove the synchronization context, though I don't know if it's guaranteed to always run on a different thread without it or if it might still be allowed to run on the main thread. |
|
I think last time I screwed somehow up. Today on my system disabling the Can you add the inverse test, too? This would be the code: [TestMethod]
public void WithoutGLibSynchronizationContextAsyncShouldNotResumeOnMainLoop()
{
SynchronizationContext.SetSynchronizationContext(new SynchronizationContext());
var mainLoop = new MainLoop();
int? resumeThread = null;
AsyncMethod(() =>
{
resumeThread = System.Threading.Thread.CurrentThread.ManagedThreadId;
mainLoop.Quit();
});
mainLoop.Run();
resumeThread.Should().NotBe(System.Threading.Thread.CurrentThread.ManagedThreadId);
}I wanted to go a bit into the topic of exceptions to see if we need to implement something similar to Application.DispatcherUnhandledException. I tried to throw an exception like this: AsyncMethod(() =>
{
throw new Exception();
});But I was not able to catch it. You mentioned to "swallow" the Exception. If this could be done it should be possible to avoid crashing the runtime. If an exception occurs we could let the user gracefully handle the situation. How would you catch those exceptions? |
|
Yeah we need something like For the inverse test there is a problem because |
|
Yeah you are probably right we don't need to test things which are not working. It somehow defeats the purpose of tests. In regard to exception handling I agree that there needs to be another PR but probably not for #845 but for some infrastructure classes which allow to signal an unhandled exception. I would like to land this PR first. Then your PR can use this code already and #845 can use it, too. To be able to test this it would be nice if you could add a comment with the solution to catch an exception. If you want to, you can create this PR, too. Otherwise I would do it. Regarding your statement that this operates on a "global" level it got me thinking. If there is no mainloop the new SynchronizationContext would probably not work? So I wonder if the new SynchronizationContext should have a static method like "Ensure()" which sets it as the context for the current thread. This method should probably not be called in "Module.Initialize" but perhaps in the static constructor of "Gio.Application" and "Glib.MainLoop". To enable it only if the user does something what actually runs a "MainLoop". Perhaps the code must even be run in the regular constructor as there could be different main loops running at the same time. Then we would probably need another test that checks that async calls work even if no explicit SynchronizationContext is set. If the MainLoop call is not called Automatically for "GTK.Application" or "Adw.Application" it needs to be added there, too. What do you think? |
Okay sounds good. Let me know when it is available so I can update my PR.
I think I don't have enough capacity to work on this PR. AFAIK the only way to prevent the exceptions from being crossing the managed boundary is wrapping everything in the callback with try/catch something like this: private static void ScheduleAction(Action action)
{
var proxy = new SourceFuncNotifiedHandler(() =>
{
try
{
action();
}
catch (Exception ex)
{
// Do something with the exception.
}
return false;
});
Internal.Functions.IdleAdd(GLib.Constants.PRIORITY_DEFAULT_IDLE, proxy.NativeCallback, IntPtr.Zero, proxy.DestroyNotify);
}
Right because the callback will never be executed. I was wrong about the race condition on I agree that we should change the way to install the
I think the best place to install it is just before the call to |
|
Yeah this sounds best. But it will be hard to implement. We need to check or create some extension points somehow. I already tried the try / catch block but for some reason it did not work. I hope it was not my ide failing me again. @cameronwhite if you have some capacity could you add some try / catch blocks down the call hierarchy and confirm that an exception is not caught? I really would like to land this as it seems to be some pretty import building block I think. |
|
I just tested and found out
I updated
Can we do it in the public void Run()
{
var previousContext = SynchronizationContext.Current;
SynchronizationContext.SetSynchronizationContext(new GLibSynchronizationContext());
try
{
Internal.MainLoop.Run(Handle);
}
finally
{
SynchronizationContext.SetSynchronizationContext(previousContext);
}
}And did something similar to |
Thanks for testing and sharing. I will evaluate again. Perhaps there is some bug regarding compilation in Rider.
The problem is that the code is autogenerated and the generator does not know that special code should be executed. An easy solution would be to have a method "RunWithSynchronizationContext" and keep the original method alone. |
|
I tested again as we got support for Turns out that putting the try / catch block in the position works. The problem was in my unit test. As i did not quit the main loop in case of an exception the test was running indefinitely. I updated the var previousContext = SynchronizationContext.Current;
SynchronizationContext.SetSynchronizationContext(new MainLoopSynchronizationContext());
try
{
Internal.MainLoop.Run(Handle);
}
finally
{
SynchronizationContext.SetSynchronizationContext(previousContext);
}I updated the private static void ScheduleAction(Action action)
{
var proxy = new SourceFuncNotifiedHandler(() =>
{
try
{
action();
}
catch(Exception ex)
{
UnhandledException.Raise(ex);
}
return false;
});
Internal.Functions.IdleAdd(GLib.Constants.PRIORITY_DEFAULT_IDLE, proxy.NativeCallback, IntPtr.Zero, proxy.DestroyNotify);
}This test is green for me (I renamed [TestMethod]
public void Test()
{
var exceptionCaught = false;
SynchronizationContext.SetSynchronizationContext(new MainLoopSynchronizationContext());
var mainLoop = new MainLoop();
GLib.UnhandledException.SetHandler(e =>
{
exceptionCaught = true;
mainLoop.Quit();
});
AsyncMethod(() =>
{
throw new Exception();
});
mainLoop.Run();
exceptionCaught.Should().BeTrue();
}There is one issue: As a workaround I set the synchronization context at the beginning. I tried to set the synchronization context only in From my point of view this is only a problem of the Unit-Test. Any ideas how to circumvent this problem? If this is fixed I think this PR could be merged if all the discussed changes are applied. |
|
@ultimaweapon I think I worked it out. To get your PR merged please do the following:
private static void ScheduleAction(Action action)
{
var proxy = new SourceFuncNotifiedHandler(() =>
{
try
{
action();
}
catch(Exception ex)
{
UnhandledException.Raise(ex);
}
return false;
});
Functions.IdleAdd(Constants.PRIORITY_DEFAULT_IDLE, proxy.NativeCallback, IntPtr.Zero, proxy.DestroyNotify);
}
using System;
using System.Threading.Tasks;
using FluentAssertions;
using Microsoft.VisualStudio.TestTools.UnitTesting;
namespace GLib.Tests;
[TestClass, TestCategory("IntegrationTest")]
public class SynchronizationContextTest : Test
{
[TestMethod]
public void AsyncMethodIsExecutedOnMainLoopThread()
{
var mainLoop = new MainLoop();
var context = mainLoop.GetContext();
var source = Functions.TimeoutSourceNew(1);
source.Attach(context);
int? resumeThread = null;
source.SetCallback(() =>
{
AsyncMethod(() =>
{
resumeThread = System.Threading.Thread.CurrentThread.ManagedThreadId;
mainLoop.Quit();
});
return false;
});
mainLoop.RunWithSynchronizationContext();
resumeThread.Should().Be(System.Threading.Thread.CurrentThread.ManagedThreadId);
}
[TestMethod]
public void ExceptionInAsyncMethodCanBeHandledViaUnhandledExceptionHandler()
{
var mainLoop = new MainLoop();
var context = mainLoop.GetContext();
var source = Functions.TimeoutSourceNew(1);
source.Attach(context);
source.SetCallback(() =>
{
AsyncMethod(() =>
{
throw new Exception();
});
return false;
});
var exceptionCaught = false;
UnhandledException.SetHandler(e =>
{
exceptionCaught = true;
mainLoop.Quit();
});
mainLoop.RunWithSynchronizationContext();
exceptionCaught.Should().BeTrue();
}
private static async void AsyncMethod(Action finish)
{
await Task.Delay(1);
finish();
}
} |
|
Sorry for late response. I'm having some personal problems so I can't working on this until the end of next week. If you are in a hurry you can take over this PR. |
|
Done. Let me know if there is any problem. |
Closes #854
No unit tests yet. Need some feedback before adding a unit tests.