patternjavaMinor
Concurrent activity on ArrayList
Viewed 0 times
activityarraylistconcurrent
Problem
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 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+")";}
}Solution
Point
Your
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
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:
So, you create N threads. Each thread gets its own instance of
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.
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.
Code Snippets
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 + ")";
}
}public void run() {
synchronized(this) {
for(int i=0;i<1;i++) {
add(p);
}
}
display();
}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) {}
}Context
StackExchange Code Review Q#67225, answer score: 6
Revisions (0)
No revisions yet.