RandomAccessDataFile depletes its FilePool if seek fails

[hengyunabc]

No description provided.

[wilkinsona]

Thanks for the PR. Can you please provide so more information about the change that you're proposing and why you believe it to be necessary. A test that reproduces the apparent deadlock would be most welcome.

[hengyunabc]

I tried to write a demo, but failed. I will try my best to explain this case.

I found this problem from a running java process.

According to the thread stack dump, a thread is blocked at FilePool.close().

// org.springframework.boot.loader.data.RandomAccessDataFile.FilePool.close()
public void close() throws IOException {
  this.available.acquireUninterruptibly(this.size);

In FilePool class, the Semaphore size is 4.

private class FilePool {
  private final Semaphore available;

But in the heap dump, in the blocked thread, the state of Semaphore#sync#state is 3. The blocked thread try to acquireUninterruptibly(4), so it blocked.

java.util.concurrent.locks.AbstractQueuedSynchronizer.state

public class Semaphore implements java.io.Serializable {
    private static final long serialVersionUID = -3222578661600680210L;
    /** All mechanics via AbstractQueuedSynchronizer subclass */
    private final Sync sync;

    /**
     * Synchronization implementation for semaphore.  Uses AQS state
     * to represent permits. Subclassed into fair and nonfair
     * versions.
     */
    abstract static class Sync extends AbstractQueuedSynchronizer {

public abstract class AbstractQueuedSynchronizer
    extends AbstractOwnableSynchronizer
    implements java.io.Serializable {
  /**
   * The synchronization state.
   */
  private volatile int state;

Which needs a Semaphore permit do not be released. but there are no other threads blocked at doRead method.

org.springframework.boot.loader.data.RandomAccessDataFile.DataInputStream.doRead(byte[], int, int)

According to RandomAccessDataFile, there is only one place call available.acquireUninterruptibly();.

RandomAccessFile file = this.file;
if (file == null) {
  file = RandomAccessDataFile.this.filePool.acquire();
  file.seek(RandomAccessDataFile.this.offset + this.position);
}
try {
  if (b == null) {
    int rtn = file.read();
    moveOn(rtn == -1 ? 0 : 1);
    return rtn;
  }
  else {
    return (int) moveOn(file.read(b, off, cappedLen));
  }
}
finally {
  if (this.file == null) {
    RandomAccessDataFile.this.filePool.release(file);
  }
}

If file.seek(RandomAccessDataFile.this.offset + this.position); throw Exception, the Semaphore permit will not be released.

0. The logic of the code is not enough strict completed
1. I met the problem twice, I think that the code needs to be improved.

[philwebb]

I wonder if we can just push the seek into the try block to protect against failures:

		public int doRead(byte[] b, int off, int len) throws IOException {
			if (len == 0) {
				return 0;
			}
			int cappedLen = cap(len);
			if (cappedLen <= 0) {
				return -1;
			}
			RandomAccessFile file = this.file;
			try {
				if (this.file == null) {
					file = RandomAccessDataFile.this.filePool.acquire();
					file.seek(RandomAccessDataFile.this.offset + this.position);
				}
				if (b == null) {
					int rtn = file.read();
					moveOn(rtn == -1 ? 0 : 1);
					return rtn;
				}
				else {
					return (int) moveOn(file.read(b, off, cappedLen));
				}
			}
			finally {
				if (this.file == null && file != null) {
					RandomAccessDataFile.this.filePool.release(file);
				}
			}
		}

[wilkinsona]

@hengyunabc Thanks again for the PR and for the analysis of the problem. As suggested by @philwebb above, we decided to fix it in a slightly different way. Please see e11b7af for details.

[hengyunabc]

Surround with try/finally will be better?

// org.springframework.boot.loader.data.RandomAccessDataFile.FilePool.release(RandomAccessFile)
		public void release(RandomAccessFile file) {
			try {
				this.files.add(file);
			} finally {
				this.available.release();
			}
		}