static method - thread safety

[N Fel]

I've read a lot on threads, but have yet to see an answer for the following.

Is a static method thread-safe if:
1.) it's not accessing any static variables, and
2.) it's not modifying objects passed in?

I have a utility class for reporting which has static methods that accept a List. I'm wondering if it should be synchronized and why. Multiple users run reports simultaneously, so the same method can be called simultaneously by multiple threads (each passing in a different Vector, ArrayList, or Set). Here's a sample method in my utility class:

public static Double calculateTotal(Collection myItems) {
double dTotal = 0.0;

Iterator theIterator = myItems.iterator();

while (theIterator.hasNext()) {
MyItem theItem = (MyItem) theIterator.next();
dTotal += theItem.price;
}

Double theTotal = new Double(dTotal);
return theTotal;
}

Thanks so much for any insight you can offer.

[Henry Wong]

A method should be thread safe if it is not accessing anything that could be changed simultaneously by another thread. In your example, it is not thread safe because the collection that is passed to it, may be modified by another thread, which can cause your iterator to fail, with a concurrent modification exception.

Henry

[N Fel]

That sounds good, but does that mean I'd have the same issue if the method was not static? Synchronizing the method wouldn't help either, right?

How could I make it thread safe?

[Henry Wong]

Originally posted by N Fel:
That sounds good, but does that mean I'd have the same issue if the method was not static? Synchronizing the method wouldn't help either, right?

How could I make it thread safe?

Yup, same problem regardless of whether the method is static or not.

To make it thread safe, make sure no thread is running in parallel, for either reading or writing, while a thread is modifying the share data. You need to protected it somehow prior to calling your method.

Henry

[H. Hall]

This is an old thread, but I don't think the answer given is correct, and people (like me) still find this when they google, so here goes:

Henry, Java passes arguments to methods by value not by reference. This means that Java passes a copy of the original object to methods.

Therefore you cannot modify the original object within the method by performing an action on the argument because you are operating on a copy. And, for the same reason, the argument will not be modified when the original object is changed.

Therefore the example static method is thread safe.

--cheers

[Paul Clapham]

Originally posted by H. Hall:
Henry, Java passes arguments to methods by value not by reference. This means that Java passes a copy of the original object to methods.

If we're going to have corrections, let's have correct ones. Yours wasn't correct, because it's the reference to the object that is the parameter, not the object itself. Read this for a full explanation:

http://www.javaranch.com/campfire/StoryPassBy.jsp

[H. Hall]

If we're going to have corrections, let's have correct ones. Yours wasn't correct, because it's the reference to the object that is the parameter, not the object itself. Read this for a full explanation:

http://www.javaranch.com/campfire/StoryPassBy.jsp

Hmmmm. I looked at your reference and it appears to agree with me. You can find this about half way down:

NOW let's look at passing values to methods

Java is pass-by-value.

Always.

That means "copy the value, and pass the copy."

-- cheers

[H. Hall]

You are right. Java passes args by value only, and makes a copy of the thing being passed. But in the case of objects, the thing being copied is the pointer to the object. Else how could you call a method of an instance of an object being passed by an argument.

Duh.

--cheers

[Paul Clapham]

Yup. And Henry was right because in that example there was only one Collection and several references to it. That's exactly the situation where you need to worry about synchronization.

Good to see you're onside with the explanation. I haven't figured out why it goes over people's heads until the word "pointer" pops up, whereas the word "reference" doesn't seem to produce the same result.

[Henry Wong]

That was easy -- I didn't have to do anything...

Henry

[Manish Khurana]

I still do not get it. If each thread is working with a different instance of collection and once inside the static method the thread does not alter static variables and your code uses local variables that each thread has seperate. I do not think you woyld need synchronisation here as the threads will net be conflicting.

[Henry Wong]

If each thread is working with a different instance of collection ...

In this example, you don't know that. All you know is that a collection is passed to the static method. You don't know if each call to the static method has a different instance of the collection.

once inside the static method the thread does not alter static variables and your code uses local variables that each thread has seperate....

Not altering static variables (or parameters) is *not* good enough. If the method is using any shared variables, it needs to be synchronized. In this example, the method is only using the iterator, to fetch data, to use to calculate the sum -- it is not changing the values in the collection. However, this iterator can throw a concurrent modification exception if another thread changes the collection.

Henry

[Manish Khurana]

let me see if i understand you. Eevn if iterator is a local variable in a static method and so each thread has its own variable but both would point to the same Collection.

[Henry Wong]

Originally posted by Manish Khurana:
let me see if i understand you. Eevn if iterator is a local variable in a static method and so each thread has its own variable but both would point to the same Collection.

It's not very hard to prove this. It took me 10 minutes to write the following code to break the original poster's example.

// Code to break Method Above
public static void main(String[] args) throws Exception {
    ExecutorService pool = Executors.newFixedThreadPool(5);
    final Collection items = new ArrayList();
 
    pool.submit(new Callable() {
        public Object call() throws Exception {
            while (true) {
                for(double d = 0; d < 100000.0; d += 0.8765) {
                    items.add(new MyItem(d));
                }
                for (Iterator iter = items.iterator(); iter.hasNext();) {
                    MyItem element = (MyItem) iter.next();
                    iter.remove();
                }
            }
        }
    });
    Thread.sleep(1000);
 
    Double total = calculateTotal(items);
}

And I didn't change a single line of the original code.

Henry
[ January 30, 2008: Message edited by: Henry Wong ]

[Henry Wong]

BTW, since the OP never posted the MyItem class, I wrote a really quick and simple one...

public class MyItem {
    public double price;
 
    public MyItem(double startprice) {
        price = startprice;
    }
}

Henry

[Nicholas Jordan]

Originally posted by Henry Wong:
BTW, since the OP never posted the MyItem class, I wrote a really quick and simple one...

Thought I would build on it, I will leave it to you to point the investigators to the Uh-Oh,.... what happens now. comment in the code.

package demo;
 
/*
 * Demonstrates Thread Interleaving, does not use Executors.
 * Makes use of ArrayList, which is not synchronized.
 */
 
 public class ThreadedItem extends MyItem implements Runnable
 {
     protected static index1 = 0x0000;// This oughta make for some investigation room.
     protected Collection<Double> items = new ArrayList<Double>(10);
     protected ThreadedItem(double startprice)
     {
         super(startprice);//
     }
     public void run()
     {
         // We now put ten doubles in the ArrayList / Collection
         while(++index < 10)// simple, one would think
         {
             items.add(<new Double(startVal.nextDouble()));//
         }
         return;//
     }
     public static void main(String[] args) throws Exception 
     {
         int index2 = 20;// some arbitrary number picked for testing.
         while(--index > 0)
         {
             ThreadedItem ti = new ThreadedItem();// So far, so good on first entry.
             new Thread(ti).start();// Uh-Oh,.... what happens now.
         }
         return;// Now what items contains is really anyone's guess.
     }
 }

Nick

For what it is worth, this is really never an old question - it gets to the epicenter of what Threading is. I suggest readers try to see Threads as the machine in operation, separately from any source code that may be written.
[ January 30, 2008: Message edited by: Nicholas Jordan ]

[g venkata]

Nice Thread...

He can mark the caller method as synchronized to make

calculateTotal

thread safe.

Venkat Ram

[Dieter Quickfend]

Not necessarily. It all depends on the scope of the Collection. If the Collection is globally accessible, synchronizing the caller method will do little. To make this method thread safe, it needs to allow an immutable collection be passed as parameter and return a new, local Collection with the processed data. This way, no other thread can modify the data during the method call.