Skip to content

Commit

Permalink
Fallback to RandomAccessFile on ClosedByInterruptException
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@philwebb
philwebb committed Apr 16, 2024
1 parent 4203e1f commit 9b0593e
Show file tree
Hide file tree
Showing 5 changed files with 98 additions and 71 deletions.
115 changes: 72 additions & 43 deletions ...s/spring-boot-loader/src/main/java/org/springframework/boot/loader/zip/FileDataBlock.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

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;
Expand All @@ -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;
}
Expand All @@ -79,7 +80,7 @@ public int read(ByteBuffer dst, long pos) throws IOException {
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);
}
Expand All @@ -92,7 +93,7 @@ public int read(ByteBuffer dst, long pos) throws IOException {
* @throws IOException on I/O error
*/
void open() throws IOException {
this.channel.open();
this.fileAccess.open();
}

/**
Expand All @@ -102,7 +103,7 @@ void open() throws IOException {
*/
@Override
public void close() throws IOException {
this.channel.close();
this.fileAccess.close();
}

/**
Expand All @@ -112,7 +113,7 @@ public void close() throws IOException {
* @throws E if the channel is closed
*/
<E extends Exception> void ensureOpen(Supplier<E> exceptionSupplier) throws E {
this.channel.ensureOpen(exceptionSupplier);
this.fileAccess.ensureOpen(exceptionSupplier);
}

/**
Expand Down Expand Up @@ -145,14 +146,14 @@ FileDataBlock slice(long offset, long size) {
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;

Expand All @@ -162,6 +163,10 @@ static class ManagedFileChannel {

private FileChannel fileChannel;

private boolean fileChannelInterrupted;

private RandomAccessFile randomAccessFile;

private ByteBuffer buffer;

private long bufferPosition = -1;
Expand All @@ -170,7 +175,7 @@ static class ManagedFileChannel {

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");
}
Expand All @@ -194,34 +199,45 @@ int read(ByteBuffer dst, long position) throws IOException {
}

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) {
if (Thread.currentThread().isInterrupted()) {
fillBufferUsingRandomAccessFile(position);
return;
}
try {
if (this.fileChannelInterrupted) {
repairFileChannel();
this.fileChannelInterrupted = false;
}
finally {
if (interrupted) {
Thread.currentThread().interrupt();
}
}
this.buffer.clear();
this.bufferSize = this.fileChannel.read(this.buffer, position);
this.bufferPosition = position;
}
catch (ClosedByInterruptException ex) {
this.fileChannelInterrupted = true;
fillBufferUsingRandomAccessFile(position);
}
throw new ClosedByInterruptException();
}

private void repairFileChannel() throws IOException {
if (tracker != null) {
tracker.closedFileChannel(this.path, this.fileChannel);
private void fillBufferUsingRandomAccessFile(long position) throws IOException {
if (this.randomAccessFile == null) {
this.randomAccessFile = new RandomAccessFile(this.path.toFile(), "r");
tracker.openedFileChannel(this.path);
}
this.fileChannel = FileChannel.open(this.path, StandardOpenOption.READ);
if (tracker != null) {
tracker.openedFileChannel(this.path, this.fileChannel);
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 {
tracker.closedFileChannel(this.path);
this.fileChannel = FileChannel.open(this.path, StandardOpenOption.READ);
tracker.openedFileChannel(this.path);
}

void open() throws IOException {
Expand All @@ -230,9 +246,7 @@ void open() throws IOException {
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);
Expand All @@ -251,18 +265,21 @@ void close() throws IOException {
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);
}
}

<E extends Exception> void ensureOpen(Supplier<E> exceptionSupplier) throws E {
synchronized (this.lock) {
if (this.referenceCount == 0 || !this.fileChannel.isOpen()) {
if (this.referenceCount == 0) {
throw exceptionSupplier.get();
}
}
Expand All @@ -280,9 +297,21 @@ public String toString() {
*/
interface Tracker {

void openedFileChannel(Path path, FileChannel fileChannel);
Tracker NONE = new Tracker() {

@Override
public void openedFileChannel(Path path) {
}

@Override
public void closedFileChannel(Path path) {
}

};

void openedFileChannel(Path path);

void closedFileChannel(Path path, FileChannel fileChannel);
void closedFileChannel(Path path);

}

Expand Down
2 changes: 1 addition & 1 deletion ...ing-boot-loader/src/test/java/org/springframework/boot/loader/jar/NestedJarFileTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -303,7 +303,7 @@ void cleanupFromReleasesResources() throws IOException {
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);
Expand Down
7 changes: 3 additions & 4 deletions .../java/org/springframework/boot/loader/zip/AssertFileChannelDataBlocksClosedExtension.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
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;
Expand Down Expand Up @@ -52,7 +51,7 @@ public void beforeEach(ExtensionContext context) throws Exception {
@Override
public void afterEach(ExtensionContext context) throws Exception {
tracker.assertAllClosed();
FileDataBlock.tracker = null;
FileDataBlock.tracker = Tracker.NONE;
}

private static final class OpenFilesTracker implements Tracker {
Expand All @@ -64,12 +63,12 @@ private static final class OpenFilesTracker implements Tracker {
private final List<Closeable> 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);
}

Expand Down
6 changes: 3 additions & 3 deletions ...test/java/org/springframework/boot/loader/zip/FileChannelDataBlockManagedFileChannel.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
*/
Expand All @@ -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;

}
Loading

0 comments on commit 9b0593e

Please sign in to comment.