Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorrect block allocation in transferFrom implementation #163

Closed
fred01 opened this issue Aug 10, 2021 · 3 comments · Fixed by #164
Closed

Incorrect block allocation in transferFrom implementation #163

fred01 opened this issue Aug 10, 2021 · 3 comments · Fixed by #164
Assignees
Labels
fixed P3 type=defect Bug, not working as expected

Comments

@fred01
Copy link

fred01 commented Aug 10, 2021

I have a simple test that creates a file and fills it by the transferFrom method.

    @Test
    fun testBlockSizeFit() {
        val blockSize = 4096L
        val fs = Jimfs.newFileSystem(
            Configuration.unix().toBuilder()
                .setBlockSize(4096)
                .setMaxSize(blockSize*2)
                .build())
        val tmpDir = fs.getPath("/tmp/").also { Files.createDirectory(it) }
        val tmpFile = Files.createTempFile(tmpDir, "file", ".tmp")
        val data = Random.nextBytes(blockSize.toInt())
        assertThat(data.size.toLong()).isEqualTo(blockSize) // ensure data size is correct
        val fileChannel = FileChannel.open(tmpFile, StandardOpenOption.CREATE, StandardOpenOption.WRITE)
        fileChannel.use {
            it.transferFrom(Channels.newChannel(ByteArrayInputStream(data)), 0, Long.MAX_VALUE)
        }
        assertThat(tmpDir.fileStore().usableSpace).isEqualTo(blockSize)
    }

This test fails on the assertion with the following values:

expected:<[4096]L> but was:<[0]L>
Expected :[4096]L
Actual   :[0]L

But the test passes if I pass 4096 instead of Long.MAX_VALUE as the value of limit parameter of the transferFrom method.
It seems, transferFrom doesn't respect the actual size of the source channel and allocates extra block when copying.

@eamonnmcmanus eamonnmcmanus added P3 type=defect Bug, not working as expected labels Aug 10, 2021
copybara-service bot pushed a commit that referenced this issue Aug 11, 2021
1. `transferFrom` was not behaving according to the spec when given a position that was greater than the size of that file. The spec says that nothing is transferred in that case, but Jimfs was using the same behavior as `write` in this case: the file was first zero-filled from the start until the given position and then bytes were transferred starting there. Jimfs now simply returns 0 in this case.
2. When given a non-blocking channel that can return 0 bytes from a call to `read`, `transferFrom` was spinning until the input channel returned -1. The spec is for `transferFrom` to return immediately in that case.
3. When a transfer ended on a block boundary (i.e. after exactly filling one block) but still had bytes remaining based on the given `count`, an extra (empty) block would be left allocated on the end of the file. That block is now correctly de-allocated so it can be used elsewhere. Fixes #163.

Also simplify the implementation of `transferFrom` a bit.

RELNOTES=Fixed several bugs with the behavior of `FileChannel.transferFrom`.
PiperOrigin-RevId: 390185370
@cgdecker cgdecker added the fixed label Aug 11, 2021
@cgdecker
Copy link
Member

Thanks for the report. I caught a few other issues with transferTo while I was fixing it.

@fred01
Copy link
Author

fred01 commented Aug 13, 2021

Wow! Thanks for quick fix, that's awesome!

@cpovirk
Copy link
Member

cpovirk commented Jul 6, 2023

And as of 1.3.0 (now on Maven Central), the fix is actually released.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fixed P3 type=defect Bug, not working as expected
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants