Unexpected behaviour in Enum.GetValues or passing Enums

Question

I am writing a program that interacts with a 3rd-party program. This 3rd-party program allows the user to make buttons that can run recordings of steps made in the 3rd-party program. BUT! These buttons can also run a user-defined batch file. So I use this functionality to interact with my program by creating files, and checking if these files exist.

My program consist of two classes, an Actionlistener and an Actionperformer. The actionperformer contains a enum with possible actions.

The read function looks like this:

static public void CheckForActions()
{
    //For every action in the Enum
    foreach (ActionPerformer.PossibleActions action in Enum.GetValues(typeof(ActionPerformer.PossibleActions)))
    {
        //If a file exists with the same name as the current action
        if (File.Exists(_location + action.ToString()))
        {
            //Delete "message" and create a new thread to perform this action.
            File.Delete(_location + action);
            Thread _t = new Thread(() => 
            { 
                new ActionPerformer(action);
            });

            _t.SetApartmentState(ApartmentState.STA);
            _t.Start();

            //Add thread to list so they can be joined propperly
            _list.Add(_t);

            //Write information to log
            Logger.LogInfo(_t, "Starting new action: " + action.ToString(), DateTime.Now);
        }
    }

    //If there are items in the list
    if (_list.Count > 0)
    {
        //Dispose every thread once its done.
        foreach (Thread _t in _list)
        {
            _t.Join();
            Logger.LogInfo("Finishing action.", DateTime.Now);
        }
        _list.Clear();
    }
}

And the ActionPerformer class looks like this:

class ActionPerformer
{
    public enum PossibleActions
    {
        action1,
        action2,
    }

    public ActionPerformer(PossibleActions action)
    {
        Logger.LogInfo(action.ToString(), DateTime.Now);
    }
}

So when I run the program with action1 as parameter, and read the logger output I should get something like this:

Starting new action: action1 [13:30:05]
action1 [13:30:05]
Finishing action. [13:30:05]

But the first time I call CheckForActions I always get this as output:

Starting new action: action1 [13:30:05]
action2 [13:30:05] //Notice how it is not action1?
Finishing action. [13:30:05]

The second time I call CheckForActions, everything works as expected...

Does anyone know what is happening?

Answer 1

Assign a local variable to the iterated value, as the thread may get started on the next iteration, like this:

foreach (ActionPerformer.PossibleActions action in Enum.GetValues(typeof(ActionPerformer.PossibleActions)))
{
   var localAction = action;

   // Use localAction instead of action from here-on in
   ...
}

This seems like a "bug" with multi-threading, so the behaviour is fixed in .NET 4.5 so that the behaviour is that expected by developers.

Answer 2

You're having a closure.

Simpliest workarround : make a copy of action.

foreach (ActionPerformer.PossibleActions action in Enum.GetValues(typeof(ActionPerformer.PossibleActions)))
{
    //If a file exists with the same name as the current action
    if (File.Exists(_location + action.ToString()))
    {
        var actionCopy = action;

        //Delete "message" and create a new thread to perform this action.
        File.Delete(_location + action);
        Thread _t = new Thread(() => 
        { 
            new ActionPerformer(actionCopy);
        });

        _t.SetApartmentState(ApartmentState.STA);
        _t.Start();

        //Add thread to list so they can be joined propperly
        _list.Add(_t);

        //Write information to log
        Logger.LogInfo(_t, "Starting new action: " + action.ToString(), DateTime.Now);
    }
}

You could also pass action as a parameter of ThreadStart(object)

Answer 3

The problem is not in Enum.GetValues, but in the way how you're passing the value.

Thread _t = new Thread(() => 
{ 
     new ActionPerformer(action);
});

This one creates new closure, which contains REFERENCE to the variable action (and, therefore, when the 'action' value changes, the thread sees new value.

You can pass the 'action' to the Thread as an parameter.

Thread _t = new Thread((act) => 
{ 
     new ActionPerformer(act);
});
_t.Start(action);

or, use the approach suggested by others (create local variable in the foreach's body, and access it in closure.)

I think that Resharper emits warning, if you access modified variable in closure.

BTW, do not prefix local variables with underscore. It doesn't follow usual c# standard.