How to avoid concurrent access to a resource?

Question

I have four threads runs asynchronously using CompletableFuture as shown below in the code. and all of them should access "grownSeedXYList". sometime when i run the code i receive no errors, but some time i receive "java.util.concurrent.completionexception" and i thinkk this is because the "grownSeedXYList" is not synchronized.

please let me know how to synchronize "grownSeedXYList"?

update:

this.grownSeedXYList is a list that will be populated with some Point objects based on the runnable class used (GrowSeedSERun, GrowSeedNWRun, GrowSeedNERun, GrowSeedSWRun)

the four threads run using Compltable future

this.grownSeedXYList = new ArrayList<Point>();
this.growSeedFutureList = CompletableFuture.runAsync(new GrowSeedSERun(this.saliencyMat, this.seedXY, this.seedVal), this.growSeedExecutor);
                this.growSeedFutureList = CompletableFuture.runAsync(new GrowSeedNWRun(this.saliencyMat, this.seedXY, this.seedVal), this.growSeedExecutor);
                this.growSeedFutureList = CompletableFuture.runAsync(new GrowSeedNERun(this.saliencyMat, this.seedXY, this.seedVal), this.growSeedExecutor);
                this.growSeedFutureList = CompletableFuture.runAsync(new GrowSeedSWRun(this.saliencyMat, this.seedXY, this.seedVal), this.growSeedExecutor);
                CompletableFuture.allOf(this.growSeedFutureList).join();

GrowSeedSERun class:

private class GrowSeedSERun implements Runnable {

    private Mat saliencyMat = null;
    private double seedVal;
    private Point seedXY = null;

    public GrowSeedSERun(Mat saliencyMat, Point seedXY, double seedVal) {
        // TODO Auto-generated constructor stub
        this.saliencyMat = saliencyMat;
        this.seedXY = seedXY;
        this.seedVal = seedVal;
    }
    public void run() {
        // TODO Auto-generated method stub
        growSeedsSE(this.saliencyMat, this.seedXY, this.seedVal);
    }
}

growSeedsSE:

private void growSeedsSE(Mat saliencyMat, Point seedXY, Double seedVal) {
    // TODO Auto-generated method stub
    int origX = (int) seedXY.x;
    int origY = (int) seedXY.y;

    if ( this.withinRange(saliencyMat.get(origY, ++origX)[0]) ) {

        if ( (this.grownSeedXYList != null) && (!this.grownSeedXYList.contains(new Point(origX, origY))) ) {

            //Log.D(TAG, "growSeedsSE", "newX: origX: "+origX);
            //Log.D(TAG, "growSeedsSE", "newX: origY: "+origY);
            //Log.D(TAG, "growSeedsSE", "newX: value: "+saliencyMat.get(origY, origX)[0]);

            this.grownSeedXYList.add(new Point(origX, origY));

        } else {
            Log.D(TAG, "growSeedsSE", "point: "+ new Point(origX, origY)+" contained in the list");
        }
        this.growSeedsSE(this.saliencyMat, new Point(origX, origY), this.saliencyMat.get(origY, origX)[0]);

    } else if ( this.withinRange(saliencyMat.get(++origY, (int) this.seedXY.x)[0]) ) {
        origX = (int) this.seedXY.x;

        if ( (this.grownSeedXYList != null) && (!this.grownSeedXYList.contains(new Point(origX, origY))) ) {

            //Log.D(TAG, "growSeedsSE", "newY: origX: "+origX);
            //Log.D(TAG, "growSeedsSE", "newY: origY: "+origY);
            //Log.D(TAG, "growSeedsSE", "newY: value: "+saliencyMat.get(origY, origX)[0]);

            this.grownSeedXYList.add(new Point(origX, origY));

        }  else {
            Log.D(TAG, "growSeedsSE", "point: "+ new Point(origX, origY)+" contained in the list");
        }
        this.growSeedsSE(this.saliencyMat, new Point(origX, origY), this.saliencyMat.get(origY, origX)[0]);
    }
}

