Reputation: 3343
I am having doubts about the volatile
keyword in Java when it comes to arrays (after reading a few questions here and specially this document ).
I have a series of threads reading information from an int array named cardArray
, and if an entry does not exist in the array I want to create it. What I use for that is a synchronized method like:
public synchronized void growArray(int id) {
if(getIndex(id) == -1) {
cardArray = Arrays.copyOf(cardArray, cardArray.length + 1);
cardArray[cardArray.length - 1] = id;
}
}
getIndex(int id) is a method which returns the position of such unique id in the array or -1 if the array does not exist. cardArray was declared like:
protected volatile int[] cardArray = new int[10];
The idea behind this is that two threads may want to make the array grow at the same time. Since the growArray()
method is synchronized, they will have to make it one by one. Once the first thread makes the array grow, the subsequent threads should not do it, since getIndex(id) should indicate that the id already exists. However, the fact that is the array what is declared as a volatile
and not its elements makes me suspicious about if this will be the actual behavior.
Is this what will happen or may the following threads accessing growArray()
still try to make the array grow? If so, is there any alternative apart from atomic arrays? I was thinking (in case this does not work as I want) to add cardArray = cardArray
after writing the element, but I do not know if it would make a difference.
Upvotes: 1
Views: 289
Reputation: 9366
Depending on your requirements, you may want to use ArrayList or HashSet or LinkedHashSet. If you're going to be writing much code in Java you'll really benefit from familiarizing yourself with the Java collections library.
Here is an explanation of some of the options available:
http://docs.oracle.com/javase/tutorial/collections/implementations/index.html
If you decide to use an ArrayList, your implementation would look like this:
// This is how you might create your list.
private List<Integer> myList = new ArrayList<Integer>();
public synchronized void growArray(int id) {
if( ! cardList.contains(id) ){
cardList.add(id);
}
}
If you use a HashSet (which guarantees uniqueness and grows automatically) you might do this:
// This is how you might create your list.
private Set<Integer> myList = new HashSet<Integer>();
public synchronized void growArray(int id) {
cardList.add(id);
}
Hope this helps!
Upvotes: 1
Reputation: 40256
Is this what will happen or may the following threads accessing growArray() still try to make the array grow?
If a thread exists the method with getIndex(id) == -1
being false, any thread that then enters the method will too see this update. This is because the method is synchronized. In fact you can remove volatile entirely and it would still work.
However, since you are updating the volatile, any non-synchronized
access would benefit from the volatile write, so you may want to keep it declared that way.
Upvotes: 0
Reputation: 18624
I don't see any issue with that. As far as I understand the cardArray
variable is volatile. You are changing the reference and since it is volatile
the second thread should see the new value and work with the updated array.
Problem I see is if both of your threads try to insert same id
at the same time so you end up with a duplicate value if that matters. So if one thread see a value missing, it will start to create a new array to insert it. But at the same time if another array is checking the same value, it will wait for the first thread to do its job. And then insert value without checking it already exists. You can fix that by checking for the value again.
Another issue is that if you have concurrent threads reading the array, there would be a short time when array will be longer but last value would not be what you expect. You may try to create array, update last value and lastly change the variable referencing it.
As comments say though, you'll be better using another data structure.
Upvotes: 0