3

I have the following code which gives a java.util.concurrentmodificationexception.

I'm not an expert working with threads, but I thought that if I have a synchronized list, it should be thread safe...

EDIT: This is the full code of my method.

@Override
    protected List< ExportSchedule > export( List< ExportSchedule > exportSchedules )
    {
        final HandleSystemDoiAdministrator handleSystemDoiAdministrator = HandleSystemDoiAdministratorFactory.getInstance();
        final List< ExportSchedule > successfullyExported = new ArrayList<>();
        final List<ExportError> unsuccessfullyExported = Collections.synchronizedList( new ArrayList<ExportError>() );

        ExecutorService executorService = Executors.newFixedThreadPool( 10 );

        for ( final ExportSchedule exportSchedule : exportSchedules )
        {
            executorService.execute( new Runnable() {
                public void run()
                {
                    String doi = exportSchedule.getDoi().getDoi();
                    String url = exportSchedule.getDoi().getUrl();

                    boolean success = handleSystemDoiAdministrator.updateDoiHandle( doi, url );

                    if ( success )
                    {
                        successfullyExported.add( exportSchedule );
                    }
                    else
                    {
                        if ( handleSystemDoiAdministrator.isWarn() )
                        {
                            DoiErrorHelper.persistExportError(
                                ExportInterface.HANDLE_SERVER,
                                doi,
                                "Warning: Error exporting DOI " + doi + " with URL " + url + " to Handle Server: "
                                        + handleSystemDoiAdministrator.getResponseOutcome().toString(),
                                exportSchedule.getDoi().getDoiPool() );
                        }
                        if ( handleSystemDoiAdministrator.isFatal() )
                        {


                            synchronized(unsuccessfullyExported) {
                            unsuccessfullyExported.add( DoiErrorHelper.createExportError( doi, "Fatal: Error exporting DOI " + doi + " with URL " + url + " to Handle Server: "
                                        + handleSystemDoiAdministrator.getResponseOutcome().toString(), null, new Date(), exportSchedule.getDoi().getDoiPool().getName(),
                                        exportSchedule.getDoi().getDoiPool().getDoiPrefix(), ExportInterface.HANDLE_SERVER ) );}
                        }
                    }
                }
            } );
        }

        executorService.shutdown();
        try
        {
            executorService.awaitTermination( 1, TimeUnit.MINUTES );
        }
        catch ( InterruptedException e )
        {
            // TODO Auto-generated catch block
            e.printStackTrace();
        }

        return successfullyExported;
    }

EDIT 2:

