Нужна консультация по синхронизации Java Vector/ConcurrentModificationException

Question

В устаревшем приложении у меня есть Вектор, который хранит хронологический список файлов для обработки, а несколько потоков запрашивают его для последующего процесса. (Обратите внимание, что я понимаю, что есть, вероятно, лучшие коллекции для использования (не стесняйтесь предлагать), но сейчас у меня нет времени для изменения этой величины.)

В заданный промежуток времени другой поток проверяет рабочий каталог, чтобы увидеть, остались ли какие-либо файлы сиротами, потому что что-то пошло не так. Метод, вызываемый этим потоком, иногда выдает исключение ConcurrentModificationException, если система ненормально занята. Поэтому я знаю, что по крайней мере два потока пытаются использовать вектор сразу.

Вот код. Я считаю, что проблема заключается в использовании клона() для возвращаемого вектора.

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; 
} 

Метод

GetFileList() выглядит следующим образом:

public synchronized Vector<File> getFileList() { 
    synchronized(fileList){ 
     return fileList; 
    } 
} 

Как быстро исправить, было бы изменить метод GetFileList вернуть копию вектора следующим образом хватает?

public synchronized Vector<File> getFileListCopy() { 
    synchronized(fileList){ 
     return (Vector<File>)fileList.clone(); 
    } 
} 

Я должен признать, что я обычно путают с использованием синхронизированных в Java, как она относится к коллекциям, а просто объявить метод как таковой не достаточно. Как вопрос бонуса, объявляет метод как синхронизированный и обертывает обратный вызов другим синхронизированным блоком, просто сумасшедшим кодированием? Выглядит излишне.

EDIT: Ниже приведены другие методы, которые касаются списка.

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(); 
    } 
} 

Answer 1

Где обновляется m_dataStore? Это вероятный преступник, если он не синхронизирован.

Answer 2

В принципе, здесь есть две отдельные проблемы: sycnhronization и ConcurrentModificationException. Vector в отличие от, например, ArrayList синхронизирован внутри, поэтому основная операция, например add() или get(), не требует синхронизации. Но вы можете получить ConcurrentModificationException даже из одного потока, если вы выполняете итерацию по Vector и изменяете его, тем временем, например. путем вставки элемента. Итак, если вы выполнили операцию модификации внутри цикла for, вы можете сломать Vector даже одним потоком. Теперь, если вы вернете свой Vector вне своего класса, вы не запретите кому-либо изменять его без правильной синхронизации в своем коде. Синхронизация по fileList в оригинальной версии getFileList() бессмысленна. Возвращение копии вместо оригинала могло бы помочь, так как можно было использовать коллекцию, которая допускает модификацию при повторении, например CopyOnWriteArrayList (но обратите внимание на дополнительную стоимость модификаций, в некоторых случаях это может быть showstopper).

Answer 3

«Я обычно путают с использованием синхронизированных в Java, как это относится к коллекциям, а просто объявить метод как таковой не достаточно»

Correct. synchronized по методу означает, что в этот метод может войти только один поток. Но если одна и та же коллекция видна из нескольких методов, это не очень помогает.

Для предотвращения одновременного доступа двух потоков к одной и той же коллекции, их необходимо синхронизировать с тем же объектом - например. самой коллекции. Вы сделали это в некоторых своих методах, но isFileInDataStore, похоже, обращается к коллекции, возвращенной getFileList, без синхронизации на ней.

Обратите внимание, что получения коллекцию в синхронном порядке, как вы сделали в getFileList, не хватает - это доступ, который нуждается в синхронизации. Клонирование коллекции (возможно) устранит проблему, если вам нужен только доступ на чтение.

Как и при синхронизации, я предлагаю вам отслеживать, какие потоки задействованы - например. распечатать стек вызовов из исключения и/или использовать отладчик. Лучше понять, что происходит, чем просто синхронизировать и клонировать, пока ошибки не исчезнут!

Answer 4

Во-первых, вы должны переместить свою логику в любой класс m_dataStore, если вы этого не сделали.

Как только вы это сделали, сделайте свой список окончательным и синхронизируйте его ТОЛЬКО, если вы изменяете его элементы. Темы, которые нужно читать только, не нуждаются в синхронном доступе. Они могут закончить опрос устаревшего списка, но я полагаю, что это не проблема. Это повышает производительность.

Насколько я могу судить, вам нужно будет синхронизировать только при добавлении и удалении, и вам нужно только заблокировать свой список.

например.

упаковка ответ;

импорт java.util.logging.Level; импорт java.util.logging.Logger;

общественного класса 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 
} 

}