0

Suddenly a project I have been working on is throwing a ConcurrentModificationException when adding elements to a list and after trying to see what's going on for quite a while I am as confused as when this error first happened.

I have a Tournament class and it is composed of Event instances. Events have a list of timeslots List<Timeslot> timeslots (when the matches are to be played). When creating an instance of a tournament, the tournament's timeslots are all the event's timeslots combined. Timeslots shared by different events are only added once.

See how I am instantiating a tournament (I am omitting the irrelevant arguments):

List<Localization> localizations = TournamentUtils.buildGenericLocalizations(1, "Court");
List<Timeslot> timeslots = TournamentUtils.buildSimpleTimeslots(8);
Event firstEvent = new Event(/*...*/, timeslots.subList(0, 2)););
Event secondEvent = new Event(/*...*/, timeslots);

Tournament tournament = new Tournament("Tournament", firstEvent, secondEvent);

And this is how the relevant part of the tournament constructor looks like:

public Tournament(String name, List<Event> categories) {
    // ...

    allPlayers = new ArrayList<>();
    allTimeslots = new ArrayList<>();
    allLocalizations = new ArrayList<>();

    events = new ArrayList<>(categories);

    for (Event event : events) {
        event.getPlayers().stream().filter(p -> !allPlayers.contains(p)).forEach(allPlayers::add);
        event.getTimeslots().stream().filter(t -> !allTimeslots.contains(t)).forEach(allTimeslots::add);
        event.getLocalizations().stream().filter(l -> !allLocalizations.contains(l)).forEach(allLocalizations::add);
        event.setTournament(this);
    }

    // ...
}

public Tournament(String name, Event... categories) {
    this(name, new ArrayList<>(Arrays.asList(categories)));
}

The exception happens at the event.getTimeslots().stream().filter(t -> !allTimeslots.contains(t)).forEach(allTimeslots::add); line, I have tried doing the same with a traditional foreach instead of using streams, but the issue was exactly the same.

I honestly have no clue of why this is happening, since I am not modifying the list that's being iterated in any moment.

Edit. This is the other approach I tried, but I still get the same exception.

