I2P-Bote concurrent.ExecutionException

[killyourtv]

I2P version: 0.9.22-21-deb1
Java version: Oracle Corporation 1.8.0_66-internal (OpenJDK Runtime Environment 1.8.0_66-internal-b17)
Wrapper version: 3.5.26
Server version: 8.1.17.v20150415
Servlet version: Jasper JSP 2.1 Engine
Platform: Linux amd64 4.2.0-1-amd64
Processor: Bulldozer FX-6100/8100 (athlon64)
Jbigi: Locally optimized native BigInteger library loaded from file
Encoding: UTF-8
Charset: UTF-8

Version	0.4.1
Signed by	str4d@mail.i2p
Date	2015-09-12 15:46
Author	str4d@mail.i2p

11/03/15 14:03:28.565 ERROR [EmailChecker] i2p.bote.service.EmailChecker : Error while checking whether new mail has arrived.
     java.util.concurrent.ExecutionException: java.util.concurrent.ExecutionException: java.util.ConcurrentModificationException
     at java.util.concurrent.FutureTask.report(FutureTask.java:122)
     at java.util.concurrent.FutureTask.get(FutureTask.java:206)
     at i2p.bote.service.EmailChecker.updatePendingTasks(EmailChecker.java:202)
     at i2p.bote.service.EmailChecker.isCheckingForMail(EmailChecker.java:152)
     at i2p.bote.service.EmailChecker.checkForMail(EmailChecker.java:112)
     at i2p.bote.service.EmailChecker.run(EmailChecker.java:230)
     Caused by: java.util.concurrent.ExecutionException: java.util.ConcurrentModificationException
     at java.util.concurrent.FutureTask.report(FutureTask.java:122)
     at java.util.concurrent.FutureTask.get(FutureTask.java:206)
     at i2p.bote.network.CheckEmailTask.call(CheckEmailTask.java:131)
     at i2p.bote.network.CheckEmailTask.call(CheckEmailTask.java:61)
     at java.util.concurrent.FutureTask.run(FutureTask.java:266)
     at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
     at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
     at java.lang.Thread.run(Thread.java:745)
     Caused by: java.util.ConcurrentModificationException
     at java.util.ArrayList$Itr.checkForComodification(ArrayList.java:901)
     at java.util.ArrayList$Itr.next(ArrayList.java:851)
     at i2p.bote.network.kademlia.BucketManager.getAllUnlockedPeers(BucketManager.java:268)
     at i2p.bote.network.kademlia.ClosestNodesLookupTask.call(ClosestNodesLookupTask.java:107)
     at i2p.bote.network.kademlia.KademliaDHT.getClosestNodes(KademliaDHT.java:180)
     at i2p.bote.network.kademlia.KademliaDHT.find(KademliaDHT.java:263)
     at i2p.bote.network.kademlia.KademliaDHT.findAll(KademliaDHT.java:190)
     at i2p.bote.network.CheckEmailTask$EmailPacketTask.run(CheckEmailTask.java:201)
     at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
     ... 4 more

[zzz]

On quick glance, the locking in BucketManager? looks shaky. See also previously fixed #1586 (or was it...)

May be desirable to someday migrate to common net.i2p.kademlia, although there may be features in HH's impl we need to backport first. Last I looked, his had some advantages. Also the whole SBucket thing... Anyway, just musing, irrelevant to this ticket.

[kay]

It seems (here and in the other ticket mentioned above) that
BucketManager?.getAllUnlockedPeers iterates over a bucket peer-list
that gets changed during the iteration. The List of peers is
a Collections.synchronizedList that mandates manual synchronization
when iterating over it.

I propose two solutions both concerning the AbstractBucket? class:

1) use CopyOnWriteArrayList? for the peers' List, i.e.

 import java.util.List;
+import java.util.concurrent.CopyOnWriteArrayList;
 
 import net.i2p.data.Destination;
 import net.i2p.data.Hash;
@@ -46,7 +49,7 @@ abstract class AbstractBucket implements
     protected int capacity;
     
     public AbstractBucket(int capacity) {
-        peers = Collections.synchronizedList(new ArrayList<KademliaPeer>());
+        peers = new CopyOnWriteArrayList<KademliaPeer>();
         this.capacity = capacity;
     }

2) return a copy-iterator, i.e. using i2p.bote.Util

@@ -109,5 +114,6 @@ abstract class AbstractBucket implements
     @Override
     public Iterator<KademliaPeer> iterator() {
-        return peers.iterator();
+        return Util.synchronizedCopy(peers).iterator();
     }

Solution no. 1 seems better b/c the iterator would throw
an UnsupportedOperationException? for modification attempts,
and thus fail fast.

[str4d]

I concur with solution 1, it's the same thing I did for BucketManager? in #1586.

Committed in f9edfbef5dfd2aa7109ff5761992774a6a82c249.

[str4d]

Haven't had any more reports after 0.4.2 release, assuming fixed.