From 9e4f160c17a4822ec62a5082816f7fb402f7ab8a Mon Sep 17 00:00:00 2001 From: Phillip Webb Date: Fri, 6 Oct 2023 23:44:55 -0700 Subject: [PATCH] Attempt to fix NestedJarFile file lock issues on Windows Update `DefaultCleanerTracking` and `@AssertFileChannelDataBlocksClosed` to capture and store the source object if it is a `Cleanable` so that it can be released later. Although the real cleaner cannot keep a reference to `obj`, it is safe for us to do so in tests since we are in control of the object lifecycle and we don't need it to be garbage collected. This commit also updates the `UrlJarFile` to call the cleaner so that it can be tracked. See gh-37668 --- .../loader/net/protocol/jar/UrlJarFile.java | 8 ++++++- .../net/protocol/jar/UrlNestedJarFile.java | 4 +++- .../boot/loader/ref/DefaultCleaner.java | 8 +++---- .../boot/loader/launch/ArchiveTests.java | 15 ++++++++---- .../protocol/jar/UrlJarFileFactoryTests.java | 2 ++ .../loader/ref/DefaultCleanerTracking.java | 4 ++-- ...tFileChannelDataBlocksClosedExtension.java | 24 ++++++++++++++----- 7 files changed, 47 insertions(+), 18 deletions(-) diff --git a/spring-boot-project/spring-boot-tools/spring-boot-loader/src/main/java/org/springframework/boot/loader/net/protocol/jar/UrlJarFile.java b/spring-boot-project/spring-boot-tools/spring-boot-loader/src/main/java/org/springframework/boot/loader/net/protocol/jar/UrlJarFile.java index 513d79799c..e70af3081a 100644 --- a/spring-boot-project/spring-boot-tools/spring-boot-loader/src/main/java/org/springframework/boot/loader/net/protocol/jar/UrlJarFile.java +++ b/spring-boot-project/spring-boot-tools/spring-boot-loader/src/main/java/org/springframework/boot/loader/net/protocol/jar/UrlJarFile.java @@ -24,6 +24,8 @@ import java.util.jar.Manifest; import java.util.zip.ZipEntry; import java.util.zip.ZipFile; +import org.springframework.boot.loader.ref.Cleaner; + /** * A {@link JarFile} subclass returned from a {@link JarUrlConnection}. * @@ -37,6 +39,8 @@ class UrlJarFile extends JarFile { UrlJarFile(File file, Runtime.Version version, Consumer closeAction) throws IOException { super(file, true, ZipFile.OPEN_READ, version); + // Registered only for test cleanup since parent class is JarFile + Cleaner.instance.register(this, null); this.manifest = new UrlJarManifest(super::getManifest); this.closeAction = closeAction; } @@ -53,7 +57,9 @@ class UrlJarFile extends JarFile { @Override public void close() throws IOException { - this.closeAction.accept(this); + if (this.closeAction != null) { + this.closeAction.accept(this); + } super.close(); } diff --git a/spring-boot-project/spring-boot-tools/spring-boot-loader/src/main/java/org/springframework/boot/loader/net/protocol/jar/UrlNestedJarFile.java b/spring-boot-project/spring-boot-tools/spring-boot-loader/src/main/java/org/springframework/boot/loader/net/protocol/jar/UrlNestedJarFile.java index 33bbed2e83..1f9f62b2a3 100644 --- a/spring-boot-project/spring-boot-tools/spring-boot-loader/src/main/java/org/springframework/boot/loader/net/protocol/jar/UrlNestedJarFile.java +++ b/spring-boot-project/spring-boot-tools/spring-boot-loader/src/main/java/org/springframework/boot/loader/net/protocol/jar/UrlNestedJarFile.java @@ -56,7 +56,9 @@ class UrlNestedJarFile extends NestedJarFile { @Override public void close() throws IOException { - this.closeAction.accept(this); + if (this.closeAction != null) { + this.closeAction.accept(this); + } super.close(); } diff --git a/spring-boot-project/spring-boot-tools/spring-boot-loader/src/main/java/org/springframework/boot/loader/ref/DefaultCleaner.java b/spring-boot-project/spring-boot-tools/spring-boot-loader/src/main/java/org/springframework/boot/loader/ref/DefaultCleaner.java index e592de5c85..01c6817a38 100644 --- a/spring-boot-project/spring-boot-tools/spring-boot-loader/src/main/java/org/springframework/boot/loader/ref/DefaultCleaner.java +++ b/spring-boot-project/spring-boot-tools/spring-boot-loader/src/main/java/org/springframework/boot/loader/ref/DefaultCleaner.java @@ -17,7 +17,7 @@ package org.springframework.boot.loader.ref; import java.lang.ref.Cleaner.Cleanable; -import java.util.function.Consumer; +import java.util.function.BiConsumer; /** * Default {@link Cleaner} implementation that delegates to {@link java.lang.ref.Cleaner}. @@ -28,15 +28,15 @@ class DefaultCleaner implements Cleaner { static final DefaultCleaner instance = new DefaultCleaner(); - static Consumer tracker; + static BiConsumer tracker; private final java.lang.ref.Cleaner cleaner = java.lang.ref.Cleaner.create(); @Override public Cleanable register(Object obj, Runnable action) { - Cleanable cleanable = this.cleaner.register(obj, action); + Cleanable cleanable = (action != null) ? this.cleaner.register(obj, action) : null; if (tracker != null) { - tracker.accept(cleanable); + tracker.accept(obj, cleanable); } return cleanable; } diff --git a/spring-boot-project/spring-boot-tools/spring-boot-loader/src/test/java/org/springframework/boot/loader/launch/ArchiveTests.java b/spring-boot-project/spring-boot-tools/spring-boot-loader/src/test/java/org/springframework/boot/loader/launch/ArchiveTests.java index 900511176f..77371024b4 100644 --- a/spring-boot-project/spring-boot-tools/spring-boot-loader/src/test/java/org/springframework/boot/loader/launch/ArchiveTests.java +++ b/spring-boot-project/spring-boot-tools/spring-boot-loader/src/test/java/org/springframework/boot/loader/launch/ArchiveTests.java @@ -26,6 +26,7 @@ import org.junit.jupiter.api.io.TempDir; import org.springframework.boot.loader.launch.Archive.Entry; import org.springframework.boot.loader.testsupport.TestJar; +import org.springframework.boot.loader.zip.AssertFileChannelDataBlocksClosed; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatIllegalStateException; @@ -40,6 +41,7 @@ import static org.mockito.Mockito.withSettings; * * @author Phillip Webb */ +@AssertFileChannelDataBlocksClosed class ArchiveTests { @TempDir @@ -75,8 +77,9 @@ class ArchiveTests { CodeSource codeSource = mock(CodeSource.class); given(protectionDomain.getCodeSource()).willReturn(codeSource); given(codeSource.getLocation()).willReturn(jarFile.toURI().toURL()); - Archive archive = Archive.create(protectionDomain); - assertThat(archive).isInstanceOf(JarFileArchive.class); + try (Archive archive = Archive.create(protectionDomain)) { + assertThat(archive).isInstanceOf(JarFileArchive.class); + } } @Test @@ -99,13 +102,17 @@ class ArchiveTests { void createFromFileWhenJarFileReturnsJarFileArchive() throws Exception { File target = new File(this.temp, "missing"); TestJar.create(target); - assertThat(Archive.create(target)).isInstanceOf(JarFileArchive.class); + try (Archive archive = Archive.create(target)) { + assertThat(archive).isInstanceOf(JarFileArchive.class); + } } @Test void createFromFileWhenDirectoryReturnsExplodedFileArchive() throws Exception { File target = this.temp; - assertThat(Archive.create(target)).isInstanceOf(ExplodedArchive.class); + try (Archive archive = Archive.create(target)) { + assertThat(archive).isInstanceOf(ExplodedArchive.class); + } } } diff --git a/spring-boot-project/spring-boot-tools/spring-boot-loader/src/test/java/org/springframework/boot/loader/net/protocol/jar/UrlJarFileFactoryTests.java b/spring-boot-project/spring-boot-tools/spring-boot-loader/src/test/java/org/springframework/boot/loader/net/protocol/jar/UrlJarFileFactoryTests.java index 5f16d8c6cb..69ace5f6d7 100644 --- a/spring-boot-project/spring-boot-tools/spring-boot-loader/src/test/java/org/springframework/boot/loader/net/protocol/jar/UrlJarFileFactoryTests.java +++ b/spring-boot-project/spring-boot-tools/spring-boot-loader/src/test/java/org/springframework/boot/loader/net/protocol/jar/UrlJarFileFactoryTests.java @@ -34,6 +34,7 @@ import org.mockito.MockitoAnnotations; import org.springframework.boot.loader.net.protocol.Handlers; import org.springframework.boot.loader.testsupport.TestJar; +import org.springframework.boot.loader.zip.AssertFileChannelDataBlocksClosed; import static org.assertj.core.api.Assertions.assertThat; @@ -42,6 +43,7 @@ import static org.assertj.core.api.Assertions.assertThat; * * @author Phillip Webb */ +@AssertFileChannelDataBlocksClosed class UrlJarFileFactoryTests { @TempDir diff --git a/spring-boot-project/spring-boot-tools/spring-boot-loader/src/test/java/org/springframework/boot/loader/ref/DefaultCleanerTracking.java b/spring-boot-project/spring-boot-tools/spring-boot-loader/src/test/java/org/springframework/boot/loader/ref/DefaultCleanerTracking.java index 8049acd7d0..c2342c7a54 100644 --- a/spring-boot-project/spring-boot-tools/spring-boot-loader/src/test/java/org/springframework/boot/loader/ref/DefaultCleanerTracking.java +++ b/spring-boot-project/spring-boot-tools/spring-boot-loader/src/test/java/org/springframework/boot/loader/ref/DefaultCleanerTracking.java @@ -17,7 +17,7 @@ package org.springframework.boot.loader.ref; import java.lang.ref.Cleaner.Cleanable; -import java.util.function.Consumer; +import java.util.function.BiConsumer; /** * Utility that allows tests to set a tracker on {@link DefaultCleaner}. @@ -29,7 +29,7 @@ public final class DefaultCleanerTracking { private DefaultCleanerTracking() { } - public static void set(Consumer tracker) { + public static void set(BiConsumer tracker) { DefaultCleaner.tracker = tracker; } diff --git a/spring-boot-project/spring-boot-tools/spring-boot-loader/src/test/java/org/springframework/boot/loader/zip/AssertFileChannelDataBlocksClosedExtension.java b/spring-boot-project/spring-boot-tools/spring-boot-loader/src/test/java/org/springframework/boot/loader/zip/AssertFileChannelDataBlocksClosedExtension.java index caa8d23e21..997e0e02c5 100644 --- a/spring-boot-project/spring-boot-tools/spring-boot-loader/src/test/java/org/springframework/boot/loader/zip/AssertFileChannelDataBlocksClosedExtension.java +++ b/spring-boot-project/spring-boot-tools/spring-boot-loader/src/test/java/org/springframework/boot/loader/zip/AssertFileChannelDataBlocksClosedExtension.java @@ -16,6 +16,8 @@ package org.springframework.boot.loader.zip; +import java.io.Closeable; +import java.io.IOException; import java.lang.ref.Cleaner.Cleanable; import java.nio.channels.FileChannel; import java.nio.file.Path; @@ -57,7 +59,9 @@ class AssertFileChannelDataBlocksClosedExtension implements BeforeEachCallback, private final Set paths = new LinkedHashSet<>(); - private final List cleanup = new ArrayList<>(); + private final List clean = new ArrayList<>(); + + private final List close = new ArrayList<>(); @Override public void openedFileChannel(Path path, FileChannel fileChannel) { @@ -71,16 +75,24 @@ class AssertFileChannelDataBlocksClosedExtension implements BeforeEachCallback, void clear() { this.paths.clear(); - this.cleanup.clear(); + this.clean.clear(); } - void assertAllClosed() { - this.cleanup.forEach(Cleanable::clean); + void assertAllClosed() throws IOException { + for (Closeable closeable : this.close) { + closeable.close(); + } + this.clean.forEach(Cleanable::clean); assertThat(this.paths).as("open paths").isEmpty(); } - private void addedCleanable(Cleanable cleanable) { - this.cleanup.add(cleanable); + private void addedCleanable(Object obj, Cleanable cleanable) { + if (cleanable != null) { + this.clean.add(cleanable); + } + if (obj instanceof Closeable closeable) { + this.close.add(closeable); + } } }