9

We have built a huge winforms project, already in progress for multiple years.

Sometimes, our users get an exception which looks like this one.

The resolution of this problem seems to be:

don't acces UI components from a background thread

.

But since our project is a very big project with a lot of different threads, we don't succeed in finding all these.

Is there a way to check (with some tool or debugging option) which components are called from a background thread?

To clarify:

I created a sample winforms project with a single Form, containing two Button

public partial class Form1 : Form
{
    public Form1()
    {
        InitializeComponent();
    }

    private void button1_Click(object sender, EventArgs e)
    {
        button1.Text = "Clicked!";
    }

    private void button2_Click(object sender, EventArgs e)
    {

        Task.Run(() =>
        {
            button2.BackColor = Color.Red; //this does not throw an exception
            //button2.Text = "Clicked"; //this throws an exception when uncommented
        });
    }
}

The background color of button2 is set to red when the button is clicked. This happens in a background thread (which is considered bad behavior). However, it doesn't (immediately) throw an exception. I would like a way to detect this as 'bad behavior'. Preferably by scanning my code, but if it's only possible by debugging, (so pausing as soon as a UI component is accessed from a background thread) it's also fine.

Fortega
  • 19,463
  • 14
  • 75
  • 113
  • What you're showing is a `NullReferenceException`. Doesn't seem to be a UI thread issue. – doubleYou Jan 05 '18 at 12:44
  • 1
    It may be tedious but you should go through your code base and use `InvokeRequired` as needed. – Crowcoder Jan 05 '18 at 12:58
  • @doubleYou I'm pretty sure this null reference is caused by a UI thread issue. See also the answers to the question I linked. However, that doesn't even really matter, as I'm not asking to solve this null reference directly. – Fortega Jan 05 '18 at 12:58
  • @Crowcoder that is indeed what we are doing now, but we are missing a (or maybe more) call. And with this huge codebase, I was wondering if there was a better way than going through each line of code. – Fortega Jan 05 '18 at 12:59
  • 1
    @ close voter: can you please specify why you think this question is out of scope? – Fortega Jan 05 '18 at 13:01
  • Just to understand @Fortega and sorry if it's obvious, but this error occur when you are debugging? cause it would stop at the line where it happens :-\ right? – Zorkind Jan 05 '18 at 13:02
  • Can we assume you are working with some recent version of Visual Studio? – Fildor Jan 05 '18 at 13:02
  • @Zorkind It does happen during debugging. But the stacktrace always originates on the UI thread. If I understand it well, the object is accessed in a background thread, but this does not throw an error. When a text size is calculated in the UI thread after this, the error is thrown (but not always). – Fortega Jan 05 '18 at 13:08
  • @Fildor Visual Studio 2017 Professional – Fortega Jan 05 '18 at 13:11
  • If we could have some code, at least. :-\ i understand you don't know which code would be useful, but, we would have to be guessing, as it is. – Zorkind Jan 05 '18 at 13:18
  • 1
    @Zorkind I added some code for clarification – Fortega Jan 05 '18 at 13:53
  • I see, you want some sort of tool for code reviewing. ReSharp is the first thing that comes to mind. – Zorkind Jan 05 '18 at 14:04
  • @Zorkind I have Resharper Ultimate installed, but I don't see how this can help me at the moment... – Fortega Jan 05 '18 at 14:13
  • Well i thought it could have a setting for that kind of bad code. Searching for threads trying to access the UI thread's components. I guess you would have to make your own then. – Zorkind Jan 05 '18 at 14:23
  • If you handle the AppDomain.CurrentDomain.UnhandledException, you don't get a better clue as to what code is throwing the exception? – Brendon Jan 05 '18 at 15:08
  • I'm not sure I understand why debugging doesn't catch all exceptions? Or do you mean you also want to catch things that *don't* throw exceptions (like the back color thing)? why? and how could you do that if it's not an error, beyond reviewing the code manually? – Simon Mourier Jan 08 '18 at 11:29
  • @SimonMourier he wants to preemptively find code mistakes, like calling an UI thread object from another thread, beforehand, in an automated manner, because he have too much code to go through looking for this mistake, and since this is not a mistake that the compiler catches, he want something that does this analysis of the code for him. – Zorkind Jan 08 '18 at 11:55
  • what i think you could do is some sort of text search, getting all the components created in a class that inherits a specific UI class, and search for points in the class's text where the proprieties of this class is called from within a thread block. Not something easy to do, i know. :-\ – Zorkind Jan 08 '18 at 12:01
  • Guys, this doesn't deserve downvotes IMHO, wouldn't a compilation warning for this be good? So many newbies get caught with this, and most only hear about it when their software is out in the wild. Also, OP has 13k rep, he's helped people and the question is *not* out of scope. It's just hard. – Jeremy Thompson Jan 09 '18 at 07:26
  • Not calling OP a newbie, he's the opposite of that! – Jeremy Thompson Jan 09 '18 at 07:37
  • @JeremyThompson Thanks for the heads up :) Although I would not base a downvote on the rep of the OP, I don't see why this question would be out of scope either. – Fortega Jan 11 '18 at 09:40
  • Did you end up finding the problem? – Jeremy Thompson Jan 17 '18 at 05:51
  • @JeremyThompson I found a few problems while debugging, but I don't have a structural solution (yet)... – Fortega Jan 18 '18 at 14:21