You haven't shown us what `grownSeedXYList` is or where it's initialized or declared. Also, if you're getting a `CompletionException`, post its stacktrace. — Sotirios Delimanolis, Jun 08 '15 at 15:22
You reassign `growSeedFutureList` four times in succession. What is the purpose of that? Are you trying to add the new `CompletableFutures` to a `List`? — pens-fan-69, Jun 08 '15 at 15:29
So it's an `ArrayList`. `ArrayList` are not thread safe, so if you're sharing the same instance across threads, you're in trouble. — Sotirios Delimanolis, Jun 08 '15 at 15:32
@pens-fan-69 actually i do not need the this.growSeedFutureList, but it seems i learned the completablefuture wrongly..please advise — Amrmsmb, Jun 08 '15 at 15:33
@SotiriosDelimanolis thanks, but can u please let me know how to avoid to be in trouble? — Amrmsmb, Jun 08 '15 at 15:34
@user2121 FYI, there is no need to check for a `null` `grownSeedXYList` given that you explicitly initialize it at the beginning and don't appear to reassign it anywhere. The `null` check would seem to unnecessarily complicate the code, particularly when you correctly synchronize on the `grownSeedXYList`. — pens-fan-69, Jun 08 '15 at 16:09

pens-fan-69 · Accepted Answer · 2015-06-08T16:34:22.400

2

Your grownSeedXYList is shared among the threads. Your easiest option would be to use declare it as follows:

this.grownSeedXYList = Collections.synchronizedList(new ArrayList<Point>());

That will cause the grownSeedXYList collection to be thread safe. FYI, I believe, in the absence of the complete walkback for the CompletionException that you probably received it on the call to join because on of the threads caught a ConcurrentModificationException

Edit: As called out by @user270349 in the comments below and as noted by the javadoc, you still will need to synchronize on grownSeedXYList if you iterate over it. In such a case, you would do the following:

synchronized(grownSeedXYList) {
    Iterator i = grownSeedXYList.iterator();
    while (i.hasNext())
        foo(i.next());
    }
}

However, no synchronization is necessary if you use a for/each block as in:

for (Point point : grownSeedXYList) {
    foo(point);
}

edited Jun 08 '15 at 16:34

answered Jun 08 '15 at 15:34

pens-fan-69

979
7
11

1

That is not enough if there are iterations over the list while other thread write to it. See this question http://stackoverflow.com/questions/15437799/i-used-synchronized-list-and-i-still-get-concurrentmodificationexception – aalku Jun 08 '15 at 15:40
Note that there are performance implications of using a synchronized collection, but in this case it's a necessary trade off because of the concurrency. You just wouldn't want to use it in places where concurrency wasn't an issue. – pens-fan-69 Jun 08 '15 at 15:40
1

@user270349 Good point. @user2121 didn't appear to be doing so in this case. However, as always it's good to note caveats like this! It's also called out in the JavaDoc for `Collections.synchronizedList()`, btw (http://docs.oracle.com/javase/8/docs/api/java/util/Collections.html#synchronizedList-java.util.List-). – pens-fan-69 Jun 08 '15 at 15:42
@user270349 I stand corrected. He is using `contains()` which does, of course, perform iteration. So good catch! – pens-fan-69 Jun 08 '15 at 15:58
No, I think you are wrong this time. Contains iterate but intenally so it is protected by the atomic synchronization. No thread can modify the list during the iteration. I meant external iteration, with a loop outside the list class. – aalku Jun 08 '15 at 16:23
@user270349 I actually took a look at the source code to satisfy myself and you are indeed correct. – pens-fan-69 Jun 08 '15 at 16:33

Jared Potter · Answer 2 · 2015-06-08T15:54:36.300

In Java, use a Mutex lock around the resource.

A safer lock with turn flags can be found here. https://docs.oracle.com/javase/tutorial/essential/concurrency/newlocks.html

import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReentrantLock;

public class main {

    public static void main(String[] args) 
    {
        final Lock lock = new ReentrantLock();

        try {
            lock.tryLock();
        } finally {
            lock.unlock();
        }
    }
}

How to avoid concurrent access to a resource?

2 Answers2