From 9b0593efe35ffd5de057324400c2dd7f447b1b24 Mon Sep 17 00:00:00 2001 From: Phillip Webb Date: Tue, 16 Apr 2024 14:29:32 -0700 Subject: [PATCH] Fallback to RandomAccessFile on ClosedByInterruptException Refine the fix for gh-38611 so that `ClosedByInterruptException` no longer retries in a loop. Our previous fix was flawed due to the fact that another interrupt could occur after we clear the first and whilst we are reading data. If this happens 10 times in a row, we raise an exception and end up causing NoClassDefFoundError errors. Our new approach retains the use of `FileChannel` and a direct buffer up to the point that a `ClosedByInterruptException` is raised or the thread is detected as interrupted. At that point, we temporarily switch to using a `RandomAccessFile` to access the data. This will block the thread until the data has been read. Fixes gh-40096 --- .../boot/loader/zip/FileDataBlock.java | 119 +++++++++++------- .../boot/loader/jar/NestedJarFileTests.java | 2 +- ...tFileChannelDataBlocksClosedExtension.java | 7 +- ...ileChannelDataBlockManagedFileChannel.java | 6 +- .../boot/loader/zip/FileDataBlockTests.java | 39 +++--- 5 files changed, 100 insertions(+), 73 deletions(-) diff --git a/spring-boot-project/spring-boot-tools/spring-boot-loader/src/main/java/org/springframework/boot/loader/zip/FileDataBlock.java b/spring-boot-project/spring-boot-tools/spring-boot-loader/src/main/java/org/springframework/boot/loader/zip/FileDataBlock.java index e94784a561..cd0a1da432 100644 --- a/spring-boot-project/spring-boot-tools/spring-boot-loader/src/main/java/org/springframework/boot/loader/zip/FileDataBlock.java +++ b/spring-boot-project/spring-boot-tools/spring-boot-loader/src/main/java/org/springframework/boot/loader/zip/FileDataBlock.java @@ -18,6 +18,7 @@ package org.springframework.boot.loader.zip; import java.io.File; import java.io.IOException; +import java.io.RandomAccessFile; import java.nio.ByteBuffer; import java.nio.channels.ClosedByInterruptException; import java.nio.channels.ClosedChannelException; @@ -39,22 +40,22 @@ class FileDataBlock implements CloseableDataBlock { private static final DebugLogger debug = DebugLogger.get(FileDataBlock.class); - static Tracker tracker; + static Tracker tracker = Tracker.NONE; - private final ManagedFileChannel channel; + private final FileAccess fileAccess; private final long offset; private final long size; FileDataBlock(Path path) throws IOException { - this.channel = new ManagedFileChannel(path); + this.fileAccess = new FileAccess(path); this.offset = 0; this.size = Files.size(path); } - FileDataBlock(ManagedFileChannel channel, long offset, long size) { - this.channel = channel; + FileDataBlock(FileAccess fileAccess, long offset, long size) { + this.fileAccess = fileAccess; this.offset = offset; this.size = size; } @@ -79,7 +80,7 @@ class FileDataBlock implements CloseableDataBlock { originalDestinationLimit = dst.limit(); dst.limit(dst.position() + remaining); } - int result = this.channel.read(dst, this.offset + pos); + int result = this.fileAccess.read(dst, this.offset + pos); if (originalDestinationLimit != -1) { dst.limit(originalDestinationLimit); } @@ -92,7 +93,7 @@ class FileDataBlock implements CloseableDataBlock { * @throws IOException on I/O error */ void open() throws IOException { - this.channel.open(); + this.fileAccess.open(); } /** @@ -102,7 +103,7 @@ class FileDataBlock implements CloseableDataBlock { */ @Override public void close() throws IOException { - this.channel.close(); + this.fileAccess.close(); } /** @@ -112,7 +113,7 @@ class FileDataBlock implements CloseableDataBlock { * @throws E if the channel is closed */ void ensureOpen(Supplier exceptionSupplier) throws E { - this.channel.ensureOpen(exceptionSupplier); + this.fileAccess.ensureOpen(exceptionSupplier); } /** @@ -145,14 +146,14 @@ class FileDataBlock implements CloseableDataBlock { if (size < 0 || offset + size > this.size) { throw new IllegalArgumentException("Size must not be negative and must be within bounds"); } - debug.log("Slicing %s at %s with size %s", this.channel, offset, size); - return new FileDataBlock(this.channel, this.offset + offset, size); + debug.log("Slicing %s at %s with size %s", this.fileAccess, offset, size); + return new FileDataBlock(this.fileAccess, this.offset + offset, size); } /** * Manages access to underlying {@link FileChannel}. */ - static class ManagedFileChannel { + static class FileAccess { static final int BUFFER_SIZE = 1024 * 10; @@ -162,6 +163,10 @@ class FileDataBlock implements CloseableDataBlock { private FileChannel fileChannel; + private boolean fileChannelInterrupted; + + private RandomAccessFile randomAccessFile; + private ByteBuffer buffer; private long bufferPosition = -1; @@ -170,7 +175,7 @@ class FileDataBlock implements CloseableDataBlock { private final Object lock = new Object(); - ManagedFileChannel(Path path) { + FileAccess(Path path) { if (!Files.isRegularFile(path)) { throw new IllegalArgumentException(path + " must be a regular file"); } @@ -194,34 +199,45 @@ class FileDataBlock implements CloseableDataBlock { } private void fillBuffer(long position) throws IOException { - for (int i = 0; i < 10; i++) { - boolean interrupted = (i != 0) ? Thread.interrupted() : false; - try { - this.buffer.clear(); - this.bufferSize = this.fileChannel.read(this.buffer, position); - this.bufferPosition = position; - return; - } - catch (ClosedByInterruptException ex) { - repairFileChannel(); - } - finally { - if (interrupted) { - Thread.currentThread().interrupt(); - } - } + if (Thread.currentThread().isInterrupted()) { + fillBufferUsingRandomAccessFile(position); + return; } - throw new ClosedByInterruptException(); + try { + if (this.fileChannelInterrupted) { + repairFileChannel(); + this.fileChannelInterrupted = false; + } + this.buffer.clear(); + this.bufferSize = this.fileChannel.read(this.buffer, position); + this.bufferPosition = position; + } + catch (ClosedByInterruptException ex) { + this.fileChannelInterrupted = true; + fillBufferUsingRandomAccessFile(position); + } + } + + private void fillBufferUsingRandomAccessFile(long position) throws IOException { + if (this.randomAccessFile == null) { + this.randomAccessFile = new RandomAccessFile(this.path.toFile(), "r"); + tracker.openedFileChannel(this.path); + } + byte[] bytes = new byte[BUFFER_SIZE]; + this.randomAccessFile.seek(position); + int len = this.randomAccessFile.read(bytes); + this.buffer.clear(); + if (len > 0) { + this.buffer.put(bytes, 0, len); + } + this.bufferSize = len; + this.bufferPosition = position; } private void repairFileChannel() throws IOException { - if (tracker != null) { - tracker.closedFileChannel(this.path, this.fileChannel); - } + tracker.closedFileChannel(this.path); this.fileChannel = FileChannel.open(this.path, StandardOpenOption.READ); - if (tracker != null) { - tracker.openedFileChannel(this.path, this.fileChannel); - } + tracker.openedFileChannel(this.path); } void open() throws IOException { @@ -230,9 +246,7 @@ class FileDataBlock implements CloseableDataBlock { debug.log("Opening '%s'", this.path); this.fileChannel = FileChannel.open(this.path, StandardOpenOption.READ); this.buffer = ByteBuffer.allocateDirect(BUFFER_SIZE); - if (tracker != null) { - tracker.openedFileChannel(this.path, this.fileChannel); - } + tracker.openedFileChannel(this.path); } this.referenceCount++; debug.log("Reference count for '%s' incremented to %s", this.path, this.referenceCount); @@ -251,10 +265,13 @@ class FileDataBlock implements CloseableDataBlock { this.bufferPosition = -1; this.bufferSize = 0; this.fileChannel.close(); - if (tracker != null) { - tracker.closedFileChannel(this.path, this.fileChannel); - } + tracker.closedFileChannel(this.path); this.fileChannel = null; + if (this.randomAccessFile != null) { + this.randomAccessFile.close(); + tracker.closedFileChannel(this.path); + this.randomAccessFile = null; + } } debug.log("Reference count for '%s' decremented to %s", this.path, this.referenceCount); } @@ -262,7 +279,7 @@ class FileDataBlock implements CloseableDataBlock { void ensureOpen(Supplier exceptionSupplier) throws E { synchronized (this.lock) { - if (this.referenceCount == 0 || !this.fileChannel.isOpen()) { + if (this.referenceCount == 0) { throw exceptionSupplier.get(); } } @@ -280,9 +297,21 @@ class FileDataBlock implements CloseableDataBlock { */ interface Tracker { - void openedFileChannel(Path path, FileChannel fileChannel); + Tracker NONE = new Tracker() { - void closedFileChannel(Path path, FileChannel fileChannel); + @Override + public void openedFileChannel(Path path) { + } + + @Override + public void closedFileChannel(Path path) { + } + + }; + + void openedFileChannel(Path path); + + void closedFileChannel(Path path); } diff --git a/spring-boot-project/spring-boot-tools/spring-boot-loader/src/test/java/org/springframework/boot/loader/jar/NestedJarFileTests.java b/spring-boot-project/spring-boot-tools/spring-boot-loader/src/test/java/org/springframework/boot/loader/jar/NestedJarFileTests.java index 40a59c7baa..8dfc3e564c 100644 --- a/spring-boot-project/spring-boot-tools/spring-boot-loader/src/test/java/org/springframework/boot/loader/jar/NestedJarFileTests.java +++ b/spring-boot-project/spring-boot-tools/spring-boot-loader/src/test/java/org/springframework/boot/loader/jar/NestedJarFileTests.java @@ -303,7 +303,7 @@ class NestedJarFileTests { Cleanable cleanable = mock(Cleanable.class); given(cleaner.register(any(), action.capture())).willReturn(cleanable); try (NestedJarFile jar = new NestedJarFile(this.file, null, null, false, cleaner)) { - Object channel = Extractors.byName("resources.zipContent.data.channel").apply(jar); + Object channel = Extractors.byName("resources.zipContent.data.fileAccess").apply(jar); assertThat(channel).extracting("referenceCount").isEqualTo(1); action.getValue().run(); assertThat(channel).extracting("referenceCount").isEqualTo(0); 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 bb6b536681..d302dfcee0 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 @@ -19,7 +19,6 @@ 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; import java.util.ArrayList; import java.util.LinkedHashSet; @@ -52,7 +51,7 @@ class AssertFileChannelDataBlocksClosedExtension implements BeforeEachCallback, @Override public void afterEach(ExtensionContext context) throws Exception { tracker.assertAllClosed(); - FileDataBlock.tracker = null; + FileDataBlock.tracker = Tracker.NONE; } private static final class OpenFilesTracker implements Tracker { @@ -64,12 +63,12 @@ class AssertFileChannelDataBlocksClosedExtension implements BeforeEachCallback, private final List close = new ArrayList<>(); @Override - public void openedFileChannel(Path path, FileChannel fileChannel) { + public void openedFileChannel(Path path) { this.paths.add(path); } @Override - public void closedFileChannel(Path path, FileChannel fileChannel) { + public void closedFileChannel(Path path) { this.paths.remove(path); } diff --git a/spring-boot-project/spring-boot-tools/spring-boot-loader/src/test/java/org/springframework/boot/loader/zip/FileChannelDataBlockManagedFileChannel.java b/spring-boot-project/spring-boot-tools/spring-boot-loader/src/test/java/org/springframework/boot/loader/zip/FileChannelDataBlockManagedFileChannel.java index 00c1f480df..293383b9ac 100644 --- a/spring-boot-project/spring-boot-tools/spring-boot-loader/src/test/java/org/springframework/boot/loader/zip/FileChannelDataBlockManagedFileChannel.java +++ b/spring-boot-project/spring-boot-tools/spring-boot-loader/src/test/java/org/springframework/boot/loader/zip/FileChannelDataBlockManagedFileChannel.java @@ -16,10 +16,10 @@ package org.springframework.boot.loader.zip; -import org.springframework.boot.loader.zip.FileDataBlock.ManagedFileChannel; +import org.springframework.boot.loader.zip.FileDataBlock.FileAccess; /** - * Test access to {@link ManagedFileChannel} details. + * Test access to {@link FileAccess} details. * * @author Phillip Webb */ @@ -28,6 +28,6 @@ public final class FileChannelDataBlockManagedFileChannel { private FileChannelDataBlockManagedFileChannel() { } - public static int BUFFER_SIZE = FileDataBlock.ManagedFileChannel.BUFFER_SIZE; + public static int BUFFER_SIZE = FileDataBlock.FileAccess.BUFFER_SIZE; } diff --git a/spring-boot-project/spring-boot-tools/spring-boot-loader/src/test/java/org/springframework/boot/loader/zip/FileDataBlockTests.java b/spring-boot-project/spring-boot-tools/spring-boot-loader/src/test/java/org/springframework/boot/loader/zip/FileDataBlockTests.java index ac27a7c43a..09569a461f 100644 --- a/spring-boot-project/spring-boot-tools/spring-boot-loader/src/test/java/org/springframework/boot/loader/zip/FileDataBlockTests.java +++ b/spring-boot-project/spring-boot-tools/spring-boot-loader/src/test/java/org/springframework/boot/loader/zip/FileDataBlockTests.java @@ -19,7 +19,6 @@ package org.springframework.boot.loader.zip; import java.io.File; import java.io.IOException; import java.nio.ByteBuffer; -import java.nio.channels.FileChannel; import java.nio.file.Files; import java.nio.file.Path; @@ -55,7 +54,7 @@ class FileDataBlockTests { @AfterEach void resetTracker() { - FileDataBlock.tracker = null; + FileDataBlock.tracker = Tracker.NONE; } @Test @@ -160,25 +159,25 @@ class FileDataBlockTests { TestTracker tracker = new TestTracker(); FileDataBlock.tracker = tracker; FileDataBlock block = createBlock(); - assertThat(block).extracting("channel.referenceCount").isEqualTo(0); + assertThat(block).extracting("fileAccess.referenceCount").isEqualTo(0); tracker.assertOpenCloseCounts(0, 0); block.open(); - assertThat(block).extracting("channel.referenceCount").isEqualTo(1); + assertThat(block).extracting("fileAccess.referenceCount").isEqualTo(1); tracker.assertOpenCloseCounts(1, 0); block.open(); - assertThat(block).extracting("channel.referenceCount").isEqualTo(2); + assertThat(block).extracting("fileAccess.referenceCount").isEqualTo(2); tracker.assertOpenCloseCounts(1, 0); block.close(); - assertThat(block).extracting("channel.referenceCount").isEqualTo(1); + assertThat(block).extracting("fileAccess.referenceCount").isEqualTo(1); tracker.assertOpenCloseCounts(1, 0); block.close(); - assertThat(block).extracting("channel.referenceCount").isEqualTo(0); + assertThat(block).extracting("fileAccess.referenceCount").isEqualTo(0); tracker.assertOpenCloseCounts(1, 1); block.open(); - assertThat(block).extracting("channel.referenceCount").isEqualTo(1); + assertThat(block).extracting("fileAccess.referenceCount").isEqualTo(1); tracker.assertOpenCloseCounts(2, 1); block.close(); - assertThat(block).extracting("channel.referenceCount").isEqualTo(0); + assertThat(block).extracting("fileAccess.referenceCount").isEqualTo(0); tracker.assertOpenCloseCounts(2, 2); } @@ -188,31 +187,31 @@ class FileDataBlockTests { FileDataBlock.tracker = tracker; FileDataBlock block = createBlock(); FileDataBlock slice = block.slice(1, 4); - assertThat(block).extracting("channel.referenceCount").isEqualTo(0); + assertThat(block).extracting("fileAccess.referenceCount").isEqualTo(0); tracker.assertOpenCloseCounts(0, 0); block.open(); - assertThat(block).extracting("channel.referenceCount").isEqualTo(1); + assertThat(block).extracting("fileAccess.referenceCount").isEqualTo(1); tracker.assertOpenCloseCounts(1, 0); slice.open(); - assertThat(slice).extracting("channel.referenceCount").isEqualTo(2); + assertThat(slice).extracting("fileAccess.referenceCount").isEqualTo(2); tracker.assertOpenCloseCounts(1, 0); slice.open(); - assertThat(slice).extracting("channel.referenceCount").isEqualTo(3); + assertThat(slice).extracting("fileAccess.referenceCount").isEqualTo(3); tracker.assertOpenCloseCounts(1, 0); slice.close(); - assertThat(slice).extracting("channel.referenceCount").isEqualTo(2); + assertThat(slice).extracting("fileAccess.referenceCount").isEqualTo(2); tracker.assertOpenCloseCounts(1, 0); slice.close(); - assertThat(slice).extracting("channel.referenceCount").isEqualTo(1); + assertThat(slice).extracting("fileAccess.referenceCount").isEqualTo(1); tracker.assertOpenCloseCounts(1, 0); block.close(); - assertThat(block).extracting("channel.referenceCount").isEqualTo(0); + assertThat(block).extracting("fileAccess.referenceCount").isEqualTo(0); tracker.assertOpenCloseCounts(1, 1); slice.open(); - assertThat(slice).extracting("channel.referenceCount").isEqualTo(1); + assertThat(slice).extracting("fileAccess.referenceCount").isEqualTo(1); tracker.assertOpenCloseCounts(2, 1); slice.close(); - assertThat(slice).extracting("channel.referenceCount").isEqualTo(0); + assertThat(slice).extracting("fileAccess.referenceCount").isEqualTo(0); tracker.assertOpenCloseCounts(2, 2); } @@ -233,12 +232,12 @@ class FileDataBlockTests { private int closeCount; @Override - public void openedFileChannel(Path path, FileChannel fileChannel) { + public void openedFileChannel(Path path) { this.openCount++; } @Override - public void closedFileChannel(Path path, FileChannel fileChannel) { + public void closedFileChannel(Path path) { this.closeCount++; }