C# string becoming empty

Question

Relatively new to C# and coding in general (first post here). I have a WinForms local application where some information is displayed to the user in a ReadOnly(true) RichTextBox. Almost all my classes need to send information to that RichTextBox. To simplify this process, I created a method inside a static class that uses a locked delegate to send the information to that RichTextBox. Here is a sample:

static class MyClass
{
    public delegate void MessageReceivedEventHandler(string message);
    public static event MessageReceivedEventHandler messageReceivedEventHandler;

    public static void MessageBox(string message)
    {
        lock (messageReceivedEventHandler)
        {
            //Thread.Sleep(20);
            messageReceivedEventHandler?.Invoke(message);
        }
    }
}
partial class MyForm : Form
{
    public MyForm()
    {
        MyClass.messageReceivedEventHandler += OnMessageReceived;
    }

    private void OnMessageReceived(string message)
    {
        richTextBox1.Text = richTextBox1.Text.Insert(0, $" {message}\n");
    }
    private void Button1_click()
    {
        MyClass.MessageBox("This should be working!");
        //Add more work here...
    }
}

The code above would simply print "This should be working!" inside the RichtTextbox.

The problem is the text from richTextBox1 sometimes becoming empty. This issue seems to appear when the MessageBox method is being called in rapid succession. My assumption was that since I have diffent Tasks running at the same time (in other parts of my code), it probably is two Tasks attempting to use the same static ressource, hence the use of Lock. But I still have the issue.

Adding the Thread.Sleep(20) seems to fix the problem, but that is far from elegant/robust. It starts breaking up again when the time inside Sleep is <10ms.

Edit 1: To clarify what I mean by "string becoming empty", it means the text from richTextBox1 is == "" at some points, which should not happen since the code is always inserting the text, not replacing it. The OnMessageReceived method is the only place where action is taken on the RichTextBox text.

Edit 2: I saw many questions related to the other tasks running. First, yes it is a multi-threaded application. The only relation between those tasks and my main form is the "print" function I wrote above. To give more context, this application is used to control the position of stepper motors relative to an electrical signal. When doing so, I need to print important information in my main form. This is why losing the information in my RichTextBox (where I print the information) is an issue. The possible reason of why I am losing the text inside that RichTextBox should be the focus of this thread.

Keep in mind that this is a personnal side project, and not a large scale application.

Thanks, Laurent

Answer 1

There are multiple problems in your code.

First, you should not lock on a public object, since that allows other threads to lock on the same object, risking interlocking your threads. Second, your symptoms suggest multiple threads are trying to access the ressources. Rather than depending on complex thread locking code, you'd rather schedule UI operations on the UI context, which will allow calling adding message from background tasks.

The best way to do that is to that is by using Control.BeginInvoke()

You can't copy your form instance everywhere, so we'll expose a static method. You could make the class a singleton, but if you need multiple instances that won't work. I'll give a more versatile example. When the static method is called, you don't have access to the form instance anymore, so we'll use IOC pattern with an event and delegate.

Let's make a private static event that all instances will register a callback to in the constructor. When the static method raises the static event, all instances callback will be called. The callback will schedule a modification of its text box.

partial class MyForm : Form
{
    private class MessageWriteRequestedEventArgs : EventArgs
    {
        public string Message { get; }
        public MessageWriteRequestedEventArgs(string message)
        {
            Message = message;
        }
    }

    private static event EventHandler<MessageWriteRequestedEventArgs> MessageWriteRequested;

    public MyForm()
    {
        MessageWriteRequested += OnMessageWriteRequested;
    }

    public static void WriteMessage(string message)
    {
        MessageWriteRequested?.Invoke(this, new MessageWriteRequestedEventArgs(message));
    }

    private void OnMessageWriteRequested(object sender, MessageWriteRequestedEventArgs e)
    {
        richTextBox1.BeginInvoke(() => WriteMessageSafe(e.message));            
    }

    private void WriteMessageSafe(string message)
    {
        richTextBox1.Text = richTextBox1.Text.Insert(0, $" {message}\n");
    }

    private void Button1_click()
    {
        // you're on ui context, you're safe to access local ui resources
        WriteMessageSafe("This should be working!");
        // if you have multiple MyForm instances, you need to use the event
        WriteMessage("Broadcasting my tralala");
    }
}

If you need to write to the textbox from anywhere else :

// do stuff
MyForm.WriteMessage("Ho Ho Ho !");

Answer 2

.NET already includes a class for reporting progress (or any other information) from an asynchronous operation in a thread-safe manner, Progress< T>. It doesn't need locking and even better, it decouples the sender and receiver. Many long-running BCL operations accept an IProgress<T> parameter to report progress.

You haven't explained what's going on in the form, or what task is reporting the data. Assuming the producer is another method in the same form, you could create a Progress<T> instance in the same method that starts the async operation, eg :

