views:

711

answers:

6
+1  Q: 

c# locking

Hello, I am trying to prevent data races in a multihreaded server. My problem is the following: there is a List<RServer>, the type RServer is a class with several fields. Now, the server has several threads all running at the same time and they can modify both the List (adding more items) and the individual RServer instances (changing the fields).

So my strategy is to make a readonly object RServerLock = new object( ) in each of the RServer instances and additionally a readonly object RServerListLock = new object( ) and enclose all the code that modifies either (the List or a RServer instance) in a lock. Is this safe? What happens if a thread tries to lock a RServerLock while another one is locking it?

A: 

That is safe. If one thread has acquired the lock, other threads will have to wait until the lock is released.

However, improbable as it is, you could hit a performance problem since the lock may be TOO global. It really depends on what your state is and how it is mutated by those threads, so I can't help you with that.

Shachar
A: 

To restate a bit - each RServer instance has a variable called RServerLock which will be locked using a lock block.

Thread 1 (T1) takes a lock on RServer 1 (R1). Thread 2 (T2) comes in an tries to modify R1 which causes the R1 lock block to be entered. In this case T2 will wait till T1 is finished.

One thing to be really careful of is how many RServer instances you end up with. If you end up with a plethora of instances, you are carrying around an extra 20 bytes of data just for locking. At this point, you may want to consider lock striping.

Justin Rudd
+1  A: 

If a thread tries to lock an objec that is already locked, it will block until the lock has been released. There aren't concurrency issues when two threads tries to lock it at the same time as the lock is an atomic operation and one of the threads will thus always be the victim of the lock and end up blocking.

Your strategy sounds sound, provided that you need a complete lock for the RServer instance. If you can lock on the specific RServer instance fields specifically, that might be more efficient. It will however increase the number of lock operations and will be more complicated.

Mark S. Rasmussen
+7  A: 

If you have a contended lock, the second thread has to wait until the first releases the lock.

Your plan sounds nearly okay - but you need to lock when reading data as well, to make sure you get the most recent values, and consistent ones. Otherwise you could be half way through writing some values in one thread, and see some of the new values - but possibly not all - and the old values, all at the same time in a different thread.

If you can avoid doing this as much as possible, your life will be easier :) Immutable types make threading a lot simpler.

Don't forget that if you ever have code which will need two locks at the same time (e.g. adding one RServer and modifying another, atomically) you must make sure that you always aquire locks in the same order - if one thread tries to acquire lock B while it's holding lock A, and a different thread tries to acquire lock A while it's holding lock B, you'll end up with deadlock.

See my threading tutorial or Joe Albahari's for more details. Also, if you're interested in concurrency, Joe Duffy has an excellent book which is coming out very soon.

Jon Skeet
A: 

If I understand your question correctly, it sounds like you're trying to create a wheel where perfectly good wheels exist already.

Check out the system.threading namespace in MSDN. It has lots of locking mechanisms designed specifically for dealing with situations like this.

http://msdn.microsoft.com/en-us/library/system.threading.aspx

Cheers! Greg [MSFT]

Greg Oliver
+2  A: 

Looks like you have a prime candidate for a ReaderWriterLock. The best class to use (if your runtime supports it, I think 3.0+) is ReaderWriterLockSlim as the original ReaderWriterLock has performance issues.

One of the MSDN magazine authors also came across a problem with the RWLS class, I won't go into the specifics here, but you can look at it here.

I know the following code will spawn the fury of the IDisposable purists, but sometimes it really makes nice syntactic sugar. In any case, you may find the following useful:

    /// <summary>
    /// Opens the specified reader writer lock in read mode,
    /// specifying whether or not it may be upgraded.
    /// </summary>
    /// <param name="slim"></param>
    /// <param name="upgradeable"></param>
    /// <returns></returns>
    public static IDisposable Read(this ReaderWriterLockSlim slim, bool upgradeable)
    {
        return new ReaderWriterLockSlimController(slim, true, upgradeable);
    } // IDisposable Read

    /// <summary>
    /// Opens the specified reader writer lock in read mode,
    /// and does not allow upgrading.
    /// </summary>
    /// <param name="slim"></param>
    /// <returns></returns>
    public static IDisposable Read(this ReaderWriterLockSlim slim)
    {
        return new ReaderWriterLockSlimController(slim, true, false);
    } // IDisposable Read

    /// <summary>
    /// Opens the specified reader writer lock in write mode.
    /// </summary>
    /// <param name="slim"></param>
    /// <returns></returns>
    public static IDisposable Write(this ReaderWriterLockSlim slim)
    {
        return new ReaderWriterLockSlimController(slim, false, false);
    } // IDisposable Write

    private class ReaderWriterLockSlimController : IDisposable
    {
        #region Fields

        private bool _closed = false;
        private bool _read = false;
        private ReaderWriterLockSlim _slim;
        private bool _upgrade = false;

        #endregion Fields

        #region Constructors

        public ReaderWriterLockSlimController(ReaderWriterLockSlim slim, bool read, bool upgrade)
        {
            _slim = slim;
            _read = read;
            _upgrade = upgrade;

            if (_read)
            {
                if (upgrade)
                {
                    _slim.EnterUpgradeableReadLock();
                }
                else
                {
                    _slim.EnterReadLock();
                }
            }
            else
            {
                _slim.EnterWriteLock();
            }
        } //  ReaderWriterLockSlimController

        ~ReaderWriterLockSlimController()
        {
            Dispose();
        } //  ~ReaderWriterLockSlimController

        #endregion Constructors

        #region Methods

        public void Dispose()
        {
            if (_closed)
                return;
            _closed = true;

            if (_read)
            {
                if (_upgrade)
                {
                    _slim.ExitUpgradeableReadLock();
                }
                else
                {
                    _slim.ExitReadLock();
                }
            }
            else
            {
                _slim.ExitWriteLock();
            }

            GC.SuppressFinalize(this);
        } // void Dispose

        #endregion Methods
    } // Class ReaderWriterLockSlimController

Put that in an extension method class (public static class [Name]) and use it as follows:

using(myReaderWriterLockSlim.Read())
{
  // Do read operations.
}

Or

using(myReaderWriterLockSlim.Read(true))
{
  // Read a flag.
  if(flag)
  {
    using(myReaderWriterLockSlim.Write()) // Because we said Read(true).
    {
      // Do read/write operations.
    }
  }
}

Or

using(myReaderWriterLockSlim.Write()) // This means you can also safely read.
{
  // Do read/write operations.
}
Jonathan C Dickinson