5 Answers5

4

I've got 2 recommendations to use together, the first is a Visual Studio Plugin called DebugSingleThread.

You can freeze all the threads and work on one at a time (obviously the non-main-UI threads) and see each threads access to controls. Tedious I know but not so bad with the second method.


The second method is to get the steps in order to reproduce the problem. If you know the steps to reproduce it, it will be easier to see whats causing it. To do this I made this User Action Log project on Github.

It will record every action a user makes, you can read about it here on SO: User Activity Logging, Telemetry (and Variables in Global Exception Handlers).

I'd recommend you also log the Thread ID, then when you have been able to reproduce the problem, go to the end of the log and work out the exact steps. Its not as painful as it seems and its great for getting application telemetry.

You might be able to customise this project, eg trap a DataSource_Completed event or add a dummy DataSource property that sets the real Grids DataSource property and raises an INotifyPropertyChanged event - and if its a non-main thread ID then Debugger.Break();.


My gut feeling is you're changing a control's (eg a grid) data source in a background thread (for that non-freeze feel) and thats causing a problem with synchronisation. This is what happened to the other DevExpress customer who experienced this. Its discussed here in a different thread to the one you referenced.

Jeremy Thompson
  • 61,933
  • 36
  • 195
  • 321
3

Is your app set to ignore cross threading intentionally?

Cross-thread operations should be blowing up all the time in winforms. It checks for them like crazy in just about every method. for a starting point check out https://referencesource.microsoft.com/#System.Windows.Forms/winforms/Managed/System/WinForms/Control.cs.

Somewhere in your app, somebody might have put this line of code:

Control.CheckForIllegalCrossThreadCalls = False;

Comment that out and run the app, then follow the exceptions.

