Add test to check GUI thread safety

[HebaruSan]

Background

WinForms is based on some of quite old Windows GUI libraries, from before multithreading was ubiquitous. The .NET implementation inherited a peculiar threading gotcha: All changes to the GUI must be made from the same "GUI thread", or the app will crash or freeze or worse.

Motivation

If you look for ways to deal with this problem, mostly you'll find people recommending Control.InvokeRequired/Control.Invoke, which we already have in the form of CKAN.GUI.Util.Invoke. This is fine when you remember to use it correctly, but it's easy to forget, and when that happens, we get crashes or freezes or worse.

• https://jonskeet.uk/csharp/threads/winforms.html

A few people have created more elaborate systems that wrap the GUI objects in some kind of proxy class that calls Invoke as needed. These generally suffer from poor runtime performance if you use them heavily (since switching threads for every property access requires a lot of marshalling between threads) and awkward maintenance (defining an interface for every GUI object you create, or creating your GUI objects with a third party facility like AOPFactory.Create<MyForm>()). And they still don't solve the problem of what happens if you forget to use them!

• https://osherove.com/blog/2007/5/19/easier-winform-ui-thread-safe-methods-with-dynamicproxy2-and.html
• https://www.codeproject.com/Articles/9597/Making-Windows-Forms-thread-safe

I have wanted for a long time to add a guardrail to catch thread-unsafe GUI code, but my search for a ready-made one never succeeded.

Changes

Now CKAN is the home of the world's first and only static thread-safety checker for WinForms. If you accidentally try to access a GUI object from a background thread, a new test will emit errors like this:

This will finally provide a guardrail to prevent GUI threading problems. It's a static check that only runs as a unit test, so there is zero run time performance impact. In the future we can consider more aggressive refactoring of our GUI thread handling, confident that the test will catch any errors.

How it works

The new test uses Mono.Cecil to load and analyze the compiled GUI assembly instruction-by-instruction. Any function or lambda passed to System.Threading.Tasks.Task.Factory.StartNew or System.Threading.Tasks.Task.Run will be treated as a background thread, and any method or property setter of any class in (or inheriting from anything in) the System.Windows.Forms namespace will be treated as an unsafe GUI method. The test crawls the complete call tree of every background thread looking for calls to the GUI and throws if it finds one. Calling Util.Invoke breaks the chain and allows access to GUI within its argument function, as desired.

But, you exclaim, sometimes we need to access properties or functions of one of our Forms or UserControls from a background thread, and it's perfectly OK because those specific properties or functions don't actually do anything GUI-related! Right you are, and for such cases we can add the new [ForbidGUICalls] attribute to those functions, which does two things:

• Add the tagged function to the top-level list of function that the test checks for GUI calls, to ensure that the declaration of thread safety is in fact true
• Allow other GUI-forbidden functions to call them without triggering a test failure

The functions that our BackgroundWorkers run in the background are also marked with [ForbidGUICalls].

In effect, everything the GUI calls in a background thread now has to be explicitly tagged as such, and nothing so tagged can interact directly with anything GUI-related, so we have achieved reliable thread-safety.

[techman83]

Ok, this is freaking awesome. I love tests that allow us to utilise functionality, while saving us from foot guns!

I have one comment, that I might be missing the forest for the trees for. [ForbidGUICalls] allows us to access properties that are generally GUI thread related, but not make invoke calls? Or do we need to remember to tag all background tasks with this attribute to ensure the checks happen?

It might be a C# nuance that I'm not used to, but the name/description is throwing me. It may just require a little expansion on the description, but it feels like this is done to 'Allow' thread safe property access, rather than prevent non-invoked gui calls (which as noted, some projects do, but has performance impacts).

[HebaruSan]

I have one comment, that I might be missing the forest for the trees for. [ForbidGUICalls] allows us to access properties that are generally GUI thread related, but not make invoke calls? Or do we need to remember to tag all background tasks with this attribute to ensure the checks happen?

No; [ForbidGUICalls] is a pledge that we will not access GUI-thread-only properties. Without that attribute, a call on a usercontrol or form itself is interpreted to be a GUI-thread-only property, but the attribute allows us to add it to the roster of background-safe code.

It might be a C# nuance that I'm not used to, but the name/description is throwing me. It may just require a little expansion on the description, but it feels like this is done to 'Allow' thread safe property access, rather than prevent non-invoked gui calls (which as noted, some projects do, but has performance impacts).

No, it's not done to allow access. It's as it says, GUI calls are forbidden within a function with this attribute.

[techman83]

Ok perfect, that makes sense! Too much swapping between different language reviews for me lately!