views:

436

answers:

3
public void MyTest()
{
  bool eventFinished = false;

  myEventRaiser.OnEvent += delegate { doStuff(); eventFinished = true; };
  myEventRaiser.RaiseEventInSeperateThread()

  while(!eventFinished) Thread.Sleep(1);

  Assert.That(stuff);
}

Why can't eventFinished be volatile and does it matter?

It would seem to me that in this case the compiler or runtime could become to smart for its own good and 'know' in the while loop that eventFinished can only be false. Especially when you consider the way a lifted variable gets generated as a member of a class and the delegate as a method of that same class and thereby depriving optimizations of the fact that eventFinished was once a local variable.

+7  A: 

There exists a threading primitive, ManualResetEvent to do precisely this task - you don't want to be using a boolean flag.

Something like this should do the job:

public void MyTest()
{
    var doneEvent = new ManualResetEvent(false);

    myEventRaiser.OnEvent += delegate { doStuff(); doneEvent.Set(); };
    myEventRaiser.RaiseEventInSeperateThread();
    doneEvent.WaitOne();

    Assert.That(stuff);
}

Regarding the lack of support for the volatile keyword on local variables, I don't believe there is any reason why this might not in theory be possible in C#. Most likely, it is not supported simply because there was no use for such a feature prior to C# 2.0. Now, with the existence of anonymous methods and lamda functions, such support could potentially become useful. Someone please clarify matters if I'm missing something here.

Noldorin
Where's Eric when you need him? ;-p
Marc Gravell
Hehe. Indeed, we're getting into murky areas of C#/CLR now - on which I'm sure he could shed some light.
Noldorin
Thank you for the ManualResetEvent. It even works when the event happens to be called in the same thread, which is a possibility I did not want to exclude in MyTest().
Patrick Huizinga
If your threading and locking is so complex, performance-sensitive and processor-ordering-sensitive that you need to be marking stuff as volatile then you should not be relying on the compiler to do all kinds of crazy rewrites of local variables into fields of closure classes for you. You should be writing the code explicitly so that it is as clear and precise as possible, so as to communicate the exact semantics of the code to the maintenance programmers who will need to understand it. The point of the syntactic sugar is to hide the mechanism; you want to be exposing the mechanism!
Eric Lippert
What happens if the doneEvent.Set(); occur before doneEvent.WaitOne(); ?
Zanoni
@Eric: Thanks for the explanation. Doesn't volatile just mean telling the compiler not to optimise variable assignments, though? In this case, it wouldn't seem to be a problem to make local vars volatile.
Noldorin
@Zanoni: That's not a problem. In that case, the call to WaitOne() just returns immediately.
Noldorin
I'm not following you. Under what circumstances (other than a "local" that is actually implemented as a field) could the same local be accessed by two different threads?
Eric Lippert
Well the local itself couldn't be accessed by two different threads, as you point out, but the field created by the closure *could*. Marking a local variable as volatile could indicate that the field generated by the closure would be marked the same.
Noldorin
Right. And if you are in a situation so complex that you need to do that, then do not use automatically generated closures in the first place. Don't be relying on a whole lot of implementation-dependent, compiler-generated code to do the right thing when you have complex, performance-sensitive, processor-ordering-sensitive code. Roll your own closure class in that case so that you can clearly analyze the thread safety of all of its parts.
Eric Lippert
Look at it this way: the language makes no guarantee that hoisted locals are implemented as fields. They could be implemented as elements of an array, or properties, or whatever, as long as the language is able to generate code in the runtime that maintains the closure and variable semantics. Do we want to add a feature to the language that puts a big restriction on how compiler writers are allowed to generate code? No, particularly since the feature is a bad idea to begin with. Hoisted locals being fields is already a leaky abstraction; we don't want to make it worse.
Eric Lippert
@Eric: Thanks for the explanation - I see exactly where you're coming from now. (Also, I wasn't necessarily arguing that it *should* be a feature, but rather enquiring as to the reason you couldn't). So yeah, I fully agree that this would be hiding too much of the implementation and lead to either restricted or unexpected behaviour here.
Noldorin
+1  A: 

What would happen if the Event raised didn't complete until after the process had exited the scope of that local variable? The variable would have been released and your thread would fail.

The sensible approach is to attach a delegate function that indicates to the parent thread that the sub-thread has completed.

Lazarus
The code is fine; that is a "captured" variable, and is implemented as a field on a compiler-generated class. The thread won't fail.
Marc Gravell
Couldn't it be the same case with a class-level variable, though? If the instance is garbage-collected before the event in the other thread finishes, then the same issue occurs.
Noldorin
It can't be garbage collected while it is still in the visible scope of either thread.
Marc Gravell
@Marc: Yeah, I just considered that after commenting. Nonetheless, volatile local variables can be useful on local variables in C# 2.0, it would seem.
Noldorin
Or they might if they existed, but I know what you mean...
Marc Gravell
I don't see how a garbage collector could 'steal the show' here. The MyTest() method is doing a semi- while(true){} loop while waiting for the result. The only bad that that can happen is if the event delegate forgot to set eventFinished to true and thereby causing MyTest() to hang. Luckily, I'm smart enough not to forget. ;-)
Patrick Huizinga
@Marc: Yeah. s/can/might
Noldorin
+5  A: 

In most scenarios, local variables are specific to a thread, so the issues associated with volatile are completely unnecessary.

This changes when, like in your example, it is a "captured" variable - when it is silently implemented as a field on a compiler-generated class. So in theory it could be volatile, but in most cases it wouldn't be worth the extra complexity.

In particular, something like a Monitor (aka lock) with Pulse etc could do this just as well, as could any number of other threading constructs.

Threading is tricky, and an active loop is rarely the best way to manage it...


Re the edit... secondThread.Join() would be the obvious thing - but if you really want to use a separate token, see below. The advantage of this (over things like ManualResetEvent) is that it doesn't require anything from the OS - it is handled purely inside the CLI.

using System;
using System.Threading;
static class Program {
    static void WriteLine(string message) {
        Console.WriteLine(Thread.CurrentThread.Name + ": " + message);
    }
    static void Main() {
        Thread.CurrentThread.Name = "Main";
        object syncLock = new object();
        Thread thread = new Thread(DoStuff);
        thread.Name = "DoStuff";
        lock (syncLock) {
            WriteLine("starting second thread");
            thread.Start(syncLock);
            Monitor.Wait(syncLock);
        }
        WriteLine("exiting");
    }
    static void DoStuff(object lockHandle) {
        WriteLine("entered");

        for (int i = 0; i < 10; i++) {
            Thread.Sleep(500);
            WriteLine("working...");
        }
        lock (lockHandle) {
            Monitor.Pulse(lockHandle);
        }
        WriteLine("exiting");
    }
}
Marc Gravell
What if RaiseEventInSeperateThread() was effectively implemented as: new Thread(() => { Thread.sleep(100); OnEvent(); }; How would you use a Monitor or lock to have MyTest() wait for the event delegate to finish executing?
Patrick Huizinga
I mean: new Thread(() => { Thread.sleep(100); OnEvent(); }).Start();
Patrick Huizinga
You'd have the one thread WaitOne and the other Pulse. I'll try to add an example later...
Marc Gravell