3

My problem is if I use multi-thread on the same string sometime

the string won't get replace.(I wrote this on notepad so syntax may be

wrong)

using System.Thread ... Others ofcourse

class ....
{
    private static StringBuild container = new StringBuilder();

    static void Main(...)
    {
    container.Append(Read From File(Kind of long));
    Thread thread1 = new Thread(Function1);
        Thread thread2 = new Thread(Function2);
    thread1.Start();
    thread2.Start();
    //Print out container
    }

    static void Function1
    {
    //Do calculation and stuff to get the Array for the foreach
    foreach (.......Long loop........)
    {
    container.Replace("this", "With this")
    }
    }
    //Same goes for function but replacing different things.
    static void Function2
    {
    //Do calculation and stuff to get the Array for the foreach
    foreach (.......Long loop........)
    {
    container.Replace("this", "With this")
    }
    }
}

Now sometime some element does not get replace. So my solution to this is calling container.Replace on a different

method and do a "lock" which work but is it the right way?

private class ModiflyString
{
        public void Do(string x, string y)
            {
                lock (this)
                {
                    fileInput.Replace(x, y);
                }
            }
}
Athiwat Chunlakhan
  • 7,589
  • 14
  • 44
  • 72

3 Answers3

6

You should lock the StringBuilder object itself (inside the replace functions):

lock (container)
{
   container.Replace("this", "With this");
}

or create a separate lock object:

static object _stringLock = new object();

...

lock(stringLock)
{
    container.Replace("this", "With this");
}
Philippe Leybaert
  • 168,566
  • 31
  • 210
  • 223
  • I never thought of locking the stringbuilder itself, very nice. thanks, – Athiwat Chunlakhan Aug 12 '09 at 20:26
  • Phil's second example is what I'm referring to when I mentioned a "dummy" object. However, I think his first example where he locks the container is the best of them all. Since the container represents the data that is being shared between threads, you want to lock on that. If you used my example, there is the problem that if you had threads which are trying to call ModifyString.Do for totally separate containers, they will block one another, even though the containers they are working with are totally different. So you would hurt performance. So my example is really a bad implementation. – AaronLS Aug 12 '09 at 20:28
  • I can't edit this, anyway what is better locking on the object itself or locking on a "dummy" object – Athiwat Chunlakhan Aug 12 '09 at 20:41
3

Your locking won't work when you create more than 1 ModifyString object and I'm guessing you do.

A simple version:

   public void Do(string x, string y)
   {
      lock (fileInput)
      {
         fileInput.Replace(x, y);
      }
   }

It may be better to create a separate object to do the locking on, but the above shows the principle better: all competing threads should lock on the same object.

A standard approach would look like:

private static StringBuild container = new StringBuilder();
private static object syncLock = new object();  // simple object, 1-1 with container

and then you can (thread-)safely use:

   lock(syncLock)
   {
       container.Replace(...);
   }
H H
  • 263,252
  • 30
  • 330
  • 514
2

That will work fine as long as both threads have the same instance of the ModifyString class. In other words, this will work because the lock on "this" must be a lock on the same instance:

class Blah
{
    private static StringBuild container = new StringBuilder();

    private static ModifyString modifyString = new ModifyString();

    static void Main(...)
    {
    container.Append(Read From File(Kind of long));
    Thread thread1 = new Thread(Function1);
        Thread thread2 = new Thread(Function2);
    thread1.Start();
    thread2.Start();
    //Print out container
    }

    static void Function1
    {       

        //Do calculation and stuff to get the Array for the foreach
        foreach (.......Long loop........)
        {
           modifyString.Do("this", "With this")
       }
    }
    //Same goes for function but replacing different things.
    static void Function2
    {
        //Do calculation and stuff to get the Array for the foreach
        foreach (.......Long loop........)
        {
            modifyString.Do("this", "With this")
        }
    }
}

It will NOT work if you did the below, because lock(this) would not work sense they are two seperate instances:

class Blah
{
    private static StringBuild container = new StringBuilder();

    static void Main(...)
    {
    container.Append(Read From File(Kind of long));
    Thread thread1 = new Thread(Function1);
        Thread thread2 = new Thread(Function2);
    thread1.Start();
    thread2.Start();
    //Print out container
    }

    static void Function1
    {
       ModifyString modifyString = new ModifyString();
       //Do calculation and stuff to get the Array for the foreach
       foreach (.......Long loop........)
       {
          modifyString.Do("this", "With this")
       }
    }
    //Same goes for function but replacing different things.
    static void Function2
    {
       ModifyString modifyString = new ModifyString();

       //Do calculation and stuff to get the Array for the foreach
        foreach (.......Long loop........)
        {
            modifyString.Do("this", "With this")
        }
    }
}

Some people would actually create a "dummy" object to perform the lock on instead of using "this" (you can't lock on the string since it is a value type).

AaronLS
  • 37,329
  • 20
  • 143
  • 202