Re: C# locks and atomicity - C# Discussion Boards

Judah Gabriel Himango24-Jun-08 11:10

24-Jun-08 11:10

OnDoSomethingCompletedAsync is called only through the code shown above. In short, it can be only be called when DoSomething is finished executing asynchronously. And DoSomething won't be started if it's already running. Bah.

Joe Woodbury writes below that I should have declared the fields volatile. I thought using a lock would be sufficient, but I will try the volatile declaration.

Life, family, faith: Give me a visit.

From my latest post: "A lot of Christians struggle, perhaps at a subconscious level, about the phrase "God of Israel". After all, Israel's God is the God of Judaism, is He not? And the God of Christianity is not the God of Judaism, right?"

Judah Himango

User 665824-Jun-08 11:21

User 6658

24-Jun-08 11:21

I also considered using volatile, but I found out something different - I inserted some WriteLines to the methods, and this case happens quite a lot of times:

Enter Foo
Do something
Enter Result <---- huh? Shouldn't there be a lock?
Exit Result
Exit Foo

However, declaring the syncObject as static prohibits this case.

Maybe the lock isn't working properly?

regards

modified 12-Sep-18 21:01pm.

Joe Woodbury24-Jun-08 10:56

Joe Woodbury

24-Jun-08 10:56

I can see a very narrow circumstance where it will fail. The problem is that isWorking is not declared volatile, so you aren't guaranteed it will actually be set before OnDoSomethingCompletedAsync() is invoked. If the optimizer caches the value and writes it after the scope of the lock, you could have a problem IF the thread switch happens just before that instruction.

Try private volatile bool isWorking;

Anyone who thinks he has a better idea of what's good for people than people do is a swine.
- P.J. O'Rourke

Judah Gabriel Himango24-Jun-08 10:59

24-Jun-08 10:59

I considered that, but thought the lock keyword would take care of it. Volatile it is! Thanks.

leppie24-Jun-08 20:01

leppie

24-Jun-08 20:01

From what I can see, as you are using BeginInvoke, the lock will not be active between after the invoke and just before the completion. Just a guess.

xacc.ide - now with TabsToSpaces support
IronScheme - 1.0 alpha 4a out now (29 May 2008)

Judah Gabriel Himango25-Jun-08 5:01

25-Jun-08 5:01

Right, the lock won't be active. However, we set the isWorking boolean while holding the lock. Thus, no other work would fire. (Theoretically.)

My only thought is that changes to the isWorking field aren't immediately seen by other threads due to CPU caching, even with the locks in there. I'm gonna try Joe Woodbury's suggestion of making the field volatile. Unfortunately, since this happens so rarely, I won't be able to see whether this fixed it for several months. Bah.

FWIW, I also pointed MS threading guru Joe Duffy over to this thread. I'm hoping he can enlighten me. Smile | :)

Frank Horn24-Jun-08 22:36

24-Jun-08 22:36

If thread1 calls Foo and then thread2 calls Foo while DoSomething is running, it will be allright because thread2 can't enter the lock owned by thread1. But if thread1 calls Foo again while DoSomething is still running on behalf of thread1's first Foo call, it can go wrong. Thread1 can enter Foo because it's the owner of the lock, so it starts a second DoSomething pool thread. Now when the first DoSomething pool thread is done (after the second DoSomething pool thread has started) it will set isWorking to false and release the lock in OnDoSomethingCompletedAsync. So when the second DoSomething pool thread is done and no one has meanwhile called Foo again, the second DoSomething pool thread will enter OnDoSomethingCompletedAsync and find isWorking = false.

Judah Gabriel Himango25-Jun-08 5:06

25-Jun-08 5:06

Thanks for your detailed response, Frank.

Let me see if I'm understanding your correctly. You're saying that if Thread1 holds the lock inside Foo, spawns DoSomething on another thread, and then while still holding the lock, spawns DoSomething on another thread again, this can happen?

Frank Horn25-Jun-08 21:30

25-Jun-08 21:30

Sure, it can happen, because Thread1 is the owner of the lock and entering Foo again only increases the count on the lock, so that Thread1 has to leave it twice before another thread can enter.

Judah Gabriel Himango26-Jun-08 5:12

26-Jun-08 5:12

Ok. Unfortunately, the thread doesn't enter the lock 2x. Frown | :(

Frank Horn26-Jun-08 7:50

26-Jun-08 7:50

OK, so that's not the reason. But isWorking can still be false in OnDoSomethingCompletedAsync:

1. Thread1 calls Foo and returns. Now isWorking=true, and PoolThread1 is processing DoSomething. No lock.
2. Thread2 calls Foo and returns. Still isWorking=true, and PoolThread2 is processing DoSomething. No lock.
3. PoolThread1 finishes, calls OnDoSomethingCompletedAsync and returns. Now isWorking=false. No lock.
4. PoolThread2 finishes, and because Foo has not been called again, it finds isWorking=false in OnDoSomethingCompletedAsync.

I don't know why you want isWorking to be true in OnDoSomethingCompletedAsync. If you really want to enforce it, you could enclose the whole DoSomething code inside a lock(syncObject) block. But that would be the end of parallel procession. So what is all this about? Or was it just an academic question?

Judah Gabriel Himango26-Jun-08 8:28

26-Jun-08 8:28

Frank Horn wrote:
1. Thread1 calls Foo and returns. Now isWorking=true, and PoolThread1 is processing DoSomething. No lock.
2. Thread2 calls Foo and returns. Still isWorking=true, and PoolThread2 is processing DoSomething. No lock.

