Does locking C# delegates for writing guarantee thread safety?

Question

C# delegates are immutable, so when you add or remove a function from the delegate, it is replaced by a new object.

Considering the following piece of code:

lock(shared_instance.my_delegate){
  shared_instance.my_delegate+=Foo;
}

Consider the following situation:

Threads 1 and 2 arrive at the lock, thread 2 blocks on the delegate instance, let's call it A.

Thread 1 replaces A with a new instance, B, and exits the lock.

Does thread 2 acquire lock on B or on A? If it's on A, then it seems to me that thread 3 could come along, acquire a lock on B at the same time when thread 2 acquires a lock on A, and they could simultaneously try to overwrite the deleage and a race would occur, correct?

EDIT:

I now realize that the question could be generalized to any reference type:

lock(shared_ref){
  shared_ref=different_ref;
}

Basically what I want to know is: If this happens, waiting threads will still lock on the old reference, yes?

Don't lock over `shared_instance.my_delegate`. Use dedicated variable for that, usually just an `object` that is initialized in the constructor. That will solve your problem and remove all the ambiguity. — Frank J, Sep 24 '15 at 17:17
since you use `my_delegate+=` , my_delegate is actually an event right? and the you should look at this answer: http://stackoverflow.com/questions/3522361/add-delegate-to-event-thread-safety . also don't lock on events, they are null be default. in short: event += handler is thread safe as lock as event.add is not replaced (same goes for -= and event.remove) — x4rf41, Sep 24 '15 at 17:17
also: can you show more code? like an entire example of the class `shared_instance` is of? i can explain it to you then — x4rf41, Sep 24 '15 at 17:21
@FrankJ I know, I don't intend to, it just piqued my interest. — Tomeamis, Sep 24 '15 at 17:23
@x4rf41No, it's a delegate, although it's used somewhat like an event. `my_delegate` is a public field of `shared_instance`, but it's only accesssible to the class containing the code example (other classes only know an interface). — Tomeamis, Sep 24 '15 at 17:26
@Tomeamis: Well in that case, I would think your assumption is correct. However that is based on no evidence whatsoever... — Frank J, Sep 24 '15 at 17:27
ok now, i get it, im pretty sure your assumption is correct then, that is also the reason why you should never lock an object that might change (without a proper safe exchange). the Thread aquires the lock on the reference, not the field. meaning if the field changes, the the thread still has the lock on the old field value — x4rf41, Sep 24 '15 at 17:33
@x4rf41 Thank you, could you please write that as an aswer so I could mark it? — Tomeamis, Sep 24 '15 at 17:37
@Tomeamis i just added some sample code to play around with. the way i do it in the example is a good way to examine threading behavior by ensuring the timing with sleeps — x4rf41, Sep 24 '15 at 17:59

score 1 · Answer 1 · answered Sep 24 '15 at 17:40

1

Does thread 2 acquire lock on B or on A? If it's on A, then it seems to me that thread 3 could come along, acquire a lock on B at the same time when thread 2 acquires a lock on A, and they could simultaneously try to overwrite the deleage and a race would occur, correct?

You have a race condition in both cases. You have a race condition with your code, you have the race condition if you use a dedicated object to lock on, and you have the same race condition if you just have no lock at all. In all of the three cases one of the threads will set the value and have it immediately overridden, and one will set the value second and have it stick. In all of these cases it's indeterminate which thread will "win". In none of these cases will anything else happen besides the value being set to one of the two possible values.

answered Sep 24 '15 at 17:40

Servy

202,030
26
332
449

I thought that lock introduces memory barriers, and therefore, at least in the delegate case, would load the new value. – Tomeamis Sep 24 '15 at 17:44
In the case of a simple reference, the lock would probably be unnecessary anyways, as whatever happens, only one value will remain assigned, and IIRC, references are atomic. Reordering could happen, but at that point it hardly seems to matter. – Tomeamis Sep 24 '15 at 17:47
1

@Tomeamis Yes, the `lock` does introduce a memory barrier. Looking at just this one section in isolation makes analyzing what should be done particularly difficult. Perhaps a lock on a shared variable is necessary, perhaps a memory barrier is sufficient, or perhaps something else entirely needs to happen. In general though locking around just a single variable assignment is code smell, even though there are rare cases where it makes sense. Typically there's a larger operation that should be locked around though. – Servy Sep 24 '15 at 17:48

x4rf41 · Accepted Answer · 2015-09-24T17:57:11.210

looking at this code:

lock(shared_ref){
  shared_ref=different_ref;
}

lock(shared_ref) will lock the reference of what ever is inside the shared_ref variable, replacing the value of the variable will not change the lock and at the end of the lock { } block, the old reference will be released. So if any other thread locks on shared_ref before it is changed but while it is locked, the lock will still be released. A third thread might lock onto the new reference when it is already set and a race for the setting the variable would occur, because thread 2 will be release and thread 3 never waits at the lock because noone holds the reference. So thread 2 and 3 will race for setting the new variable

i just made some sample code, which i think makes it pretty clear:

public class TestClass
{
    public static object myObject;

    public static void setObject(object newValue, string thread)
    {
        lock(myObject)
        {
            Debug.Print(thread+" reached the inside of the lock");
            Thread.Sleep(1000);
            myObject = newValue;
            Debug.Print(thread + " myObject was set");
            Thread.Sleep(1000);
        }
        Debug.Print(thread + " lock released");
    }

    public static void Test()
    {
        myObject = new object();
        Thread t1 = new Thread(t1_run);
        Thread t2 = new Thread(t2_run);
        Thread t3 = new Thread(t3_run);
        t1.Start();
        t2.Start();
        t3.Start();
    }

    private static void t1_run()
    {
        setObject(new object(), "t1");
    }

    private static void t2_run()
    {
        Thread.Sleep(500); // 500 millisecods so it will be locked on the old
        setObject(new object(), "t2");
    }

    private static void t3_run()
    {
        Thread.Sleep(1500); // just make sure myObject was replaced
        setObject(new object(), "t3");
    }
}

now obviously the output is:

t1 reached the inside of the lock
t1 myObject was set
t3 reached the inside of the lock
t1 lock released
t2 reached the inside of the lock
t3 myObject was set
t2 myObject was set
t3 lock released
t2 lock released

because the sleeps ensure the order in which t2 and t3 set myObject. But that is not ensured when the timing is very close

Does locking C# delegates for writing guarantee thread safety?

2 Answers2