for (Event event : events) {
    for (Player player : event.getPlayers())
        if (!allPlayers.contains(player))
            allPlayers.add(player);

    for (Localization localization : event.getLocalizations())
        if (!allLocalizations.contains(localization))
            allLocalizations.add(localization);

    for (Timeslot timeslot : event.getTimeslots())
        if (!allTimeslots.contains(timeslot))
            allTimeslots.add(timeslot);

    event.setTournament(this);
}
dabadaba
  • 9,064
  • 21
  • 85
  • 155
  • 2
    You are modifying the list while iterating. Use a `for` loop. Related/dup: http://stackoverflow.com/questions/25832790/odd-concurrentmodificationexception – Idos Jun 21 '16 at 12:51
  • I can see that now. I am filtering based on whether the element is contained in the `allTimeslots` list and at the same time adding to that list. However, see my edit. I am still getting the error using the traditional approach. – dabadaba Jun 21 '16 at 12:56
  • You are still using foreach my good man, use a *traditional* `for`. – Idos Jun 21 '16 at 12:57
  • I still fail to see why that produces a `ConcurrentModificationException`. I must be missing something obvious. – dabadaba Jun 21 '16 at 12:59
  • Sorry but I don't see it. If I am iterating and NOT modifying list A, and not iterating but modifying (adding) to list B, where is the problem? I don't understand the issue, and I still can't reach a solution (how should I use a `for` loop according to you? Because I also tried using indices and the problem remains) – dabadaba Jun 21 '16 at 13:04
  • IMO the code you've posted contains no reason for the ConcurrentModificationException - you are iterating over different collections (Event class instance variables) than the ones you've gathering elements into (Tournament class instance variables), so I suppose there must be more in the code that is not shown here. Besides this, the Tournament constructor's signature doesn't fit the call (List of Events vs. variable length argument list or array). – Tomasz Stanczak Jun 21 '16 at 14:29
  • @TomaszStanczak thank you for being the only one seeing this... The other tournament constructor would be `public Tournament(String name, Event... categories) { this(name, new ArrayList<>(Arrays.asList(categories))); }` – dabadaba Jun 21 '16 at 14:57
  • I have a guess that I'll put in an answer in a moment - it seems to me to be too complex for a comment. @Idos: there are cases where a `for` loop works and `foreach` or `stream` won't, yet proposing `for` just as a single cure is wrong - `for` loop is just a syntax variation and depending on the usage can cause `ConcurrentModificationException` too. – Tomasz Stanczak Jun 22 '16 at 13:00
  • In the non-stream version, at which line during which pass does the exception happen? – Tomasz Stanczak Jun 22 '16 at 13:50
  • When accessing the list (for example, `event.getTimeslots().size()`). – dabadaba Jun 22 '16 at 14:01
  • There is no such call in the code above? And which pass - while processing the first or the second event? – Tomasz Stanczak Jun 22 '16 at 14:40
  • Let us [continue this discussion in chat](http://chat.stackoverflow.com/rooms/115321/discussion-between-tomasz-stanczak-and-dabadaba). – Tomasz Stanczak Jun 22 '16 at 14:41
  • No no, you didn't understand me. You asked where would the exception happen in the non-stream version. Well if I use a for and iterate, the exception happens at the `event.getTimeslots().size()` part of the `for`. (well, the whole line of the `for` declaration) – dabadaba Jun 22 '16 at 15:21
  • You have two events being processed, please check if it is the first or the second event. A list created by a `sublist` doesn't like it if the original list gets changed behind its back. Now as I asked in the chat - can it be that the Timeslots do change their collection automatically, like if the time of the Timeslot passes it removes itself from the Event? In which case the sublist might get irritated. – Tomasz Stanczak Jun 22 '16 at 17:04
  • It happens for the second event. No, the list of timeslots never change. The only time they change is when passed to the `Event` constructor. In that case the constructor automatically orders the timeslots. Other than that, no change. And when this change happens the event is already instantiated. – dabadaba Jun 22 '16 at 17:11

2 Answers2

1

The issue is not with iterating, but with the same list being passed to different events.

Event firstEvent = new Event(/*...*/, timeslots.subList(0, 2)););
Event secondEvent = new Event(/*...*/, timeslots);

Here firstEvent and secondEvent both contain the same list as Java sublist does not create a new list, but instead returns a view of the original list.

For the list timeslots used above, the same list is being passed different values of events, and later is used to iterate over the events, to fetch (albeit different views of the same list) the timeslots.

Due to this difference in the list view, a ConcurrentModificationException occurs.

Use new Arraylist(timeslots.sublist(0,2)) instead.