Nope. Thread2 calls Foo and returns, yes, but it does NOT spawn PoolThread2 because it sees isWorking = true.

Frank Horn wrote:
I don't know why you want isWorking to be true in OnDoSomethingCompletedAsync

I want only 1 thread executing DoSomething at a time. The Debug assertion is a sanity check.

Frank Horn26-Jun-08 9:48

26-Jun-08 9:48

I want only 1 thread executing DoSomething at a time. The Debug assertion is a sanity check.

Then you could indeed run DoSomething within a lock(SyncObject) block. All threads wanting to call Foo would have to wait for the current DoSomething instance to finish.

Nope. Thread2 calls Foo and returns, yes, but it does NOT spawn PoolThread2 because it sees isWorking = true.

How does Thread2 see that isWorking = true? Does your class containing Foo a.s.o. have a public property IsWorking{get{lock(SyncObject){return isWorking;}}}, and is every call to xyz.Foo enclosed in an if(!xyz.IsWorking) block? Or in a while(true) block waiting for !xyz.IsWorking?

Judah Gabriel Himango26-Jun-08 12:08

26-Jun-08 12:08

Frank Horn wrote:
How does Thread2 see that isWorking = true?

Recall the Foo method code:

void Foo()
{
     lock (syncObject)
     {
         if (!isWorking)
         {
             isWorking = true;
             new Action(DoSomething).BeginInvoke(OnDoSomethingCompletedAsync, null);
         }
     }
}

Foo will be called many times during app run, but should only spawn DoSomething if it's not currently working.

Frank Horn26-Jun-08 20:44

Get Struct Element by string

26-Jun-08 20:44

Sorry, I forgot that. Then indeed it should never happen, which only leaves Joe Woodbury's explanation of isWorking being read from cache when Foo is entered the second time.

robert rodgers24-Jun-08 8:47

24-Jun-08 8:47

Hi All,

How do I access an element of a struct at runtime if I don't know what element I need to access at design time?

I get passed some data in basiclly key=value pairs. If I can access the element in a struct using the key I could update it with the value.

Thanks.
Rob

Judah Gabriel Himango24-Jun-08 10:24

24-Jun-08 10:24

Sounds like Reflection is what you need. But to say for sure, you'd have to give us more info.

robert rodgers25-Jun-08 3:34

25-Jun-08 3:34

Thanks for your time. I looked at the reflection information and it looks like that might work. But I think I need a little nudge in the right direction.

Thanks again. Rob

I want to turn something like this...

private List<Instruction> GetInstructions(string sInst)
        {
            List<Instruction> Instructions = new List<Instruction>();
            string[] aInst = sInst.Split(';');
            foreach (string i in aInst)
            {
                string[] u = i.Split('=');
                Instruction newInst = new Instruction();
                switch (u[0])
                {
                    case "Element1":
                        newInst.Element1 = u[1];
                        break;
                    case "Element2":
                        newInst.Element2 = u[1];
                        break;
                    case "Element3":
                        newInst.Element3 = u[1];
                        break;
                    case "Element4":
                        newInst.Element4 = u[1];
                        break;
                    case "Element5":
                        newInst.Element5 = u[1];
                        break;

                        //...
                }
                Instructions.Add(newInst);
            }
            return Instructions;
        }

Into something like this...

private List<Instruction> GetInstructions(string sInst)
{
    List<Instruction> Instructions = new List<Instruction>();
    string[] aInst = sInst.Split(';');
    foreach (string i in aInst)
    {
        string[] u = i.Split('=');
        Instruction newInst = new Instruction();
        ptrToElement = GetPointerToElement(newInst, u[1]);
        ptrToElement = u[0];
        Instructions.Add(newInst);

    }
    return Instructions;
}

robert rodgers25-Jun-08 3:42

25-Jun-08 3:42

That should have been more like this.

Thanks.
Rob

private List<Instruction> GetInstructions(string sInst)        
{            
    List<Instruction> Instructions = new List<Instruction>();            
    string[] aInst = sInst.Split(';');            
    foreach (string i in aInst)            
        {                
            string[] u = i.Split('=');                
            Instruction newInst = new Instruction();                
            ptrToElement = GetPointerToElement(newInst, u[0]);                
            ptrToElement = u[1];                
            Instructions.Add(newInst);            
        }            
        return Instructions;        
}

Judah Gabriel Himango25-Jun-08 4:55

25-Jun-08 4:55

Ok, we can make this work. Here's what it would look like:

private List<Instruction> GetInstructions(string sInst)
{
     List<Instruction> instructions = new List<Instruction>();
     string[] aInst = sInst.Split(';');
     foreach (string i in aInst)
     {
         string[] u = i.Split('=');
         Instruction newInst = new Instruction();
         string elementName = u[0];
         string elementValue = u[1];
         SetElementValue(newInst, elementName, elementValue);
         instructions.Add(newInst);

     }

     return instructions;
}

void SetElementValue(Instruction input, string elementName, string elementValue)
{
   // Use Reflection to get the property on the type.
   // Then set the value of that property.
   PropertyInfo property = typeof(Instruction).GetProperty(elementName, BindingFlags.Public | BindingFlags.Instance);
   if(property != null)
   {
       property.SetValue(input, elementValue, null);
   }
}

robert rodgers25-Jun-08 5:22