Need Java Synchronization Advice Vector / ConcurrentModificationException
In a legacy application, I have a Vector that keeps a chronological list of files to process, and multiple threads request it for the next file. (Note that I understand that there are probably better collections to use (feel free to suggest), but I don't have time to change this value right now.)
At the scheduled interval, another thread checks the working directory to see if any files are orphaned because something went wrong. The method called by this thread will sometimes throw a ConcurrentModificationException if the system is abnormally busy. So I know that at least two threads are trying to use the vector at once.
Here is the code. I believe the problem is using clone () for the returned vector.
private synchronized boolean isFileInDataStore( File fileToCheck ){
boolean inFile = false;
for( File wf : (Vector<File>)m_dataStore.getFileList().clone() ){
File zipName = new File( Tools.replaceFileExtension(fileToCheck.getAbsolutePath(), ZIP_EXTENSION) );
if(wf.getAbsolutePath().equals(zipName.getAbsolutePath()) ){
inFile = true;
break;
}
}
return inFile;
}
The getFileList () method looks like this:
public synchronized Vector<File> getFileList() {
synchronized(fileList){
return fileList;
}
}
As a quick fix, change the getFileList method to return a copy of the vector as it should be?
public synchronized Vector<File> getFileListCopy() {
synchronized(fileList){
return (Vector<File>)fileList.clone();
}
}
I have to admit that I am generally confused by the use of synchronized in Java as it relates to collections, since simply declaring a method as such is not enough. As a bonus question, declares the method as synchronized and wraps the callback with another synchronized block, just crazy coding? Looks overkill.
EDIT: Here are other methods that deal with the list.
public synchronized boolean addFile(File aFile) {
boolean added = false;
synchronized(fileList){
if( !fileList.contains(aFile) ){
added = fileList.add(aFile);
}
}
notifyAll();
return added;
}
public synchronized void removeFile( File dirToImport, File aFile ) {
if(aFile!=null){
synchronized(fileList){
fileList.remove(aFile);
}
// Create a dummy list so I can synchronize it.
List<File> zipFiles = new ArrayList<File>();
synchronized(zipFiles){
// Populate with actual list
zipFiles = (List<File>)diodeTable.get(dirToImport);
if(zipFiles!=null){
zipFiles.remove(aFile);
// Repopulate list if the number falls below the number of importer threads.
if( zipFiles.size()<importerThreadCount ){
diodeTable.put(dirToImport, getFileList( dirToImport ));
}
}
}
notifyAll();
}
}
source to share
Basically, there are two separate issues here: sycnhronization and ConcurrentModificationException. Vector
unlike, for example, it is ArrayList
synchronized internally, so the main operation, for example add()
or get()
, does not require synchronization. But you can get ConcurrentModificationException
even from a single thread if you iterate over Vector
and modify it in the meantime, eg. by inserting an element. So, if you performed the modification operation inside the loop for
, you can Vector
even split into one thread. Now, if you return yours Vector
outside of your class, you won't prevent anyone from modifying it without proper synchronization in your code. Synchronization on fileList
the original versiongetFileList()
meaningless. Returning a copy instead of the original might help, as would using a collection that allows changes on repeat, for example CopyOnWriteArrayList
(but note the additional cost of changes, in some cases it might be showstopper).
source to share
"I am usually confused about the use of synchronization in Java as it relates to collections, since simply declaring a method as such is not enough."
Right. synchronized
in a method means that only one thread can enter this method. But if the same collection is visible from multiple methods, it doesn't help much.
To prevent two threads from accessing the same collection at the same time, they need to synchronize the same object — for example, the collection itself. You did this in some of your methods, but it isFileInDataStore
appears to access the collection returned getFileList
without syncing on it.
Note that getting the collection in a synchronized manner, as it was done in getFileList
, is not enough - it is access that requires synchronization. Cloning the collection will (possibly) fix the problem if you only need read access.
As with synchronization, I suggest you keep track of which threads are involved - eg. print the call stack from the exception and / or use the debugger. Better to understand what's going on than just sync and clone until the errors go away!
source to share
First, you must move your logic to any m_dataStore class if you haven't.
Once you've done that, make your list final and sync ONLY if you change its elements. Topics to be read only do not need synchronous access. They may end up polling an outdated list, but I guess that's not a problem. This improves productivity.
As far as I can tell, you will only need to sync on add and remove, and you only need to lock your list.
eg.
packet response;
import java.util.logging.Level; import java.util.logging.Logger;
public class Example {
public static void main(String[] args)
{
Example c = new Example();
c.runit();
}
public void runit()
{
Thread.currentThread().setName("Thread-1");
new Thread("Thread-2")
{
@Override
public void run() {
test1(true);
}
}.start();
// Force a scenario where Thread-1 allows Thread-2 to acquire the lock
try {
Thread.sleep(1000);
} catch (InterruptedException ex) {
Logger.getLogger(Example.class.getName()).log(Level.SEVERE, null, ex);
}
// At this point, Thread-2 has acquired the lock, but it has entered its wait() method, releasing the lock
test1(false);
}
public synchronized void test1(boolean wait)
{
System.out.println( Thread.currentThread().getName() + " : Starting...");
try {
if (wait)
{
// Apparently the current thread is supposed to wait for some other thread to do something...
wait();
} else {
// The current thread is supposed to keep running with the lock
doSomeWorkThatRequiresALockLikeRemoveOrAdd();
System.out.println( Thread.currentThread().getName() + " : Our work is done. About to wake up the other thread(s) in 2s...");
Thread.sleep(2000);
// Tell Thread-2 that it we have done our work and that they don't have to spare the CPU anymore.
// This essentially tells it "hey don't wait anymore, start checking if you can get the lock"
// Try commenting this line and you will see that Thread-2 never wakes up...
notifyAll();
// This should show you that Thread-1 will still have the lock at this point (even after calling notifyAll).
//Thread-2 will not print "after wait/notify" for as long as Thread-1 is running this method. The lock is still owned by Thread-1.
Thread.sleep(1000);
}
System.out.println( Thread.currentThread().getName() + " : after wait/notify");
} catch (InterruptedException ex) {
Logger.getLogger(Example.class.getName()).log(Level.SEVERE, null, ex);
}
}
private void doSomeWorkThatRequiresALockLikeRemoveOrAdd()
{
// Do some work that requires a lock like remove or add
}
}
source to share