Is the compute() function thread safe? Will multiple threads loop correctly over the list?
class Foo {
private List<Integer> list;
public Foo(List<Integer> list) {
this.list = list;
}
public void compute() {
for (Integer i: list) {
// do some thing with it
// NO LIST modifications
}
}
}
Considering that data does not mutate (as you mentioned in the comment) there will not be any dirty / phantom reads.
compute". Could be modified elsewhere.If the list is created specifically for the purposes of that method, then you're good to go. That is, if the list isn't modified in any other method or class, then that code is thread safe, since you're only reading.
A general recommendation is to make a read-only copy of the collection, if you're not sure the argument comes from a trustworthy origin (and even if you are sure).
this.list = Collections.unmodifiableList(new ArrayList<Integer>(list));
Note, however, that the elements of the list must also be thread-safe. If, in your real scenario, the list contains some mutable structure, instead of Integer (which are immutable), you should make sure that any modifications to the elements are also thread-safe.
If you can guarantee that the list is not modified elsewhere while you're iterating over it that code is thread safe.
I would create a read-only copy of the list though to be absolutely sure that it won't be modified elsewhere:
class Foo {
private List<Integer> list;
public Foo(List<Integer> list) {
this.list = Collections.unmodifiableList(new ArrayList<>(list));
}
public void compute() {
for (Integer i: list) {
// do some thing with it
// NO LIST modifications
}
}
}
If you don't mind adding a dependency to your project I suggest using Guava's ImmutableList:
this.list = ImmutableList.copyOf(list);
It is also a good idea to use Guavas immutable collections wherever you're using collections that aren't changing since they are inherently thread safe due to being immutable.
list is private and no reference is published, there is absolutely no need to wrap it into an unmodifiable list.
new ArrayList<>(list).unmodifiableList we need to make a copy since it only returns an immutable view of the underlying list. Collections.unmodifiableList(list) would accomplish nothing since the original list could still be modified outside the class. Using immutable variables wherever possible is a good practice so I'm not going to change my answer.You can easily inspect the behavior when having for example 2 threads:
public class Test {
public static void main(String[] args) {
Runnable task1 = () -> { new Foo().compute(); };
Runnable task2 = () -> { new Foo().compute(); };
new Thread(task1).start();
new Thread(task2).start();
}
}
If the list is guaranteed not to be changed anywhere else, iterating on it is thread safe, if you implement compute to simply print the list content, debugging your code should help you understanding it is thread safe.
There is thread safe list in cocncurent library. If you want thread-safe collections always use it. Thread-safe list is CopyOnWriteArrayList
This version
class Foo {
private final List<Integer> list;
public Foo(List<Integer> list) {
this.list = new ArrayList<>(list);
}
public void compute() {
for(Integer i: list) {
// ...
}
}
}
is thread-safe, if following holds:
list arg to ctor can't be modified during ctor run time (e.g., it is local variable in caller) or thread-safe itself (e.g., CopyOnWriteArrayList);compute won't modify list contents (just as OP stated). I guess compute should be not void but return some numeric value, to be of any utility...
final. final merely forbids changing the reference your field points to.
Foo object itself is thread-safe in respect to it's state, once constructed. I guess, this is what OP asked for...
synchronizing the methodcomputedoes nothing about modifications of the list from other methods/classes.