0

I have 3 servers and a client sending messages. And i implement a BFT algorithm. So i have this part of code

    int tam = 0;

    if (unordered.size() <= maxOrderSize) {
        tam = unordered.size();
    } else {
        tam = maxOrderSize;
    }
    HashMap<String, byte[]> prop = new HashMap<String, byte[]>(tam);

    Iterator<String> it = unordered.keySet().iterator();

    for (int i = 0; i < tam; i++) {
        if (it.hasNext()) {
            String id = it.next();
            prop.put(id, unordered.get(id));
            it.remove();
            unordered.remove(id);

        }
    }

and during the runtime objects are imported and removed from my Map unordered. also i want to mention that unordered is defined:

    Map<String, byte[]> unordered = Collections.synchronizedMap(new HashMap<String, byte[]>());

But suddenly it creates this exception:

java.util.ConcurrentModificationException
at java.util.HashMap$HashIterator.remove(HashMap.java:1456)
at edu.bft.comm.layer.BatchControl.createOrderMessage(BatchControl.java:123)
at edu.bft.comm.layer.BatchControlTPM.run(BatchControlTPM.java:24)

Any idea why this happens?

EDIT1: I tried to remove that line: unordered.remove(id);

and i got that error:

java.util.ConcurrentModificationException
at java.util.HashMap$HashIterator.nextNode(HashMap.java:1442)
at java.util.HashMap$KeyIterator.next(HashMap.java:1466)
at edu.bft.comm.layer.BatchControl.createOrderMessage(BatchControl.java:120)
at edu.bft.comm.layer.BatchControlTPM.run(BatchControlTPM.java:24)

EDIT2: Also i want to mention that while i iterate unordered ,some new objects may added, while new messages are coming from the client.

hunter32
  • 21
  • 1
  • 6
  • 1
    The whole `for`-loop seems unnecessary and wrong. Wouldn't `while(it.hasNext(){ String id = it.next(); ...}` make a lot more sense? – Kevin Anderson May 11 '18 at 01:40
  • Yes, it probably should be a while loop as Kevin suggets. Additionally, what you really want is an iteration over the `entrySet()` instead the `keySet()`. By using the wrong Iterator, you need to make a collection lookup within `prop.put(...)`. And maybe by using the entrySet's iterator, its `remove()` operation might work. – Marcus K. May 11 '18 at 01:52
  • I update my first post check the full code. and edit2. Thanks – hunter32 May 11 '18 at 01:54
  • @KevinAnderson no i don't want to remove all the objects but just a specific number specified from **tam** – hunter32 May 11 '18 at 01:56
  • @MarcusK. check this output https://pastebin.com/eCFLd3Rn i added a print before the for loop and one print after for loop this output is with already remove the line unordered.remove(id); – hunter32 May 11 '18 at 02:04

3 Answers3

2

You're modifying the Map yourself.

Remove this line: unordered.remove(id);

From your edit, you're modifying the Map while iterating it so obviously it's the problem.

The doc said:

The set is backed by the map, so changes to the map are reflected in the set, and vice-versa. If the map is modified while an iteration over the set is in progress (except through the iterator's own remove operation), the results of the iteration are undefined.

And HashMap will throw a ConcurrentModificationException as you see.

Mạnh Quyết Nguyễn
  • 17,677
  • 1
  • 23
  • 51
  • Ok i removed that line and i started again my program. for now it works ok. I will wait to see if it will end normally. Thanks – hunter32 May 11 '18 at 01:34
  • please check first post i updated it with what happened – hunter32 May 11 '18 at 01:37
  • My code run without problem. Did you post your full code? – Mạnh Quyết Nguyễn May 11 '18 at 01:44
  • 1
    You still have ` unordered.remove(id);` in your code – Mạnh Quyết Nguyễn May 11 '18 at 01:58
  • check EDIT1.. i described what happened after removing that line – hunter32 May 11 '18 at 02:02
  • From your EDIT2: You're modifying your map from another place? – Mạnh Quyết Nguyễn May 11 '18 at 02:04
  • Yes from an another method in that class where when i receive a new message i add it in unordered map – hunter32 May 11 '18 at 02:10
  • ok thanks. Meybe a solution is todecrease how fast clients send messages. Because when with a lesser messaging rate, I do not have this problem – hunter32 May 11 '18 at 02:13
  • If you're using your Map to exchange data like that then you're doing it wrong. You continuously put data to the Map but in the consumer side, you only iterate over it once. – Mạnh Quyết Nguyễn May 11 '18 at 02:16
  • Do yourself a favour and fix the root of the problem instead of applying a hotfix that "will do most of the time". Either change the implementation of your HashMap, or try to synchronize access to the HashMap via the `synchronized`keyword (if you really cannot change the Map's implementation type)... – Marcus K. May 11 '18 at 02:17
  • unordered is defined like that: Map unordered = Collections.synchronizedMap(new HashMap()); – hunter32 May 11 '18 at 02:18
  • i changed it to ConcurrentHashMap unordered = new ConcurrentHashMap(); and i wait to see wthat will happen! – hunter32 May 11 '18 at 02:25
  • Using ConcurrentHashMap iterator will never throw ConcurrentModificationException but it's not guaranteed that the iterator see the update/new data incoming. So it's up to your decision – Mạnh Quyết Nguyễn May 11 '18 at 02:34
  • for now it works well. The project is about implementing a BFT algorithm. all messages removed from unordered are added to prop and then i send prop to the other servers... Right now i have 2 clients sending a message every 50 milliseconds....it works well for now comparatively the Collections.synchronizedMap(new HashMap()) implementation i had before – hunter32 May 11 '18 at 02:40
0

OK, after Edit2, I feel that another answer makes sense.

First of all: If you have a standard HashMap, it is not thread safe by default! So iterating over such a collection may always cause trouble if other threads concurrently modify it. Maybe adding new items won't cause an exception, maybe it will.

If you have the possibility to change the Map implementation of unordered, use something thread safe like a ConcurrentHashMap. This one won't have any trouble if someone modifies it while another thread iterates over its elements with an iterator - the iterator has its own collection of items which won't directly interfere with the original Map.

If you cannot change the implementation, you're in trouble...

Marcus K.
  • 980
  • 11
  • 26
  • for now unordered is: Map unordered = Collections.synchronizedMap(new HashMap()); and you say to change it to ConcurrentHashMap unordered = new ConcurrentHashMap(); – hunter32 May 11 '18 at 02:16
  • Correct. And then re-think your application design: Do you have a producer-consumer problem? Does every put into `unsorted` need to be pushed into `prop`? If so, you definitely need a more complex solution... – Marcus K. May 11 '18 at 02:25
  • The project is about implementing a BFT algorithm. all messages removed from unordered are added to prop and then i send prop to the other servers... – hunter32 May 11 '18 at 02:34
  • Wow, ok, that's a nice challenge. I hope you'll finally successfully complete your implementation. Or you might include an existing library which already implemented BFT. Whatever your final solution will be, it probably won't use the Map interface anymore.. – Marcus K. May 11 '18 at 03:01
0

That's exactly what ConcurrentModificationException prevented:

For example, if a thread modifies a collection directly while it is iterating over the collection with a fail-fast iterator, the iterator will throw this exception.

In general, you cannot modify the collection when the iterator is iterating. See the SO answer and this one.

Hong Duan
  • 4,234
  • 2
  • 27
  • 50