Skip to content

Commit

Permalink
Fix scala-native#3333: javalib FileInputStream#available now matches …
Browse files Browse the repository at this point in the history
…its JVM description (scala-native#3338)
  • Loading branch information
LeeTibbert committed Jun 22, 2023
1 parent fd88d5f commit 2179e73
Show file tree
Hide file tree
Showing 4 changed files with 159 additions and 71 deletions.
80 changes: 53 additions & 27 deletions javalib/src/main/scala/java/nio/channels/FileChannelImpl.scala
Original file line number Diff line number Diff line change
Expand Up @@ -415,34 +415,60 @@ private[java] final class FileChannelImpl(
}
}

/* The Scala Native implementation of FileInputStream#available delegates
* to this method. This method now implements "available()" as described in
* the Java description of FileInputStream#available. So the delegator
* now matches the its JDK description and behavior (Issue 3333).
*
* There are a couple of fine points to this implemention which might
* be helpful to know:
* 1) There is no requirement that this method itself not block.
* Indeed, depending upon what, if anything, is in the underlying
* file system cache, this method may do so.
*
* The current position should already be in the underlying OS fd but
* calling "size()" may require reading an inode or equivalent.
*
* 2) Given JVM actual behavior, the "read (or skipped over) from this
* input stream without blocking" clause of the JDK description might
* be better read as "without blocking for additional data bytes".
*
* A "skip()" should be a fast update of existing memory. Conceptually,
* and by JDK definition FileChannel "read()"s may block transferring
* bytes from slow storage to memory. Where is io_uring() when
* you need it?
*
* 3) The value returned is exactly the "estimate" portion of the JDK
* description:
*
* - All bets are off is somebody, even this thread, decreases
* size of the file in the interval between when "available()"
* returns and "read()" is called.
*
* - This method is defined in FileChannel#available as returning
* an Int. This also matches the use above in the Windows
* implementation of the private method
* "read(buffer: Array[Byte], offset: Int, count: Int)"
* Trace the count argument logic.
*
* FileChannel defines "position()" and "size()" as Long values.
* For large files and positions < Integer.MAX_VALUE,
* The Long difference "lastPosition - currentPosition" might well
* be greater than Integer.MAX_VALUE. In that case, the .toInt
* truncation will return the low estimate of Integer.MAX_VALUE
* not the true (Long) value. Matches the specification, but gotcha!
*/

def available(): Int = {
if (isWindows) {
val currentPosition, lastPosition = stackalloc[windows.LargeInteger]()
SetFilePointerEx(
fd.handle,
distanceToMove = 0,
newFilePointer = currentPosition,
moveMethod = FILE_CURRENT
)
SetFilePointerEx(
fd.handle,
distanceToMove = 0,
newFilePointer = lastPosition,
moveMethod = FILE_END
)
SetFilePointerEx(
fd.handle,
distanceToMove = !currentPosition,
newFilePointer = null,
moveMethod = FILE_BEGIN
)
ensureOpen()

(!lastPosition - !currentPosition).toInt
} else {
val currentPosition = unistd.lseek(fd.fd, 0, stdio.SEEK_CUR)
val lastPosition = unistd.lseek(fd.fd, 0, stdio.SEEK_END)
unistd.lseek(fd.fd, currentPosition, stdio.SEEK_SET)
(lastPosition - currentPosition).toInt
}
val currentPosition = position()
val lastPosition = size()

val nAvailable =
if (currentPosition >= lastPosition) 0
else lastPosition - currentPosition

nAvailable.toInt
}
}
Original file line number Diff line number Diff line change
@@ -1,12 +1,15 @@
package org.scalanative.testsuite.javalib.io

import java.io._
import java.nio.file.{Files, Path}

import scala.util.Try

import org.junit.Test
import org.junit.Assert._

import org.scalanative.testsuite.javalib.io.IoTestHelpers.withTemporaryDirectory

import org.scalanative.testsuite.utils.Platform.isWindows
import org.scalanative.testsuite.utils.AssertThrows.assertThrows

Expand Down Expand Up @@ -68,4 +71,27 @@ class FileInputStreamTest {
new FileInputStream("/the/path/does/not/exist/for/sure")
)
}

