How to correctly lock StringBuilder so it would not throw ArgumentOutOfRangeException

Question

I have read multiple posts both on stack-overflow and elsewhere that StringBuilder is not Thread safe and when accessed (read / written to) from multiple threads should be locked: here, here, here. I'm unclear about how to lock my Stringbuilder instances - in particular how many locks should be added and where.

I have the following code (snippet-ed):

    class MemoryTracker
    {
        private Process extProc    { get; set; }
        private StringBuilder sb   { get; set; }

        internal int trackByIP(string ip)
        {
             ...
             ...
             sb = new StringBuilder();
             extProc.OutputDataReceived  += new DataReceivedEventHandler((s, e)  => sb.Append(e.Data + "\n"));
             extProc.ErrorDataReceived   += new DataReceivedEventHandler((s, e)  => sb.Append(e.Data + "\n"));

             extProc.Start();
             extProc.PriorityClass       = ProcessPriorityClass.RealTime;
             ...
             ...
        }

        string getDataWStartEndPttrn(StringBuilder data, string strPttr, string endPttr, string extendedTerminator)
        {
             string s = data.ToString(); // <-- THROWS ArgumentOutOfRangeException 

             int si = getStartIdx(s, strPttr, patternDiff(strPttr, endPttr));
             int se = getEndIdx(s, endPttr, patternDiff(endPttr, strPttr));

             if (se >= 0 && si >= 0)
             {
                 string s1 = s.Substring(si, se - si);

                 string sTMP = s.Substring(se);
                 string s2 = s.Substring(se, sTMP.IndexOf(extendedTerminator));

                 return s1 + s2;
             }

             return "";
        }

Having placed the lock I still see the same error thrown.

class MemoryTracker
{
    private Process extProc    { get; set; }
    private StringBuilder sb   { get; set; }
    private Object thisLock = new Object();


    string getDataWStartEndPttrn(StringBuilder data, string strPttr, string endPttr, string extendedTerminator)
    {
        lock (thisLock)
        {
             string s = data.ToString() ; // <-- STILL THROWS ArgumentOutOfRangeException 
        }
 ...
 ...

QUESTION: How do I correctly think about where to place the lock / locks? There is no place where I explicitly create threads, so I assumed it would be a thread-safe usage of the StringBuilder.toString();

That's the wrong use of the lock. You lock around the use of a particular object. In this case you would want to lock the string builder. It appears you are passing in string builder as a parameter, which means the lock does nothing too prevent it from being accessed by a different thread. every non thread safe operation on the String builder needs to be locked, (usually anthing that modifies the contents of sb) — Hack
– Hack, Commented Dec 15, 2017 at 14:23
Thanks a lot, my idea was that I should probably lock on both append() and the .toString() calls. But maybe just one is enough (for the appends) ? — Mindaugas Bernatavičius
– Mindaugas Bernatavičius, Commented Dec 15, 2017 at 14:24
I believe the append should be enough. Not sure if you need to worry about protecting against reading while its being written too. — Hack
– Hack, Commented Dec 15, 2017 at 14:45
Yes you miss a lot of points (I'd say whole point) of locking. Create one lock object (or lock string builder itself), and lock like this: new DataReceivedEventHandler((s, e) => { lock (_sbLock) {sb.Append(e.Data + "\n"); }});, not like you are doing now. — Evk
– Evk, Commented Dec 15, 2017 at 17:33

vasil oreshenski · Accepted Answer · 2017-12-18 21:24:04Z

3

Alternative solution

Create thread safe wrapper around the StringBuilder use it instead the current. This will encapsulate the sync logic within the wrapper class. I think is a bit better this way, instead of locking in methods which use the StringBuilder class.

Use ReaderWriteLockSlim. It allows several threads to access the code block when a reader lock is aquired an only single thread to access when write lock is aquired. Simply if several threads tries to use .ToString() method this will be fine, and if meanwile some other threads tries to use .Append() concurently, this thread will wait all other threads with read (.ToString()) to finish.

Here is very basic impl, it should be good starting point.

public class ThreadSafeStringBuilder 
{
   private readonly StringBuilder core = .....
   private readonly ReaderWriterLockSlim sync = ....

   public void Append(String str) {
      try {
         this.sync.EnterWriterLock();
         this.core.Append(str);
      }
      finally {
         this.sync.ExitWriterLock()
      }
    }

  public override ToString() {
    try {
       this.sync.EnterReadLock();
       return this.core.ToString();
    }
    finnaly {
       this.sync.ExitReadLock()
    }
  }
}

Now you can use the thread safe wrapper without the need to sync the access to it everywhere.

EDIT 2017-12-18: Suggestion from the comments. You should perform some perf. test. For a simple scenario lock statement is more appropriate (check your worst case, like how many threads you expect to read or write ... etc, check also best case ... etc). In regards to the code, just replace the try, finnaly statements with lock statments, and the lock for both .Append() and .ToString() should lock on the same object.

edited Dec 18, 2017 at 21:24

answered Dec 15, 2017 at 15:14

vasil oreshenski

2,8561 gold badge17 silver badges25 bronze badges

Sign up to request clarification or add additional context in comments.

3 Comments

Kevin Gosse Over a year ago

ReaderWriterLock is efficient only if there's a lot of contention on the reading, and very little on the writing. In OP's scenario, it's almost certain that lock will perform better, and with less complexity

Mindaugas Bernatavičius Over a year ago

Thanks Vasil, it's nice to know the alternatives as well.

vasil oreshenski Over a year ago

@KevinGosse, you are right. Added an edit to include your suggestion.

Collectives™ on Stack Overflow

How to correctly lock StringBuilder so it would not throw ArgumentOutOfRangeException

1 Answer 1

3 Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

1 Answer 1

3 Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related