views:

404

answers:

3

Hello. I have an application wherein two threads write asynchronously to a single textbox. It works, except that the second thread to write to the textbox overwrites the line that the first thread just wrote. Any thoughts or insight into the problem would be greatly appreciated. I am using Microsoft Visual C# 2008 Express Edition. Thanks.

  delegate void SetTextCallback(string text);

  private void SetText(string text)
  {
     this.textBox1.Text += text;
     this.textBox1.Select(textBox1.Text.Length, 0);
     this.textBox1.ScrollToCaret();
  }

  private void backgroundWorkerRx_DoWork(object sender, DoWorkEventArgs e)
  {
     string sText = "";

     // Does some receive work and builds sText

     if (textBox1.InvokeRequired)
     {
        SetTextCallback d = new SetTextCallback(SetText);
        this.Invoke(d, new object[] { sText });
     }
     else
     {
        SetText(sText);
     }
  }
+3  A: 

EDIT: This might not solve the problem, but you might want to handle the ProgressChanged event of the BackgroundWorkers and set the text there.

For example:

void backgroundWorker_ProgressChanged(object sender, ProgressChangedEventArgs e) {
    SetText((string)e.UserState);
}  //Make this method handle the RunWorkerCompleted for both workers

//In DoWork:
    worker.ReportProgress(0, sText);

ProgressChanged is fired on the UI thread, so you don't need to call Invoke.

By the way, you should probably rename SetText to AppendText to make the code clearer.
Also, you can use the built-in delegate Action<String> instead of making your own SetTextCallback delegate type.

EDIT: Also, you should probably move the InvokeRequired check to SetText.

For example:

private void AppendText(string text) {
    if(textBox1.InvokeRequired) {
        textBox1.Invoke(new Action<string>(AppendText), text);
        return;
    }
    this.textBox1.AppendText(text);
    this.textBox1.SelectionStart = textBox1.TextLength;
    this.textBox1.ScrollToCaret();
}
SLaks
Thanks for the suggestion, but my receive backgroundworker never exits. It's set to always run becuase it needs to receive messages that are continuously sent at 10ms intervals.
Jim Fell
Then you can call `ReportProgress`.
SLaks
This is the right way of doing it. In my opinion, simply changing the code body of SetText (or AppendText as SLaks has named it here) so that the InvokeRequired check is inside it should do the trick.Using ReportProgress is the tidiest solution by far as it doesn't require any InvokeRequired shenanigans.Then, by channelling the operation through the UI thread it there will only ever be one write occuring on the text box at a time.
Andras Zoltan
@Zoltan: He's already calling it on the UI thread using `Invoke`. All I did in the second part was move the `Invoke` call.
SLaks
SLaks, it seems to work really well in that Textbox1 is now updated very rapidly, which is one of the things for which I was shooting. However, this seems to be pre-empting control over the other components in my application as soon as the backgroundWorkerRx_DoWork begins processing data. Could this be due to the massive ammount of data that is being added to the textbox?
Jim Fell
I'm not sure what you mean.
SLaks
+1  A: 

Try putting a lock over the section of the code that you believe you are having concurrency issues with.

lock(someObjectUsedOnlyForLocking)
{

}

Also, try using AppendText instead of manually concatenating the strings.

this.textBox1.AppendText(text);
Ragepotato
+1 although it's the SetText function that is likely to need the lock
Ed Woodcock
Oh, and don't be daft and use an instance lock inside the thread (easily done if you're not paying attention and a nightmare to find ;) )
Ed Woodcock
The SetText function can only be called from the main thread, so there's really nothing to lock. His calls to SetText are not happening concurrently.
Jon B
A: 

I agree with SLaks that you should be using the BackgroundWorker more properly. But to "fix" the code supplied, one issue is with the Invoke call... invocation needs to call upon the same method checking for requirement as to align the thread to the form's creator. I typically do something similar to the following (the argument usage may not be compilable but the rest is fine). Honestly, there should most likely only need to be one handler since only one thread can write at a time.

  void backgroundWorkerTx_DoWork(object sender, DoWorkEventArgs e)
  {
     if (this.InvokeRequired)
     {
        this.BeginInvoke(new EventHandler<DoWorkEventArgs>(backgroundWorkerTx_DoWork), sender, e);
        return;
     }
     //The text you wish to set should be supplied through the event arguments
     SetText((string)e.Argument);
  }
Jamie Altizer