Wednesday, 24 May 2023

ConcurrentModificationException and Streams - what not to do

I was wondering what shenanigans I can get up to with streams and mutating the underlying collection, and what a terrible idea this is.

As we all know, it's a really bad idea to mutate a list on which you are iterating, and Java makes certain this fails with a ConcurrentModificationException.

It all starts with the original problem, a for-loop that also mutates the list on which is iterates:

  @Test(expectedExceptions = ConcurrentModificationException.class)
  public void simpleTest() {
    List<Long> longs = new ArrayList<>(List.of(12L, 13L, 16L, 20L, 55L, -5L, 12L, 5L, 100L, 1000L));
    for (Long aLong : longs) {
      if (aLong > 50L || aLong < 0L) {
        longs.remove(aLong);
      }
    }
    assertThat(longs).isEqualTo(List.of(12L, 13L, 16L, 20L, 12L, 5L));
  }

Nowadays, we can use the forEach method on a collection, turning it into something like this with exactly the same problem:

  @Test(expectedExceptions = ConcurrentModificationException.class)
  public void forEachTest() {
    List<Long> longs = new ArrayList<>(List.of(12L, 13L, 16L, 20L, 55L, -5L, 12L, 5L, 100L, 1000L));
    longs.forEach(t -> {
      if (t > 50L || t < 0L) {
        longs.remove(t);
      }
    });
    assertThat(longs).isEqualTo(List.of(12L, 13L, 16L, 20L, 12L, 5L));
  }

If you are using a stream, the same problem occurs, as the stream refers to the underlying collection:

  @Test(expectedExceptions = ConcurrentModificationException.class)
  public void streamTest() {
    List<Long> longs = new ArrayList<>(List.of(12L, 13L, 16L, 20L, 55L, -5L, 12L, 5L, 100L, 1000L));
    longs.stream()
        .filter(Objects::nonNull)
        .filter(t -> t > 50L || t < 0L)
        .forEach(longs::remove);
    assertThat(longs).isEqualTo(List.of(12L, 13L, 16L, 20L, 12L, 5L));
  }

One way, though admittedly not a great way, to fix this, is to transfer it to a list first. Like so:

  @Test
  public void streamWithToListTest() {
    List<Long> longs = new ArrayList<>(List.of(12L, 13L, 16L, 20L, 55L, -5L, 12L, 5L, 100L, 1000L));
    longs.stream()
        .filter(t -> t > 50L || t < 0L)
        .collect(Collectors.toList())
        .forEach(longs::remove);
    assertThat(longs).isEqualTo(List.of(12L, 13L, 16L, 20L, 12L, 5L));
  }

But my colleague at work mentioned the new removeIf function, which seems to work particularly well:

  @Test
  public void removeIfTest() {
    List<Long> longs = new ArrayList<>(List.of(12L, 13L, 16L, 20L, 55L, -5L, 12L, 5L, 100L, 1000L));
    longs.removeIf(t -> t > 50L || t < 0L);
    assertThat(longs).isEqualTo(List.of(12L, 13L, 16L, 20L, 12L, 5L));
  }

Of course, the whole thing is patently ridiculous, because you might as well just filter unwanted elements out of the stream and create a new list from it.

But in some cases, where the "remove" is not as simple as chucking an element out of a collection (for example something as involved as, oh, I don't know, ImportantRestService.removeItem(Item item)), one has to find other ways to deal with it, which is where the second-to-last solution might come in handy.

I don't know. I'm open to suggestions.

No comments:

Post a Comment