search

awslabs / aws-java-nio-spi-for-s3

A Java NIO.2 service provider for Amazon S3
Apache License 2.0
58 stars 21 forks source link

`java.util.ConcurrentModificationException` after upgrading to 2.0.1 from 1.2.4 #474

Open hanneskaeufler opened 2 months ago

hanneskaeufler commented 2 months ago

Hi everyone,

when bumping to 2.0.1, we started seeing the following:

    java.util.ConcurrentModificationException
        at java.base/java.util.HashMap.computeIfAbsent(HashMap.java:1229)
        at software.amazon.nio.spi.s3.S3FileSystemProvider.getFileSystem(S3FileSystemProvider.java:775)
        at software.amazon.nio.spi.s3.S3FileSystemProvider.getPath(S3FileSystemProvider.java:251)
        at java.base/java.nio.file.Path.of(Path.java:209)
        [...] ommited rest of stacktrace

The exception is raised from the following (abbreviated) code:

listOfFiles
  .stream()
  .map(file -> Path.of(URI.create(file.path)).getFileName().toString())
  .toList();

which is executed in three cucumber-jvm 7.15.0 test scenarios which are executed in parallel. Configured by:

cucumber.execution.parallel.enabled = true

in junit-platform.properties.

I haven't seen anything in the 2.x changelogs which caught my eye. Any help is appreciated, thx!

markjschreiber commented 2 months ago

I think the problem you are encountering is that the underlying Map isn't synchronized/ thread safe and you have multiple threads trying to modify it as computeIfAbsent will attempt to add a key, value to a map if it isn't found.

private static final Map<String, S3FileSystem> FS_CACHE = new HashMap<>();

It would be simple enough to make this part thread safe, however there is likely other areas where errors might occur. Do you especially need thread safety? If so it would be a great thing to add and test thoroughly.

hanneskaeufler commented 1 month ago

Do you especially need thread safety?

I'd say so, running tests in parallel is a massive timesaver that we don't want to miss out on. I wouldn't know how to prevent all of us using this lib with any kind of concurrency, so making it robust in that regard seems advisable to me. If you have any pointers in how you'd approach this, I might be able to contribute something. Thanks for the prompt reply!

markjschreiber commented 1 month ago

Got it. I think we could think about adding appropriate synchronization blocks etc to support testing in parallel. I might be able to get to this in the coming weeks but would be happy to review any PRs if you have potential solutions.

My fear was more that you would need to have multi-threaded interactions with S3 which is fine except in the case of writes and parallel writes with reads. S3 itself can only support read after write consistency so full parallel safety would be very challenging for that.