@Test def available(): Unit = {
withTemporaryDirectory { dir =>
val f = dir.toPath().resolve("FisTestDataForMethodAvailable.utf-8")
val str = "They were the best of us!"
Files.write(f, str.getBytes("UTF-8"))

val fis = new FileInputStream(f.toFile())
try {
// current position less than file size.
assertEquals("available pos < size", str.length(), fis.available())
// 2023-06-15 11:21 -0400 FIXME

// move current position to > than file size.
val channel = fis.getChannel()
channel.position(str.length * 2) // two is an arbitrary value > 1
assertEquals("available pos > size", 0, fis.available())
} finally {
fis.close()
}
}
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -6,50 +6,15 @@ import org.junit.Test
import org.junit.Assert._
import org.junit.Assume._

import org.scalanative.testsuite.javalib.io.IoTestHelpers._

import org.scalanative.testsuite.utils.Platform.isWindows
import org.scalanative.testsuite.utils.AssertThrows.assertThrows

class FileOutputStreamTest {
def withTempFile(f: File => Unit): Unit = {
val tmpfile = File.createTempFile("scala-native-test", null)
try {
f(tmpfile)
} finally {
tmpfile.delete()
}
}

def withTempDirectory(f: File => Unit): Unit = {
import java.nio.file._
import attribute._
val tmpdir = Files.createTempDirectory("scala-native-test")
try {
f(tmpdir.toFile())
} finally {
Files.walkFileTree(
tmpdir,
new SimpleFileVisitor[Path]() {
override def visitFile(
file: Path,
attrs: BasicFileAttributes
): FileVisitResult = {
Files.delete(file)
FileVisitResult.CONTINUE
}
override def postVisitDirectory(
dir: Path,
exc: IOException
): FileVisitResult = {
Files.delete(dir)
FileVisitResult.CONTINUE
}
}
)
}
}

@Test def writeNull(): Unit = {
withTempFile { file =>
withTemporaryFile { file =>
val fos = new FileOutputStream(file)
assertThrows(classOf[NullPointerException], fos.write(null))
assertThrows(classOf[NullPointerException], fos.write(null, 0, 0))
Expand All @@ -58,7 +23,7 @@ class FileOutputStreamTest {
}

@Test def writeOutOfBoundsNegativeCount(): Unit = {
withTempFile { file =>
withTemporaryFile { file =>
val fos = new FileOutputStream(file)
val arr = new Array[Byte](8)
assertThrows(classOf[IndexOutOfBoundsException], fos.write(arr, 0, -1))
Expand All @@ -67,7 +32,7 @@ class FileOutputStreamTest {
}

@Test def writeOutOfBoundsNegativeOffset(): Unit = {
withTempFile { file =>
withTemporaryFile { file =>
val fos = new FileOutputStream(file)
val arr = new Array[Byte](8)
assertThrows(classOf[IndexOutOfBoundsException], fos.write(arr, -1, 0))
Expand All @@ -76,7 +41,7 @@ class FileOutputStreamTest {
}

@Test def writeOutOfBoundsArrayTooSmall(): Unit = {
withTempFile { file =>
withTemporaryFile { file =>
val fos = new FileOutputStream(file)
val arr = new Array[Byte](8)
assertThrows(classOf[IndexOutOfBoundsException], fos.write(arr, 0, 16))
Expand All @@ -86,14 +51,14 @@ class FileOutputStreamTest {
}

@Test def attemptToOpenReadonlyRegularFile(): Unit = {
withTempFile { ro =>
withTemporaryFile { ro =>
ro.setReadOnly()
assertThrows(classOf[FileNotFoundException], new FileOutputStream(ro))
}
}

@Test def attemptToOpenDirectory(): Unit = {
withTempDirectory { dir =>
withTemporaryDirectory { dir =>
assertThrows(classOf[FileNotFoundException], new FileOutputStream(dir))
}
}
Expand All @@ -103,7 +68,7 @@ class FileOutputStreamTest {
"Setting directory read only in Windows does not have affect on creating new files",
isWindows
)
withTempDirectory { ro =>
withTemporaryDirectory { ro =>
ro.setReadOnly()
assertThrows(
classOf[FileNotFoundException],
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
package org.scalanative.testsuite.javalib.io

import java.io.File
import java.io.IOException

/* Note on Technical Debt:
*
* The good news about Technical Debt is that it means that a project has
* endured long enough to accumulate it. The bad news is that there
* is accumulated Technical Debt.
*
* Specifically, the "withTemporaryDirectory" concept and implementations of it
* and close kin, exist in a number of places) in javalib unit-tests.
* They tend to have slight variations, which make both development &
* maintenance both annoying and error prone.
* Someday they should be unified into a "Single Point of Truth".
*
* This file allows the FileInputStream#available test introduced in PR 3333
* to share the "withTemporaryDirectory" previously used in FileOutputStream.
* By doing so, it avoids introducing new Technical Debt. It makes no attempt
* to solve the whole "withTemporaryDirectory" problem.
*
* The impediment to solving the larger problem is probably determining a
* proper place in the directory tree for the common file and gaining
* consensus.
*
* Perhaps inspiration or enlightenment will strike the next time someone
* implements or maintains a unit-test requiring "withTemporaryDirectory".
*/

object IoTestHelpers {
def withTemporaryFile(f: File => Unit): Unit = {
val tmpfile = File.createTempFile("scala-native-test", null)
try {
f(tmpfile)
} finally {
tmpfile.delete()
}
}

// This variant takes "Temporary" to mean: clean up after yourself.
def withTemporaryDirectory(f: File => Unit): Unit = {
import java.nio.file._
import attribute._
val tmpdir = Files.createTempDirectory("scala-native-test")
try {
f(tmpdir.toFile())
} finally {
Files.walkFileTree(
tmpdir,
new SimpleFileVisitor[Path]() {
override def visitFile(
file: Path,
attrs: BasicFileAttributes
): FileVisitResult = {
Files.delete(file)
FileVisitResult.CONTINUE
}
override def postVisitDirectory(
dir: Path,
exc: IOException
): FileVisitResult = {
Files.delete(dir)
FileVisitResult.CONTINUE
}
}
)
}
}

}

0 comments on commit 2179e73

Please sign in to comment.