(Usually you can fix the problem by wrapping the update in an invoke, e.g., in a worker thread if you see textbox1.text=SomeString; change it to `textbox.invoke(()=>{textbox1.text=SomeString;});.

You may also have to add checking for InvokeRequired, use BeginInvoke to avoid deadlocks, and return values from invoke, those are all separate topics.

this is assuming even a moderate refactor is out of the question which for even a medium sized enterprise app is almost always the case.

Note: it's not possible to guarantee successful discovery of this case thru static analysis (that is, without running the app). unless you can solve the halting problem ... https://cs.stackexchange.com/questions/63403/is-the-halting-problem-decidable-for-pure-programs-on-an-ideal-computer etc...

FastAl
  • 6,194
  • 2
  • 36
  • 60
  • This is wrong if you set `CheckForIllegalCrossThreadCalls = False;` it makes it harder to determine the problem. Unpredictable things happen and to prove the point; the stacktrace would be different. – Jeremy Thompson Jan 10 '18 at 23:22
  • The CheckForIllegalCrossThreadCalls property is never called. – Fortega Jan 11 '18 at 08:56
  • Thanks for pointing me to the Halting problem. I am familiar with this, but I don't see how this problem can be related to the Halting problem. Can you ellaborate a bit? – Fortega Jan 11 '18 at 08:57
  • Regarding halting problem - it isn't. But it's impossible to solve. It's also impossible to reliably discover this type of problem without running the code (that is what I meant by static analysis). Sorry about the red herring, I am just confirming here that you need to run the app to discover the cross threading (I'm pretty sure ;-) Now there are problems with that, too - how do you cover every path? That is a huge challenge, too. I am sure you are feeling that pain already. – FastAl Jan 11 '18 at 19:59
2

I did this to search for that specific situation but of course, need to adjust it to your needs, but the purpose of this is to give you at least a possibility.

I called this method SearchForThreads but since it's just an example, you can call it whatever you want.

The main idea here is perhaps adding this Method call to a base class and call it on the constructor, makes it somewhat more flexible.

Then use reflection to invoke this method on all classes deriving from this base, and throw an exception or something if it finds this situation in any class.

There's one pre req, that is the usage of Framework 4.5. This version of the framework added the CompilerServices attribute that gives us details about the Method's caller.

The documentation for this is here

With it we can open up the source file and dig into it.

What i did was just search for the situation you specified in your question, using rudimentary text search.

But it can give you an insight about how to do this on your solution, since i know very little about your solution, i can only work with the code you put on your post.

public static void SearchForThreads(
        [System.Runtime.CompilerServices.CallerMemberName] string memberName = "",
        [System.Runtime.CompilerServices.CallerFilePath] string sourceFilePath = "",
        [System.Runtime.CompilerServices.CallerLineNumber] int sourceLineNumber = 0)
        {
            var startKey = "this.Controls.Add(";
            var endKey = ")";

            List<string> components = new List<string>();

            var designerPath = sourceFilePath.Replace(".cs", ".Designer.cs");
            if (File.Exists(designerPath))
            {
                var designerText = File.ReadAllText(designerPath);
                var initSearchPos = designerText.IndexOf(startKey) + startKey.Length;

                do
                {
                    var endSearchPos = designerText.IndexOf(endKey, initSearchPos);
                    var componentName = designerText.Substring(initSearchPos, (endSearchPos - initSearchPos));
                    componentName = componentName.Replace("this.", "");
                    if (!components.Contains(componentName))
                        components.Add(componentName);

                } while ((initSearchPos = designerText.IndexOf(startKey, initSearchPos) + startKey.Length) > startKey.Length);
            }

            if (components.Any())
            {
                var classText = File.ReadAllText(sourceFilePath);
                var ThreadPos = classText.IndexOf("Task.Run");
                if (ThreadPos > -1)
                {
                    do
                    {
                        var endThreadPos = classText.IndexOf("}", ThreadPos);

                        if (endThreadPos > -1)
                        {
                            foreach (var component in components)
                            {
                                var search = classText.IndexOf(component, ThreadPos);
                                if (search > -1 && search < endThreadPos)
                                {
                                    Console.WriteLine($"Found a call to UI thread component at pos: {search}");
                                }
                            }
                        }
                    }
                    while ((ThreadPos = classText.IndexOf("Task.Run", ++ThreadPos)) < classText.Length && ThreadPos > 0);
                }
            }
        }

I hope it helps you out.

You can get the Line number if you split the text so you can output it, but i didn't want to go through the trouble, since i don't know what would work for you.

string[] lines = classText.Replace("\r","").Split('\n');
Zorkind
  • 607
  • 12
  • 21
  • what if its called with someStaticClass.StaticMethod(Button btn){ Task.Run()} – Steve Jan 08 '18 at 22:35
  • Yes i ser your point. Thats why i said i made this to work with his specific needs. I am sure it can be adapted for diferent cenarios, but anyhow it would be problematic to access a control in a static ui class, i dont see this happening. I am not assuming, i just really dont know if that would happen in his code base – Zorkind Jan 08 '18 at 22:39
  • @Zorkind I don't fully understand this method. Should I call it in the constructor of each (winform) component I use? As I don't have a base class for a lot of components, it would mean I have to call it from a lot of different places? Just to make sure I'm understanding it correctly – Fortega Jan 11 '18 at 09:16
  • @Zorkind it looks like it is checking the code in a Task.Run() call. But in this code, there be a lot of other calls, passsing the winforms control to other methods... – Fortega Jan 11 '18 at 09:37
  • The method itself you can put in your static program.cs, bjt you have to call it in each of your forms constructor yes. – Zorkind Jan 11 '18 at 10:52
  • or we could improve this making it find the forms for us, remember tht my intention here is not to hand you a complete solution, but to point you in a direction, since i dont know your code. – Zorkind Jan 11 '18 at 10:53
  • like i said in my post @Fortega i can only work with what i can see. i made it look for Task.Run because its what you gave us in your post, this is meant to be an example. – Zorkind Jan 11 '18 at 10:55
  • Perhaps you can make a code that open up all your form cs files and place the call to this method on the constructor using rudimentary text search. – Zorkind Jan 11 '18 at 10:59
1

Try that:

public static void Main(string[] args)
{
    // Add the event handler for handling UI thread exceptions to the event.
    Application.ThreadException += new ThreadExceptionEventHandler(exception handler);

    // Set the unhandled exception mode to force all Windows Forms errors to go through the handler.
    Application.SetUnhandledExceptionMode(UnhandledExceptionMode.CatchException);

    // Add the event handler for handling non-UI thread exceptions to the event. 
    AppDomain.CurrentDomain.UnhandledException += // add the handler here

    // Runs the application.
    Application.Run(new ......);
}

Then you can log the message and the call stack and that should give you enough information to fix the issue.

Radin Gospodinov
  • 2,313
  • 13
  • 14
  • This is incorrect. See my answer and method 2. You cannot log the message in those events, its too late, the stack has been unwound. – Jeremy Thompson Jan 10 '18 at 23:18
  • That does not work, @Radin. The stack trace we can see is always originating from the UI thread, while the underlying cause is a call from a background thread – Fortega Jan 11 '18 at 08:52
1

I recommend you update your GUI to handle this situation automatically for your convenience. You instead use a set of inherited controls.

The general principle here is to override the property Set methods in a way to make them Thread Safe. So, in each overridden property, instead of a straight update of the base control, there's a check to see if an invoke is required (meaning we're on a separate thread the the GUI). Then, the Invoke call updates the property on the GUI thread, instead of the secondary thread.

So, if the inherited controls are used, the form code that is trying to update GUI elements from a secondary thread can be left as is.

Here is the textbox and button ones. You would add more of them as needed and add other properties as needed. Rather than putting code on individual forms.

You don't need to go into the designer, you can instead do a find/replace on the designer files only. For example, in ALL designer.cs files, you would replace System.Windows.Forms.TextBox with ThreadSafeControls.TextBoxBackgroundThread and System.Windows.Forms.Button with ThreadSafeControls.ButtonBackgroundThread.

Other controls can be created with the same principle, based on which control types & properties are being updated from the background thread.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Windows.Forms;

namespace ThreadSafeControls
{
    class TextBoxBackgroundThread : System.Windows.Forms.TextBox
    {
        public override string Text
        {
            get
            {
                return base.Text;
            }

            set
            {
                if (this.InvokeRequired)
                    this.Invoke((MethodInvoker)delegate { base.Text = value; });
                else
                    base.Text = value;
            }
        }

        public override System.Drawing.Color ForeColor
        {
            get
            {
                return base.ForeColor;
            }

            set
            {
                if (this.InvokeRequired)
                    this.Invoke((MethodInvoker)delegate { base.ForeColor = value; });
                else
                    base.ForeColor = value;
            }
        }


        public override System.Drawing.Color BackColor
        {
            get
            {
                return base.BackColor;
            }

            set
            {
                if (this.InvokeRequired)
                    this.Invoke((MethodInvoker)delegate { base.BackColor = value; });
                else
                    base.BackColor = value;
            }
        }
    }

    class ButtonBackgroundThread : System.Windows.Forms.Button
    {
        public override string Text
        {
            get
            {
                return base.Text;
            }

            set
            {
                if (this.InvokeRequired)
                    this.Invoke((MethodInvoker)delegate { base.Text = value; });
                else
                    base.Text = value;
            }
        }

        public override System.Drawing.Color ForeColor
        {
            get
            {
                return base.ForeColor;
            }

            set
            {
                if (this.InvokeRequired)
                    this.Invoke((MethodInvoker)delegate { base.ForeColor = value; });
                else
                    base.ForeColor = value;
            }
        }


        public override System.Drawing.Color BackColor
        {
            get
            {
                return base.BackColor;
            }

            set
            {
                if (this.InvokeRequired)
                    this.Invoke((MethodInvoker)delegate { base.BackColor = value; });
                else
                    base.BackColor = value;
            }
        }
    }
}
Ctznkane525
  • 7,297
  • 3
  • 16
  • 40
  • You should have explained how Control.InvokeRequired works! It checks if current execution is on GUI thread. Control.Invoke uses SynchronisationContext to schedule execution of callback code on GUI thread. – Arek Bal Jan 15 '18 at 08:57
  • I certainly can add it...but based on above he seems to know that already – Ctznkane525 Jan 15 '18 at 10:53
  • 1
    The aim of Stackoverflow is to be a resource for years to come, by limiting the knowledge you share to the OPs rep (which is high) you're making your answer less relevant to people with less knowledge. Consider an [edit], good luck – Jeremy Thompson Jan 15 '18 at 11:41
  • Will certainly do that – Ctznkane525 Jan 15 '18 at 11:56
  • i hope that's a better explanation of what the code was trying to accomplish – Ctznkane525 Jan 15 '18 at 12:58