Concurrent activity on ArrayList

Question

I have an immutable Point class and a CollectionPoint class which manages instances of my Point class. The CollectionPoint class owns points under its control and must synchronize concurrent activity. I have below what I think is a thread-safe solution but would like another opinion. Is this solution thread-safe?

class Main {
    public static void main(String[] args) {
        int N = 10;

        double x = 0.0, y = 0.0;

        for(int i=0;i<N;i++) {
            x = (Math.random()*N);
            y = (Math.random()*N);
            Point p = new Point(x,y);
            Thread cp = new CollectionPoint(p);
            cp.start();

            try {
                cp.join();
            } catch(InterruptedException e) {}

        }

    }
}

class CollectionPoint extends Thread{
    // Class Needs to be thread safe
    private List<Point> list = new ArrayList<>();
    Point p;

    public CollectionPoint(Point pi) {
        p = pi;
    }

    public void run() {
        synchronized(this) {
            for(int i=0;i<1;i++) {
                add(p);
            }
        }
        display();
    }

    synchronized void add(Point x) {
        list.add(x);
    }

    synchronized boolean search(Point p) {
        for(int i=0;i<list.size();i++) {
            if(list.contains(p)) {
                return true;
            }
        }
        return false;
    }
    synchronized double getAllX(int x) {
        for(int i=0;i<list.size();i++) {
            if(list.contains(p.x() == x)) {
                return p.x();
            }
        }

        return 0.0;
    }

    synchronized void replace(Point p, Point new_p) {
        for(int i=0;i<list.size();i++) {
            if(list.contains(p)) {
                list.set(i, new_p);
            }
        }
    }

    synchronized void display() {
        for(Point x : list) {
            System.out.println("Point: "+x);
        }
    }
}

final class Point {
    // Class Needs to be thread safe
    private final double x,y;

    public Point(double x0, double y0) {
        x = x0;y = y0;
    }

    public double x() {return x;}
    public double y() {return y;}
    public String toString() {return "("+x+","+y+")";}
}

rolfl · Accepted Answer · 2014-10-19 14:59:45Z

Point

Your Point class is immutable, which makes it thread safe. So, you have that right. The code style is very compact, and not very readable. I would prefer you to have it as:

final class Point {
    // Class Needs to be thread safe
    private final double x, y;

    public Point(double x0, double y0) {
        x = x0;
        y = y0;
    }

    public double x() {
        return x;
    }

    public double y() {
        return y;
    }

    public String toString() {
        return "(" + x + "," + y + ")";
    }
}

Synchronization

OK, I started off writing a bunch of stuff here, but it's all sort of moot.

You do not run any concurrent threads.... and, even if the threads were concurrent, you are not actually running multiple threads against the same instances... you are not testing anything....

For example, you have the synchronized add() method. That's great, but, your test loop is:

public void run() {
    synchronized(this) {
        for(int i=0;i<1;i++) {
            add(p);
        }
    }
    display();
}

Now, since you synchronize outside the add method (and outside the loop), there's no other thread that can call the add anyway.

Still, that's not a problem, because your main method does:

    for(int i=0;i<N;i++) {
        x = (Math.random()*N);
        y = (Math.random()*N);
        Point p = new Point(x,y);
        Thread cp = new CollectionPoint(p);
        cp.start();

        try {
            cp.join();
        } catch(InterruptedException e) {}

    }

So, you create N threads. Each thread gets its own instance of CollectionPoint, so, none of the threads are adding content to the same CollectionPoint, there's no problem there...

but, even that is moot, because you only ever have one thread running at any one time...

You start the thread, and join to it in the same loop, so you only run one test thread, wait for it to complete, then run the next thread, etc.

Bottom line is that your code is not tested in a meaningful way, there is no concurrency on any one instance, and the actual test you do ensures that no other thread can access the class anyway.

So, your basic question is: "Is my solution thread safe?". The answer is "Yes", because you never allow more than one thread to run at any one time.

Thanks for the reply! Given that I've synchronized the methods in the CollectionPoint class they're going to run in a sequential form anyway, am i right in saying that? So in hindsight this solution is actually not concurrent at all? — Zy0n
– Zy0n, Commented Oct 19, 2014 at 15:07

toto2 · Accepted Answer · 2014-10-19 15:14:34Z

3

It's over-synchronized. You don't need to use synchronized a single time, considering what the main method is doing.

However, I suppose you did not code what you intended to do and it's hard for me to figure out what you wanted from the code only.

Your for-loops that use list.contains make no sense. You should probably re-read the doc on contains.

answered Oct 19, 2014 at 15:14

toto2

5,46017 silver badges21 bronze badges

Add a comment |

Stack Exchange Network

Concurrent activity on ArrayList

2 Answers 2

Point

Synchronization

You must log in to answer this question.

Hot Network Questions

Concurrent activity on ArrayList

2 Answers 2

Point

Synchronization

You must log in to answer this question.

Related

Hot Network Questions