5
\$\begingroup\$

I wrote the following class so that a single object can be shared across many threads. Is using the ReaderWriterLockSlim redundant in this case and only Interlocked.Funcs() can do the job and ensure thread safety?

using System.Threading;


public class SharedData<T> where T:class
{
T _data;
ReaderWriterLockSlim _lock;

public SharedData(T arg)
{
    Interlocked.Exchange<T>(ref _data, arg);
    _lock = new ReaderWriterLockSlim();
}

public T Value
{
    get
    {
        _lock.EnterReadLock();
        var ret = Interlocked.CompareExchange<T>(ref _data, default(T), default(T));
        _lock.ExitReadLock();

        return ret;
    }
    set
    {
        _lock.EnterWriteLock();
        Interlocked.Exchange<T>(ref _data, value);
        _lock.ExitWriteLock();
    }
}
}
\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review. Questions are always asked explicitly i.e can this be improved?, are they better alternatives? Your question isn't clear enough- do you want to know which of the two methods can achieve the result? \$\endgroup\$ – Siobhan Oct 29 '16 at 21:21
  • \$\begingroup\$ What problem you find in the code? is it doing correctly or is doing a little too much. is the readerwriter lock too much ? \$\endgroup\$ – Simple Fellow Oct 30 '16 at 9:47
  • \$\begingroup\$ That should be included in your question \$\endgroup\$ – Siobhan Oct 30 '16 at 10:17
  • \$\begingroup\$ A caveat: ReaderWriterLockSlim is IDisposable which ironically makes it not thread-safe, since disposing it when there are pending readers/writers throws an exception. \$\endgroup\$ – Dan Lyons Oct 31 '16 at 17:59
3
\$\begingroup\$

Using Interlocked on a class reference grants atomic transaction when you exchange an object for another;

For example, using

Interlocked.Exchange<T>(ref _data, value);

will exchange an object for an other, returning the previous object.

ReaderWriterLockSlim will protect some parts of your code, and you could also protect the object get/set with it.

Use only one of the lock, not both !

Also, you don't need usually to protect the initial set statement in your Ctor. Basically, you can only create a new object once in only one thread, before sharing anything related to this object.

One last point, with this code you are protecting the object reference, not the object itself. See below ...

class MyString {
    public string Data {get; set;}
}
...
var myString = new MyString() ;
var mySharedString = new SharedData<MyString>(myString) ;
...    
... thread 1
... mySharedString.Value.Data = "Hello" ;
...
... thread 2
... mySharedString.Value.Data = "BAD" ;

If the shared object is ReadOnly, it works, if the object is mutable, it will fail badly.

\$\endgroup\$
  • \$\begingroup\$ Thanks, this is what answers my concern that ReaderWriterSlim may be too much, About your last point. How do I make sure that the object itself is protected? like only providing the get value? \$\endgroup\$ – Simple Fellow Oct 31 '16 at 6:13
  • \$\begingroup\$ Providing only the get value is ok, it means your object is now readonly regarding this property. \$\endgroup\$ – thesyndarn Oct 31 '16 at 10:41
0
\$\begingroup\$

Try this out- it is thread safe and asynchronous.
This specific flavor does not enforce initialization, which is sometimes desirable is asynchronous scenarios.

There is a lot less overhead incurred when using this approach
As opposed to various synchronization primitives which revolve around blocking.

Consuming an asynchronous API enables your application to reuse the context, instead of wasting it by blocking. be it a UI context or a TPL Worker thread- they probably have better things to do than block.

Also note the use of the Interlocked API - it is an abstraction over a Hardware backed atomic operation which is orders of magnitude faster than locking.

public sealed class AsyncReference<T>
{

private TaskCompletionSource<T> _tcs;

public T Value
{
    set
    {
        var tcs = new TaskCompletionSource<T>();
        tcs.SetResult(value);

        TaskCompletionSource<T> previous = Interlocked.Exchange(ref _tcs, tcs);
        previous.TrySetResult(value);
    }
}

public Task<T> ValueAsync => _tcs.Task;

public AsyncReference()
{
    _tcs = new TaskCompletionSource<T>();
}

public AsyncReference(T value):this()
{
    _tcs.SetResult(value);
}

public static implicit operator Task<T>(AsyncReference<T> reference)
{
    if(reference==null)
        throw new InvalidOperationException($"null {typeof(AsyncReference<T>)} reference conversion is not supported");

    return reference._tcs.Task;
}

public static implicit operator AsyncReference<T>(T value)
{
    return new AsyncReference<T>(value);
}
}
\$\endgroup\$
  • \$\begingroup\$ If your code has any other advantages besides the one you mentioned, you should state them. Why is your code better than the OP? \$\endgroup\$ – Bruno Costa Oct 31 '16 at 13:13
  • \$\begingroup\$ @BrunoCosta Thank you for your input. I've revised my answer. \$\endgroup\$ – Eyal Perry Oct 31 '16 at 17:26
  • \$\begingroup\$ I do not see any advantages on doing Interlocked.Exchange(ref _tcs, tcs) asynchronously. If am not wrong than that operation would only take 1 assembly instruction. This is also the first time I am ever seeing a property with only a setter, I guess it could happen but I do not see any reason for that in this case. \$\endgroup\$ – Bruno Costa Oct 31 '16 at 17:35
  • \$\begingroup\$ The advantage is not in doing Interlocked asynchronously, but in enabling the asynchronous consumption of the reference. \$\endgroup\$ – Eyal Perry Oct 31 '16 at 17:38
  • \$\begingroup\$ even if you rephrase the problem in that perpective I can't see the advantage because this is an operation that shouldn't take too long. It still is probably one of the most costly assembly instructions because it does a cache invalidattion, but still... \$\endgroup\$ – Bruno Costa Oct 31 '16 at 17:52

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.