async void Button1_Click()
{
    var progress=new Progress<string>(ReportMessage);

    ReportMessage("Starting");
    await Task.Run(()=>SomeLongOp(progress));
    ReportMessage("Finished");
}

void SomeLongOp(IProgress<string> progress)
{

    for(int i=0;i<1000000;i++)
    {
        ...
        progress.Report($"Message {i}");
        ...
    }

}

void ReportMessage(string message)
{
    richTextBox1.Text = richTextBox1.Text.Insert(0, $" {message}\n");
}

By using IProgress< T>, the SomeLongOp method isn't tied to a specific form or global instance. It could easily be a method on another class

Publishing lots of messages

Let's say you have a lot of workers, doing a lot of things, eg monitoring a lot of devices, and want all of them to publish messages to the same Log textbox or RTF box. Progress< T> "simply" executes the reporting delegate or event handler on its original sync context. It doesn't have an asynchronous Report method, nor can it queue messages. In a really high-traffic environment, the synchronization switch can delay all workers.

The built-in answer to this is to use one of the pub/sub classes like ActionBlock< T> or a Channel.

An ActionBlock< T> processes the messages in its input queue in order, using a worker task that runs on the ThreadPool by default. This can be changed by specifying a different TaskScheduler in its execution options. By default, its input queue is unbounded.

One could use an ActionBlock to receive messages from multiple workers and display them on a textbox. The block can be created in the constructor, and passed to all workers as an ITargetBlock<T> interface :

ActionBlock<string> _logBlock;

public MyForm()
{

    var options=new ExecutionDataFlowBlockOptions {
        TaskScheduler=TaskScheduler.FromCurrentSynchronizationContext();
    };
    _block=new ActionBlock<string>(ReportMessage,options);

}

Now the fun begins. If the workers are created by the form itself, the workers can publish to the block directly :

public async void Start100Workers_Click(...)
{
    var workers=Enumerable.Range(0,100)
                          .Select(id=>DoWork(id,_block));
    await Task.WhenAll(workers);
}

async Task DoWork(int id,ITargetBlock<string> logBlock)
{
    .....
    await logBlock.SendAsync(message);
    ...
}

Or the block could be exposed through a public property, so other classes/forms in the application can post to it.

public ITargetBlock<string> LogBlock=>_block;

Answer 3

I'm going to show a simple way to do what I think you're after.

I started with a .NET Core 3.1 Win forms application. I added a rich text control to the form. I added a button to the form.

I added a TaskCompletionSource as a instance property - this will be used to control the tasks acting as workers which you described.

CancellationTokenSource sharedCancel = new CancellationTokenSource();

I created an interface to represent something that accepts messages as you described:

public interface IMyMessageSink
{
    Task ReceiveMessage(string message);
}

I made my form support this interface.

public partial class Form1 : Form, IMyMessageSink

The ReceiveMessage method looks like this:

    public Task ReceiveMessage(string message)
    {
        if(this.sharedCancel == null || this.sharedCancel.IsCancellationRequested)
            return Task.FromResult(0);

        this.Invoke(new Action<Form1>((s) => this.richTextBox1.Text = this.richTextBox1.Text.Insert(0, $"{message}\n")), this);

        return Task.FromResult(0);
    }

You'll see the Invoke handles the synchronization back to the UI thread.

This should probably use BeginInvoke and then convert the APM to async tasks which you can read about here. But for an SO answer the above simple code will suffice.

Also note there's no error handling. You'll want to add that to your generator and to the button handler.

Next I created a class to represent something that creates messages. This class takes the interface created and the cancellation token. It looks like this:

public class MyMessageGenerator
{
    CancellationToken cancel;
    IMyMessageSink sink;

    public MyMessageGenerator(CancellationToken cancel, IMyMessageSink sink)
    {
        this.cancel = cancel;
        this.sink = sink;
    }

    public async Task GenerateUntilCanceled()
    {
        try
        {
            while (!this.cancel.IsCancellationRequested)
            {
                await sink.ReceiveMessage(this.GetHashCode().ToString());
                await Task.Delay(5000, this.cancel);
            }
        }
        catch (OperationCanceledException)
        { }
    }
}

In the button handler we create the message generators.

    async void button1_Click(object sender, EventArgs e)
    {
        if (null == this.sharedCancel)
            return;

        await Task.Run(() => new MyMessageGenerator(this.sharedCancel.Token, this).GenerateUntilCanceled());
    }

Finally I added an override for the form closing event:

    protected override void OnClosing(CancelEventArgs e)
    {
        if (null != this.sharedCancel)
        {
            this.sharedCancel.Cancel();
            this.sharedCancel.Dispose();
            this.sharedCancel = null;
        }

        base.OnClosing(e);
    }

If the application becomes larger and more complex you would likely benefit by adding services exposed using a DI container. You can read about adding DI to a winforms app here.