Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -60,11 +60,11 @@ public OpenPathTree open() {
}
}
try {
this.lastOpen = new SharedOpenArchivePathTree(openFs());
lastOpen = this.lastOpen = new SharedOpenArchivePathTree(openFs());
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am still not 100% sure what the intention behind the split into SharedArchivePathTree, SharedOpenArchivePathTree and CallerOpenPathTree is. A sentence of JavaDoc on each of those classes would help.

My hypothesis is that the author wanted to allow multiple instances of SharedOpenArchivePathTree per SharedArchivePathTree where:

  • The one stored in lastOpen is the preferred and longer living one
  • All others are closed immediately when they are disposed via close().
  • Calling openFs() multiple times on a single file (per new SharedOpenArchivePathTree(openFs())) does not make the underlying ZipFilesystemProvider throw a FileSystemAlreadyExistsException, because perhaps the SharedArchivePathTree.CACHE somehow prevents it? Or what else is expected to happen in such situation?

When I saw SharedArchivePathTree.lastOpen writes and reads being protected by OpenArchivePathTree.lock in SharedOpenArchivePathTree, I thought it was a mistake not to protect also writing to lastOpen in SharedArchivePathTree.open():

        try {
            lastOpen = this.lastOpen = new SharedOpenArchivePathTree(openFs());
        } catch (IOException e) {
            throw new UncheckedIOException(e);
        }

But then, I figured out that the reads/writes from SharedOpenArchivePathTree are protected by per SharedOpenArchivePathTree lock anyway, so multiple instances of SharedOpenArchivePathTree can still race on accessing it. So perhaps it was not really intended not to protect SharedArchivePathTree.lastOpen by those locks at all but rather, those concurrent reads/writes to SharedArchivePathTree.lastOpen are by design and not considered harmful. Could please anybody confirm that? Git blame shows @aloubyansky as an author but hard to say if he wrote it originally.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SharedArchivePathTree has a bit of a Javadoc explaining the idea behind it.
SharedOpenArchivePathTree is the OpenArchivePathTree impl of it.
CallerOpenPathTree is basically shielding the SharedOpenArchivePathTree from the same caller calling close() multiple times on it and so from decrementing the user counter.

You're right about lastOpen not being well protected though. I'll have another look into it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SharedArchivePathTree has a bit of a Javadoc explaining the idea behind it.

I was very thankful to see it :)

SharedOpenArchivePathTree is the OpenArchivePathTree impl of it.
CallerOpenPathTree is basically shielding the SharedOpenArchivePathTree from the same caller calling close() multiple times on it and so from decrementing the user counter.

Do we really need both Open* and Caller* flavors (and all abstraction layers above)?
Does OpenPathTree have to inherit from PathTree?
All those inheritance, inner-class and delegation links make it really hard to understand what is going on.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does OpenPathTree have to inherit from PathTree?

Yeah, OpenPathTree is something that needs to be closed but it otherwise a PathTree.

The whole complication is around caching and concurrent access to the same open archive.

Unfortunately, the test rarely fails for me w/o the fix.

} catch (IOException e) {
throw new UncheckedIOException(e);
}
return new CallerOpenPathTree(this.lastOpen);
return new CallerOpenPathTree(lastOpen);
}

private class SharedOpenArchivePathTree extends OpenArchivePathTree {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
package io.quarkus.paths;

import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.ArrayList;
import java.util.List;
import java.util.UUID;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.Future;
import java.util.concurrent.TimeUnit;
import java.util.function.Consumer;
import java.util.stream.Stream;

import org.assertj.core.api.Assertions;
import org.junit.jupiter.api.Test;

public class SharedArchivePathTreeTest {
private static final int WORKERS_COUNT = 128;

@Test
void nullPointerException() throws IOException, InterruptedException, ExecutionException {
/* Reproduce https://github.com/quarkusio/quarkus/issues/48220 */
stress((OpenPathTree opened) -> {
});
}

@Test
void closedFileSystemException() throws IOException, InterruptedException, ExecutionException {
/* Reproduce https://github.com/quarkusio/quarkus/issues/48220 */
stress((OpenPathTree opened) -> {
try {
Path p = opened.getPath("org/assertj/core/api/Assertions.class");
Files.readAllBytes(p);
} catch (IOException e) {
throw new RuntimeException(e);
}
});
}

static void stress(Consumer<OpenPathTree> consumer) throws IOException {
/* Find assertj-core jar in the class path */
final String rawCp = System.getProperty("java.class.path");
final String assertjCoreJarPath = Stream.of(rawCp.split(System.getProperty("path.separator")))
.filter(p -> p.contains("assertj-core"))
.findFirst()
.orElseThrow(() -> new AssertionError("Could not find assertj-core in " + rawCp));

/* Create a copy of assertj-core jar in target directory */
final Path assertjCoreJarPathCopy = Path.of("target/assertj-core-" + UUID.randomUUID() + ".jar");
if (!Files.exists(assertjCoreJarPathCopy.getParent())) {
Files.createDirectories(assertjCoreJarPathCopy.getParent());
}
Files.copy(Path.of(assertjCoreJarPath), assertjCoreJarPathCopy);

/* Now do some concurrent opening and closing of the SharedArchivePathTree instance */
final ArchivePathTree archivePathTree = SharedArchivePathTree.forPath(assertjCoreJarPathCopy);
final ExecutorService executor = Executors.newFixedThreadPool(WORKERS_COUNT);
final List<Future<Void>> futures = new ArrayList<>(WORKERS_COUNT);
try {
for (int i = 0; i < WORKERS_COUNT; i++) {
final Future<Void> f = executor.submit(() -> {
try (OpenPathTree opened = archivePathTree.open()) {
consumer.accept(opened);
}
return null;
});
futures.add(f);
}

// Ensure all tasks are completed
int i = 0;
for (Future<Void> future : futures) {
Assertions.assertThat(future)
.describedAs("Expected success at iteration %d", i++)
.succeedsWithin(30, TimeUnit.SECONDS);
}
} finally {
executor.shutdown();
}
}

}
Loading