This is the error:

    [exec]
     [exec] [#|2014-05-08T10:16:12.951+0200|SEVERE|glassfish3.1.2|javax.enterprise.system.std.com.sun.enterprise.server.logging|_ThreadID=203;_ThreadName=Thread-2;|    at java.util.HashMap$HashIterator.nextEntry(HashMap.java:894)|
#]
     [exec]
     [exec] [#|2014-05-08T10:16:12.951+0200|SEVERE|glassfish3.1.2|javax.enterprise.system.std.com.sun.enterprise.server.logging|_ThreadID=203;_ThreadName=Thread-2;|    at java.util.HashMap$EntryIterator.next(HashMap.java:934)|#]
     [exec]
     [exec] [#|2014-05-08T10:16:12.951+0200|SEVERE|glassfish3.1.2|javax.enterprise.system.std.com.sun.enterprise.server.logging|_ThreadID=203;_ThreadName=Thread-2;|    at java.util.HashMap$EntryIterator.next(HashMap.java:932)|#]
     [exec]
     [exec] [#|2014-05-08T10:16:12.952+0200|SEVERE|glassfish3.1.2|javax.enterprise.system.std.com.sun.enterprise.server.logging|_ThreadID=203;_ThreadName=Thread-2;|    at java.util.AbstractMap.toString(AbstractMap.java:518)|#]
     [exec]
     [exec] [#|2014-05-08T10:16:12.952+0200|SEVERE|glassfish3.1.2|javax.enterprise.system.std.com.sun.enterprise.server.logging|_ThreadID=203;_ThreadName=Thread-2;|    at ch.ethz.id.wai.doi.export.handle.DoiExport2HSProcessing$1.r
un(DoiExport2HSProcessing.java:106)|#]
     [exec]
     [exec] [#|2014-05-08T10:16:12.952+0200|SEVERE|glassfish3.1.2|javax.enterprise.system.std.com.sun.enterprise.server.logging|_ThreadID=203;_ThreadName=Thread-2;|    at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoo
lExecutor.java:1145)|#]
     [exec]
     [exec] [#|2014-05-08T10:16:12.952+0200|SEVERE|glassfish3.1.2|javax.enterprise.system.std.com.sun.enterprise.server.logging|_ThreadID=203;_ThreadName=Thread-2;|    at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPo
olExecutor.java:615)|#]
     [exec]
     [exec] [#|2014-05-08T10:16:12.953+0200|SEVERE|glassfish3.1.2|javax.enterprise.system.std.com.sun.enterprise.server.logging|_ThreadID=203;_ThreadName=Thread-2;|    at java.lang.Thread.run(Thread.java:722)|#]
     [exec]
     [exec] [#|2014-05-08T10:16:28.552+0200|INFO|glassfish3.1.2|javax.enterprise.system.std.com.sun.enterprise.server.logging|_ThreadID=157;_ThreadName=Thread-2;|0|#]
     [exec]
     [exec] [#|2014-05-08T10:16:31.221+0200|SEVERE|glassfish3.1.2|javax.enterprise.system.std.com.sun.enterprise.server.logging|_ThreadID=222;_ThreadName=Thread-2;|Exception in thread "pool-41-thread-10" |#]
     [exec]
     [exec] [#|2014-05-08T10:16:31.223+0200|SEVERE|glassfish3.1.2|javax.enterprise.system.std.com.sun.enterprise.server.logging|_ThreadID=222;_ThreadName=Thread-2;|java.util.ConcurrentModificationException
     [exec]     at java.util.HashMap$HashIterator.nextEntry(HashMap.java:894)
     [exec]     at java.util.HashMap$EntryIterator.next(HashMap.java:934)
     [exec]     at java.util.HashMap$EntryIterator.next(HashMap.java:932)
     [exec]     at java.util.AbstractMap.toString(AbstractMap.java:518)
     [exec]     at ch.ethz.id.wai.doi.export.handle.DoiExport2HSProcessing$1.run(DoiExport2HSProcessing.java:106)
     [exec]     at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1145)
     [exec]     at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:615)
     [exec]     at java.lang.Thread.run(Thread.java:722)
     [exec] |#]

DoiExport2HSProcessing.java is the class containing the method and line 106 is where I add the error to the list.

Francesco
  • 2,350
  • 11
  • 36
  • 59
  • possible duplicate of [ConcurrentModificationException for ArrayList](http://stackoverflow.com/questions/3184883/concurrentmodificationexception-for-arraylist) – Damian Leszczyński - Vash May 08 '14 at 08:08
  • Are you sure this is an accurate summary of your code, and that there's nothing else using either `exportSchedules` or `unsuccessfullyExported` at the same time? – Boann May 08 '14 at 08:23
  • @Boann I edited my question with the full code of my method. As you can see, there is nothing else that uses the two lists – Francesco May 08 '14 at 08:37
  • 2
    Looking at the stack trace it seems that `concurrentmodificationexception` is happening on a `map` and not `list`. The code u shown doesnt have use any `maps` – Sajan Chandran May 08 '14 at 09:14
  • Only one instance of `handleSystemDoiAdministrator` is used by all threads which implies everything in there should be thread-safe too. – vanOekel May 08 '14 at 10:53

3 Answers3

3

ConcurrentModificationException has nothing to do with synchronized or thread safety directly. Multiple threads will make the condition for this exception (see below) invisible but it can also occur with one thread!

The synchronized list prevents you from accessing the list from multiple threads in an undefined state. But you still can iterate the elements while modifying the list. This will result in an ConcurrentModificationException.

You must not modify the list using add(), remove() etc. during iterating it. The only valid modifications are using methods of the Iterator (remove()) or ListIterator (add() and set())!

Arne Burmeister
  • 20,046
  • 8
  • 53
  • 94
  • The first statement is incorrect. Collection that has been modified by other thread will throw CME while being iterated over. – Alexey Malev May 08 '14 at 08:35
  • You do not have, but this exception also occurs when other thread does modify. Can you please correct your post so the readers won't get confused? – Alexey Malev May 08 '14 at 08:44
1

You have not synchronized the third list, successfullyExported.

That can't explain the exception from modifying unsuccessfullyExported, but I don't believe that's really happening. It can't be. Perhaps what's really happening is the DoiPool or HandleSystemDoiAdministrator (whatever they are) are also not synchronized.

Boann
  • 48,794
  • 16
  • 117
  • 146
  • At the moment `success` is always false, so that the fact that `successfullyExported` is not synch. is not relevant(thought later it will be :) ). Regarding DoiPool and HandleSystemDoiAdministrator, see that they are "simple" objects and I'm only reading them (without modifying them), shouldn't this be no problem? – Francesco May 08 '14 at 08:51
  • @Francesco If that's true then you cannot possibly be getting an exception. – Boann May 08 '14 at 08:56
  • This is what I thought too ;), but I get the error in the question (added) – Francesco May 08 '14 at 09:03
  • 1
    @Francesco Thanks. It's as I thought. You get the ConcurrentModificationException due to unsynchronized use of a shared **HashMap** in HandleSystemDoiAdministrator. I'm guessing it's the "response outcome", being simultaneously filled by `updateDoiHandle()` and iterated by the `toString()` calls. – Boann May 08 '14 at 09:12
0

From the documentation:

It is imperative that the user manually synchronize on the returned list when iterating over it.

Maybe what you really need is a CopyOnWriteArrayList instead?

Edit: Sorry, was a bit quick on the trigger. After careful examination of the code in question, and since the ConcurrentModificationException happens on an AbstractMap call to toString() and there is only one explicit call to toString() and the only place that actively seems to mutate state is this handleSystemDoiAdministrator.updateDoiHandle( doi, url ); line, I'm thinking the underlying cause is probably hidden somewhere inside that Class. As it seems to be a Singleton being used by many threads at once. I would check to make sure that it was threadsafe first.

Mikkel Løkke
  • 3,710
  • 23
  • 37