Using synchronizedList with for loop and adding items inside it

Question

I'm using

Collections.synchronizedList(new ArrayList<T>())

part of the code is:

list = Collections.synchronizedList(new ArrayList<T>());

public void add(T arg) {
    int i;
    synchronized (list) {
        for (i = 0; i < list.size(); i++) {
            T arg2 = list.get(i);

            if (arg2.compareTo(arg) < 0) {
                list.add(i, arg);
                break;
            }

        }

Is it right that for loop is actually using iterator and therefore I must wrap the for with synchronized?

Is it thread-safe to use synchronized and make addition inside it like I did here?

I'm sorry if these questions are very basic, I'm new to the subject and didn't find answers on the internet. Thank you!!

Yes, you must synchronize the loop as you did. Make additions inside the loop is thread-safe but your list should be immutable during the loop — Luca Montagnoli, Jun 19 '17 at 07:37
You may also be interested in reading [Why is there no SortedList in Java?](https://stackoverflow.com/questions/8725387/why-is-there-no-sortedlist-in-java). — Andy Turner, Jun 19 '17 at 07:59
Also: you will never add the first element to the list with this logic, so the list will remain forever empty :) — Andy Turner, Jun 19 '17 at 08:23
@AndyTurner thank you, I corrected my mistake. I need to save the maximum value of the list. I think that I need to update the maximum value inside the sychronized block as well? — user7366106, Jun 19 '17 at 09:06

Andy Turner · Accepted Answer · 2017-06-19T07:53:35.230

Is it right that for loop is actually using iterator and therefore I must wrap the for with synchronized?

There are two parts to your question.

Firstly, no, you're not using an iterator here, this is a basic for loop.

The enhanced for loop is the for loop which uses an iterator:

for (T element : list) { ... }

You can see in the language spec how this uses the iterator - search for where it says "The enhanced for statement is equivalent to a basic for statement of the form".

Secondly, even though you're not using an iterator, you do need synchronized. The two are orthogonal.

You are doing multiple operations (the size, the get and the add), with dependencies between them. You need to make sure that no other thread interferes with your logic:

the get depends on the size, since you don't want to try to get an element with index >= size, for instance;
the add depends on the get, since you're apparently trying to ensure the list elements are ordered. If another thread could sneak in and change the element after you get it, you might insert the new element in the wrong place.

You correctly avoid this potential interference this through synchronization over list, and creating the synchronizedList in such a way that nothing other than the synchronizedList can get direct access to the underlying list.

alexey28 · Answer 2 · 2017-06-19T08:02:17.587

1

If your arg2.compareTo(arg) never return 0 (zero) you can use TreeSet. Will be much more simple:

set = Collections.synchronizedSet(new TreeSet<T>());

public void add(T arg) {
    set.add(arg);
}

If you need hold same items (compareTo returns 0) then use the list:

list = new ArrayList<T>();

public void add(T arg) {
    synchronized (list) {
        int index = Collections.binarySearch(list, arg);
        list.add(index, arg);
    }
}

First and second cases complexity will be log(N) (10 for 1000 items). Your code complexity is N (1000 for 1000 items).

edited Jun 19 '17 at 08:02

answered Jun 19 '17 at 07:46

alexey28

5,170
1
20
25

1

You could just use `Collections.binarySearch` to find the element in the list, and then you wouldn't need the restriction of `arg2.compareTo(arg) != 0`. – Andy Turner Jun 19 '17 at 07:49
Thank you. Update second case with binarySearch – alexey28 Jun 19 '17 at 08:02

Using synchronizedList with for loop and adding items inside it

2 Answers2