CODEWITHSUNDEEP
javamultithreadingconcurrencyjava-8thread-safety
Mohamed Ibrahim Elsayed
Mohamed Ibrahim Elsayed

Reputation: 2964

Possibility of Race Condition on using Java Locks

I've written a Java class and someone has reviewed the code and insisted that there could be a race condition in method calculate. Here's a simplified version of the class code:

public class MyClass {
    private List<Integer> list;
    private final ReadWriteLock lock;

    public MyClass() {
        list = new ArrayList<>();
        lock = new ReentrantReadWriteLock();
    }

    public void add(Integer integer) {
        lock.writeLock().lock();
        try {
            list.add(integer);
        } finally {
            lock.writeLock().unlock();
        }
    }

    public void deleteAll() {
        lock.writeLock().lock();
        try {
            list.clear();
        } finally {
            lock.writeLock().unlock();
        }
    }

    public Integer calculate() {
        List<Integer> newList = new ArrayList<>();
        Integer result = 0;

        lock.readLock().lock();
        try {
            list.forEach(integer -> {
                // calculation logic that reads values from 'list' and adds only a subset of elements from 'list' in 'newList'
            });
        } finally {
            lock.readLock().unlock();
        }

        setList(newList);
        return result;
    }

    private void setList(List<Integer> newList) {
        lock.writeLock().lock();
        try {
            list = newList;
        } finally {
            lock.writeLock().unlock();
        }
    }
}

Now my question is:

Can a race condition really happen in this method, and if so how can I solve it (either using locks or using any other method to make the class thread safe)?

Any advice would be appreciated.

Upvotes: 2

Views: 272

Answers (2)

Mike Robinson
Mike Robinson

Reputation: 8945

To clarify the above ... the statement

List<Integer> newList = new ArrayList<>();

... instantiates a data-structure (list ...) that will subsequently be used within the block of code that is intended to be protected by lock.readLock().lock();, but is not contained within it. Therefore it is not protected.

To remedy the problem, the declaration of newList should not include initialization. Nothing which affects the presumed value of this variable should exist outside of the lock-protected block.

Upvotes: 1

Alexei Kaigorodov
Alexei Kaigorodov

Reputation: 13525

There is a time gap between creation of newList and call to setList(newList). We may assume this time gap is arbitrary long, and everything can happen when it lasts, e.g. another thread adds an object which must be retained, but it will be lost when call to setList(newList) removes list with that new object.

In fact, the method calculate is modifying and should do all the work under write lock.

Upvotes: 4

Related Questions