Jaiprakash
  • 599
  • 2
  • 7
  • Wrapping the list in an `ArrayList` solves the problem. However, I still don't get why the `ConcurrentModificationException` happens, despite your explanation. – dabadaba Jun 21 '16 at 13:16
  • @dabadaba CME happens when one thread interates over collection and another modifies(adds or removes elements) the same list. `times.sublist(0,2)` represents the same list as `timesLots` so modifying sublist fallsback to modication of orignal list thus CME. new ArrayList(sublist) solves the issue as this will create totally new, separate collection (array list in this case) that will contain only some of the elements of original list - but still it will be separate collection and CME will not be thrown. – Antoniossss Jun 21 '16 at 15:09
  • Quoting you: *"times.sublist(0,2) represents the same list as timesLots so modifying sublist fallsback to modication of orignal list thus CME"* My response to this: as I said before several times, that sublist you're talking about is **not** being modified. It is just being read. – dabadaba Jun 21 '16 at 16:40
  • I also think you're wrong, your answer is actually potentially harmful since it suggests that iterating over different views of the same list can cause `ConcurrentModificationException` - not true. You can iterate as many times over as many views of the same list as you like without any Exception. Iterating doesn't change anything so there is no reason for a MODIFICATION Exception (upper cased to stress the meaning). @Antoniossss there was no mention of threads, but even then, reading from the same list on multiple threads without modifying it shouldn't cause an exception. – Tomasz Stanczak Jun 22 '16 at 13:13
  • @TomaszStanczak So I as far as I understand, your stance here is that this is a bug in JRE as CME means exactly what I and jaiprakash wrote in the answer. Besides neither I no Jaiprakash wrote that parallel reading would cause CME, I dont know where did you read this. – Antoniossss Jun 22 '16 at 14:06
  • @Antoniossss you've mentioned threads in the first sentence of your first comment. What you have written about CME is actually correct - yet the code in the question contains no modifications of the original lists, so your argumentation which assumes the CME were right at this place is wrong - this was my point, not the description of a CME. And the answer of jaiprakash suggests that iterating over a sublist and the original list per itself may cause CME - which is also wrong. – Tomasz Stanczak Jun 22 '16 at 14:38
  • @Antoniossss the CME is not happening because of iteration over the lists. It occurs because this list was changed at some place (I assume, as there is no exception for the other 2 list scenario's). And this list was used to generate a sublist. Iterating over the list (without any add, update or delete) will not cause a CME. Also, as the OP stated, this issue goes away after wrapping into a new list, making my hypothesis that the list is changed at some place as correct. – Jaiprakash Jun 22 '16 at 16:00
  • @Jaiprakash again, this is exactly what I have stated - please read carefully. First sentence quote: "CME happens when one thread interates over collection **and another modifies** (adds or removes elements) the same list." – Antoniossss Jun 23 '16 at 07:31
0

First let's sum up the facts: you're experiencing ConcurrentModificationException which can happen in the context of your question if a "fail-fast" iterator recognizes its collection being modified while iterating.

(see ConcurrentModificationException)

Your code contains a list of events, each of them supplying lists of players, timeslots and localizations. They all are being iterated over, collecting unique objects into ArrayList instances being created just before the iteration.

The code doesn't modify any of the events' collections; it just adds elements to the newly created lists. No problems here, no reason for an exception. I've even built a small proof of concept containing just the relevant parts: two lists, one being a subview of the other and similar iteration with ArrayList collecting the elements - works as expected without any problems.

Still an exception happens – so something must be happening behind the back.

I promised my guess – since all of the lists are disconnected, all of the objects cannot modify its own collection neither in the iteration nor being added to separate lists (no code called except contains and add), I suppose either the Event class modifies its collections on a separate thread (yes I know you didn't mention threads) or the Tournament class does it (the events are being assigned to the event instance variable just before the iteration starts).

What actually happens if you change the code to iterate over the categories parameter and assign it after the for loop?

Tomasz Stanczak
  • 12,796
  • 1
  • 30
  • 32
  • The `Event` class is **never** modifying any of the lists. These are declared as `final` actually, so we can be sure of that. If I iterate `categories` instead of `events` and assign it after the loop, I still get the `ConcurrentModificationException`. – dabadaba Jun 22 '16 at 14:19
  • `final` is not enough - the variable pointing to the list is protected from modification, its content is not. – Tomasz Stanczak Jun 22 '16 at 14:30
  • Well, I am not modifying any of the lists anyway. – dabadaba Jun 22 '16 at 14:39
  • @dabadaba `final List someList =new ArrayList()` means that you will not be able to assign new value to `someField` so only calls like `someList=new LinkedList` futrher on will fail , but list itself is still mutable – Antoniossss Jun 22 '16 at 14:49
  • 1
    @dabadaba Then wrap lists you are passing into unmodifable collections and we will se what will happen.... http://docs.oracle.com/javase/1.5.0/docs/api/java/util/Collections.html#unmodifiableList(java.util.List) – Antoniossss Jun 22 '16 at 14:51
  • @Antoniossss good suggestion! – Tomasz Stanczak Jun 22 '16 at 14:52
  • When wrapping the lists in unmodifiable collections I am still getting a CME. – dabadaba Jun 22 '16 at 15:24
  • This sounds like the wrapping wouldn't be done right - the provision for a CME ist a modification, an `unmodifiableList` throws an UnsupportedOperationException on every attempt to modify the collection, which you didn't get, so the Timeslot list cannot have been modified, thus cannot have a reason to throw CME. Interesting problem you have :-), I'll vote up your question as soon as we'll find the reason... – Tomasz Stanczak Jun 22 '16 at 20:16