diff --git a/jimfs/src/main/java/com/google/common/jimfs/RegularFile.java b/jimfs/src/main/java/com/google/common/jimfs/RegularFile.java index b8bb6888..20c9f4b8 100644 --- a/jimfs/src/main/java/com/google/common/jimfs/RegularFile.java +++ b/jimfs/src/main/java/com/google/common/jimfs/RegularFile.java @@ -385,70 +385,66 @@ public long write(long pos, Iterable bufs) throws IOException { /** * Transfers up to {@code count} bytes from the given channel to this file starting at position * {@code pos}. Returns the number of bytes transferred. If {@code pos} is greater than the - * current size of this file, the file is truncated up to size {@code pos} before writing. + * current size of this file, then no bytes are transferred. * * @throws IOException if the file needs more blocks but the disk is full or if reading from src * throws an exception */ - public long transferFrom(ReadableByteChannel src, long pos, long count) throws IOException { - prepareForWrite(pos, 0); // don't assume the full count bytes will be written - - if (count == 0) { + public long transferFrom(ReadableByteChannel src, long startPos, long count) throws IOException { + if (count == 0 + // Unlike the write() methods, attempting to transfer to a position that is greater than the + // current file size simply does nothing. + || startPos > size) { return 0; } long remaining = count; + long currentPos = startPos; - int blockIndex = blockIndex(pos); - byte[] block = blockForWrite(blockIndex); - int off = offsetInBlock(pos); - - ByteBuffer buf = ByteBuffer.wrap(block, off, length(off, remaining)); - - long currentPos = pos; - int read = 0; - while (buf.hasRemaining()) { - read = src.read(buf); - if (read == -1) { - break; - } - - currentPos += read; - remaining -= read; - } - - // update size before trying to get next block in case the disk is out of space - if (currentPos > size) { - size = currentPos; - } + int blockIndex = blockIndex(startPos); + int off = offsetInBlock(startPos); - if (read != -1) { - outer: - while (remaining > 0) { - block = blockForWrite(++blockIndex); + outer: + while (remaining > 0) { + byte[] block = blockForWrite(blockIndex); - buf = ByteBuffer.wrap(block, 0, length(remaining)); - while (buf.hasRemaining()) { - read = src.read(buf); - if (read == -1) { - break outer; + ByteBuffer buf = ByteBuffer.wrap(block, off, length(off, remaining)); + while (buf.hasRemaining()) { + int read = src.read(buf); + // Note: we stop if we read 0 bytes from the src; even though the src is not at EOF, the + // spec of transferFrom is to stop immediately when reading from a non-blocking channel that + // has no bytes available rather than continuing until it reaches EOF. This makes sense + // because we'd otherwise just spin attempting to read bytes from the src repeatedly. + if (read < 1) { + if (currentPos >= size && buf.position() == 0) { + // The current position is at or beyond the end of file (prior to transfer start) and + // the current buffer position is 0. This means that a new block must have just been + // allocated to hold any potential transferred bytes, but no bytes were transferred to + // it because the src had no remaining bytes. So we need to de-allocate that block. + // It's possible that it would be preferable to always transfer to a temporary block + // first and then copy that block to a newly allocated block when it's full or src + // doesn't have any further bytes. Then if we hadn't read anything into the temporary + // block, we could simply discard it. But I think this scenario is likely rare enough + // that it's fine to temporarily allocate a block that _might_ not get used. + disk.free(this, 1); } - - currentPos += read; - remaining -= read; + break outer; } - if (currentPos > size) { - size = currentPos; - } + currentPos += read; + remaining -= read; } + + // Current write block is full + blockIndex++; + off = 0; } if (currentPos > size) { size = currentPos; } - return currentPos - pos; + return currentPos - startPos; } /** diff --git a/jimfs/src/test/java/com/google/common/jimfs/RegularFileBlocksTest.java b/jimfs/src/test/java/com/google/common/jimfs/RegularFileBlocksTest.java index 1dc9df22..5febfc56 100644 --- a/jimfs/src/test/java/com/google/common/jimfs/RegularFileBlocksTest.java +++ b/jimfs/src/test/java/com/google/common/jimfs/RegularFileBlocksTest.java @@ -19,6 +19,9 @@ import static com.google.common.truth.Truth.assertThat; import com.google.common.primitives.Bytes; +import java.io.ByteArrayInputStream; +import java.io.IOException; +import java.nio.channels.Channels; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; @@ -32,6 +35,8 @@ @RunWith(JUnit4.class) public class RegularFileBlocksTest { + private static final int BLOCK_SIZE = 2; + private RegularFile file; @Before @@ -40,7 +45,7 @@ public void setUp() { } private static RegularFile createFile() { - return RegularFile.create(-1, new HeapDisk(2, 2, 2)); + return RegularFile.create(-1, new HeapDisk(BLOCK_SIZE, 2, 2)); } @Test @@ -142,4 +147,33 @@ public void testTransferTo() { assertThat(Bytes.asList(file.getBlock(0))).isEqualTo(Bytes.asList(new byte[] {1, 2, 3})); assertThat(file.getBlock(1)).isNull(); } + + @Test + public void testTransferFrom() throws IOException { + // Test that when a transferFrom ends on a block boundary because the input has no further bytes + // and not because count bytes have been transferred, we don't leave an extra empty block + // allocated on the end of the file. + // https://github.com/google/jimfs/issues/163 + byte[] bytes = new byte[BLOCK_SIZE]; + RegularFile file = createFile(); + + long transferred = + file.transferFrom(Channels.newChannel(new ByteArrayInputStream(bytes)), 0, Long.MAX_VALUE); + assertThat(transferred).isEqualTo(bytes.length); + assertThat(file.blockCount()).isEqualTo(1); + } + + @Test + public void testTransferFrom_noBytesNoAllocation() throws IOException { + // Similar to the previous test but ensures that if no bytes are transferred at all, no new + // blocks remain allocated. + // https://github.com/google/jimfs/issues/163 + byte[] bytes = new byte[0]; + RegularFile file = createFile(); + + long transferred = + file.transferFrom(Channels.newChannel(new ByteArrayInputStream(bytes)), 0, Long.MAX_VALUE); + assertThat(transferred).isEqualTo(0); + assertThat(file.blockCount()).isEqualTo(0); + } } diff --git a/jimfs/src/test/java/com/google/common/jimfs/RegularFileTest.java b/jimfs/src/test/java/com/google/common/jimfs/RegularFileTest.java index f581690b..65a47cdd 100644 --- a/jimfs/src/test/java/com/google/common/jimfs/RegularFileTest.java +++ b/jimfs/src/test/java/com/google/common/jimfs/RegularFileTest.java @@ -334,23 +334,31 @@ public void testEmpty_transferFrom_fromStart_countGreaterThanSrcSize() throws IO assertContentEquals("111111", file); } - public void testEmpty_transferFrom_fromBeyondStart_countEqualsSrcSize() throws IOException { + public void testEmpty_transferFrom_positionGreaterThanSize() throws IOException { long transferred = file.transferFrom(new ByteBufferChannel(buffer("111111")), 4, 6); - assertEquals(6, transferred); - assertContentEquals("0000111111", file); + assertEquals(0, transferred); + assertContentEquals(bytes(), file); } - public void testEmpty_transferFrom_fromBeyondStart_countLessThanSrcSize() throws IOException { + public void testEmpty_transferFrom_positionGreaterThanSize_countEqualsSrcSize() + throws IOException { + long transferred = file.transferFrom(new ByteBufferChannel(buffer("111111")), 4, 6); + assertEquals(0, transferred); + assertContentEquals(bytes(), file); + } + + public void testEmpty_transferFrom_positionGreaterThanSize_countLessThanSrcSize() + throws IOException { long transferred = file.transferFrom(new ByteBufferChannel(buffer("111111")), 4, 3); - assertEquals(3, transferred); - assertContentEquals("0000111", file); + assertEquals(0, transferred); + assertContentEquals(bytes(), file); } - public void testEmpty_transferFrom_fromBeyondStart_countGreaterThanSrcSize() + public void testEmpty_transferFrom_positionGreaterThanSize_countGreaterThanSrcSize() throws IOException { long transferred = file.transferFrom(new ByteBufferChannel(buffer("111111")), 4, 12); - assertEquals(6, transferred); - assertContentEquals("0000111111", file); + assertEquals(0, transferred); + assertContentEquals(bytes(), file); } public void testEmpty_transferFrom_fromStart_noBytes_countEqualsSrcSize() throws IOException { @@ -366,18 +374,18 @@ public void testEmpty_transferFrom_fromStart_noBytes_countGreaterThanSrcSize() assertContentEquals(bytes(), file); } - public void testEmpty_transferFrom_fromBeyondStart_noBytes_countEqualsSrcSize() + public void testEmpty_transferFrom_postionGreaterThanSrcSize_noBytes_countEqualsSrcSize() throws IOException { long transferred = file.transferFrom(new ByteBufferChannel(buffer("")), 5, 0); assertEquals(0, transferred); - assertContentEquals(bytes("00000"), file); + assertContentEquals(bytes(), file); } - public void testEmpty_transferFrom_fromBeyondStart_noBytes_countGreaterThanSrcSize() + public void testEmpty_transferFrom_postionGreaterThanSrcSize_noBytes_countGreaterThanSrcSize() throws IOException { long transferred = file.transferFrom(new ByteBufferChannel(buffer("")), 5, 10); assertEquals(0, transferred); - assertContentEquals(bytes("00000"), file); + assertContentEquals(bytes(), file); } public void testEmpty_transferTo() throws IOException { @@ -793,11 +801,11 @@ public void testNonEmpty_transferFrom_toEnd() throws IOException { assertContentEquals("222222111111", file); } - public void testNonEmpty_transferFrom_toPastEnd() throws IOException { + public void testNonEmpty_transferFrom_positionGreaterThanSize() throws IOException { fillContent("222222"); ByteBufferChannel channel = new ByteBufferChannel(buffer("111111")); - assertEquals(6, file.transferFrom(channel, 10, 6)); - assertContentEquals("2222220000111111", file); + assertEquals(0, file.transferFrom(channel, 10, 6)); + assertContentEquals("222222", file); } public void testNonEmpty_transferFrom_hugeOverestimateCount() throws IOException {