Clever Geek Handbook

Java - a thread-safe program

Consider the following piece of code:

public class Test
    {
        private List<Object> list;

        public Object get(int id)
        {
            return list.get(id);
        }

        public void add(Object el)
        {
            list.add(el);
        }

        public List<Object> getList()
        {
            return list;
        }
    }

I have to make sure it is thread safe (there will be no sync errors). I'm new to this, so my guess is:

  • We add synchronized

    to the methods add()

    and get()

    and add synchronized(Test.class)

    to the method getList()

    ;

  • We put static static list

    and all static methods, add synchronized

    ;

This is what I came up with, it is possible that none of them are right.

+3


source to share


4 answers


synchronize add

, get

and getList

additionally in getList

return UnmodifiableList

:

import java.util.Collections;
import java.util.LinkedList;
import java.util.List;

public class Test {
    private List<Object> list = new LinkedList<>();

    public synchronized Object get(int id) {
        return list.get(id);
    }

    public synchronized void add(Object el) {
        list.add(el);
    }

    public synchronized List<Object> getList() {
        return Collections.unmodifiableList(list);
    }
}


You need to sync as well getList

because it Collections.unmodifiableList

uses Iterator

from the supplied list inside to copy it, so it could give ConcurrentModificationException

if not synchronized

using the method add

.

+2


source


Your point # 1 seems to be valid. I would put synchronized

on get()

and add()

and getList()

, but I would not put anything static, but return a copy of the list when called getList()

.

Since you won't specify which type, I wrote snippet c ArrayList

as an example.

  public synchronized List<Object> getList()
  {
      return new ArrayList<Object>(list);
  }


If you want to emphasize good practices, you can return some kind of immutable collection so that the user of "this" class does not edit the list, believing that it might affect the state.

public synchronized List<Object> getList()
{
    return Collections.unmodifiableList(list);
}
+3


source


Using a static modifier means sharing the same field list

for all instances of the Test class.
Hence your class must be singleton and doesn't seem to be designed to do this.
Thus, using static is not necessarily the best way to meet your needs.

To make a class thread safe, you must restrict multiple threads from simultaneously changing its state.

So this needs to be changed:

    public List<Object> getList()

Otherwise, clients can modify the list instance at the same time.

You should return an unmodifiable list.

You should also synchronize the add and remove methods to avoid simultaneous add or remove operations.

This way the class is synchronized:

public class Test
    {
        private List<Object> list;

        public synchronized Object get(int id)
        {
            return list.get(id);
        }

        public synchronized void add(Object el)
        {
            list.add(el);
        }

        public synchronized List<Object> getList() {
           return Collections.unmodifiableList(list);
        }

    }

Now, if you make a call to these methods, you must also synchronize them, because between their calls another thread can change the state of the list.

For example, this code:

Test test = ...;
int index = ...;
Object myObject = ...;

if (test.get(index) != myObject){
    test.add(myObject);
}

should be written like this:

Test test = ...;
int index = ...;
Object myObject = ...;

synchronized(test){
  if (test.get(index) != myObject){
      test.add(myObject);
  }
}
+2


source


None of them are completely correct.

We add synchronization to the add () and get () methods and add a synchronized (Test.class) getList () method;

Add synced with add()

, get()

and getList()

. But adding synchronized(Test.class

) to getList()

is wrong because you are adding a class lock instead of an object level lock to access the object variable.

We make the list static and all the methods static, then add the synchronized one;

This is necessary if you only want one list for all instances of a member variable Test

=> of a class class instead of an object level member variable. Otherwise, just go for instance method level locking instead of class level locking. Follow the first approach.

For class level variables, use class level locks. For object level variables, use object level locks.

Refer to the oracle pages Synchronized Methods and Inner Locks and Synchronization for a better understanding of the concepts.

A few notes from the documentation pages:

Object level locking:

Creating these methods synchronized

has two effects:

  • First, it is not possible for two method calls synchronized

    on the same object to alternate. When one thread executes a method synchronized

    on an object, all other threads that call methods synchronized

    on the same object (suspend execution) until the first thread is done with the object.

  • Second, when a method synchronized

    completes, it automatically establishes a relationship between events and a subsequent method call synchronized

    on the same object. This ensures that changes in the state of the object are visible to all threads.

Class level locking:

You may be wondering what happens when a static method is called synchronized

, since the method static

is associated with a class and not an object. In this case, the thread acquires an inline lock on the object Class

associated with the class. Thus, access to static fields of a class is controlled by a lock other than a lock on any instance of the class.

+1


source






More articles:


All Articles