thread safe in my code to manipulate List?

Hello everyone,


Here is my code for review to see whether it is thread-safety. My question is, after remove the object instance from List buffer, I exit the lock synchronization block in method RemoveAndUseItem, is it thread safe to exit the lock synchronization and call method RemoveAndUseItem (method RemoveAndUseItem will only touch the removed instance)?

Code:

---

        List<Foo> Buffer;
        object ListLock = new object();

        void AddItem(Foo f)
        {
            lock (ListLock)
            {
                Buffer.Add(f);
            }
        }

        void RemoveAndUseItem()
        {
            Foo LastItem;
            lock (ListLock)
            {
                LastItem = Buffer[Buffer.Count - 1];
                Buffer.RemoveAt(Buffer.Count - 1);
            }

            // no lock here? safe?
            SafeUseItem(LastItem);

            return;

        }

        void SafeUseItem(Foo f)
        {
            // use f here without synchronization control
        }

---

thanks in advance,
George

IF that is the ONLY place where you directly access the members inside Buffer, then yes it's thread safe.

If anything else has a copy of the pointer then it's not necessarily thread safe.

So it really all comes down to whether or not there is other code that's either manipulates Buffer, or other code that retained a pointer to a Foo object. For example, it's NOT thread safe if you did this:

Code:

---

        Foo f = new Foo();
        AddItem(f);
        f.Bar(); 
      // If f.Bar() happens at the same time that another thread is calling
      // RemoveAndUseItem() it could be thread unsafe.

---

Thanks Cat,


If multiple threads will access Buffer container, but only one thread will access SafeUseItem, then my code is thread safe, right?

Quote:

---

Originally Posted by Cat

IF that is the ONLY place where you directly access the members inside Buffer, then yes it's thread safe.

If anything else has a copy of the pointer then it's not necessarily thread safe.

So it really all comes down to whether or not there is other code that's either manipulates Buffer, or other code that retained a pointer to a Foo object. For example, it's NOT thread safe if you did this:

Code:

---

        Foo f = new Foo();
        AddItem(f);
        f.Bar(); 
      // If f.Bar() happens at the same time that another thread is calling
      // RemoveAndUseItem() it could be thread unsafe.

---

---

regards,
George

To be threadsafe, ANY method accessing your object needs to use the locking mechanism ( some locking mechanism, not neccessarily "lock()", but all need to use the same obviously). If a single one doesn't, it's not threadsafe.

Thanks nvoigt,


Any comments to my post #3?

Quote:

---

Originally Posted by nvoigt

To be threadsafe, ANY method accessing your object needs to use the locking mechanism ( some locking mechanism, not neccessarily "lock()", but all need to use the same obviously). If a single one doesn't, it's not threadsafe.

---

regards,
George

Your list is guarded by lock-blocks. Manipulating an object no longer in the list is threadsafe concerning the list. So the answer is yes.

You don't need an extra object. You can just "lock( Buffer )". That way, it's way more readable because it's obvious what you are locking and why.

Sidenote: Quoting full blocks of text is considered extremly bad style on the internet, because it adds volume without information. Quoting is required when you need to answer a specific point in another post. Quoting a post entirely therefore is contradictory to the intention of quoting something and as a result a waste of space. Maybe you can use "reply" instead of quoting half the thread.

Thanks nvoigt,


Your below answer is very clear. :-)

Quote:

---

Originally Posted by nvoigt

Your list is guarded by lock-blocks. Manipulating an object no longer in the list is threadsafe concerning the list. So the answer is yes.

You don't need an extra object. You can just "lock( Buffer )". That way, it's way more readable because it's obvious what you are locking and why.

---

regards,
George

Quote:

---

Originally Posted by George2

If multiple threads will access Buffer container, but only one thread will access SafeUseItem, then my code is thread safe, right?

---

The container code itself is thread-safe so long as all inserts, deletes, and iterations on the container are synchronized, e.g. by a lock. So yes, from what you've shown, it seems thread-safe.


The bigger question is -- do any other threads have a pointer to any of the objects inside your container? I mean, like this:

Code:

---


Foo a;        // Make a pointer to Foo
a = new Foo(); // Make it point to a Foo object
AddItem(a);    // It puts the object in the container


a.Bar();      // This line is potentially bad.  This object is in the container but you're also
              // manipulating it here.  Potentially thread unsafe depending on what Bar() does.

---

Thanks for sharing your points, Cat! There is no pointer leak in my code. But you are correct, I need to be careful not to expose pointer outside? :-)

Quote:

---

Originally Posted by Cat

The container code itself is thread-safe so long as all inserts, deletes, and iterations on the container are synchronized, e.g. by a lock. So yes, from what you've shown, it seems thread-safe.


The bigger question is -- do any other threads have a pointer to any of the objects inside your container? I mean, like this:

Code:

---


Foo a;        // Make a pointer to Foo
a = new Foo(); // Make it point to a Foo object
AddItem(a);    // It puts the object in the container


a.Bar();      // This line is potentially bad.  This object is in the container but you're also
              // manipulating it here.  Potentially thread unsafe depending on what Bar() does.

---

---

regards,
George