FakeFilesystem.checkNoOpenFiles() has better information

Including a stacktrace captured when the file was opened should help to diagnose what fix is necessary.
sneha1702 · Dec 28, 2020 · 8a43813 · 8a43813
1 parent 4715ea0
commit 8a43813
Show file tree

Hide file tree

Showing 4 changed files with 113 additions and 20 deletions.
diff --git a/okio-testing/src/commonMain/kotlin/okio/FakeFilesystem.kt b/okio-testing/src/commonMain/kotlin/okio/FakeFilesystem.kt
@@ -26,8 +26,7 @@ import okio.Path.Companion.toPath
  * better tests.
  *
  * Use [openPaths] to see which paths have been opened for read or write, but not yet closed. Tests
- * should assert that this list is empty in `tearDown()`. This way the test only pass if all streams
- * that were opened are also closed.
+ * should call [checkNoOpenFiles] in `tearDown()` to confirm that no file streams were leaked.
  *
  * By default this filesystem permits deletion and removal of open files. Configure
  * [windowsLimitations] to true to throw an [IOException] when asked to delete or rename an open
@@ -49,7 +48,8 @@ class FakeFilesystem(
   /** Keys are canonical paths. Each value is either a [Directory] or a [ByteString]. */
   private val elements = mutableMapOf<Path, Element>()
 
-  private val openPathsMutable = mutableListOf<Path>()
+  /** Files that are currently open and need to be closed to avoid resource leaks. */
+  private val openFiles = mutableListOf<OpenFile>()
 
   /**
    * Canonical paths for every file and directory in this filesystem. This omits filesystem roots
@@ -73,7 +73,31 @@ class FakeFilesystem(
    * while it is still open.
    */
   val openPaths: List<Path>
-    get() = openPathsMutable.toList()
+    get() = openFiles.map { it.canonicalPath }
+
+  /**
+   * Confirm that all files that have been opened on this filesystem (with [source], [sink], and
+   * [appendingSink]) have since been closed. Call this in your test's `tearDown()` function to
+   * confirm that your program hasn't leaked any open files.
+   *
+   * Forgetting to close a file on a real filesystem is a severe error that may lead to a program
+   * crash. The operating system enforces a limit on how many files may be open simultaneously. On
+   * Linux this is [getrlimit] and is commonly adjusted with the `ulimit` command.
+   *
+   * [getrlimit]: https://man7.org/linux/man-pages/man2/getrlimit.2.html
+   *
+   * @throws IllegalStateException if any files are open when this function is called.
+   */
+  fun checkNoOpenFiles() {
+    val firstOpenFile = openFiles.firstOrNull() ?: return
+    throw IllegalStateException(
+      """
+      |expected 0 open files, but found:
+      |    ${openFiles.joinToString(separator = "\n    ") { it.canonicalPath.toString() }}
+      """.trimMargin(),
+      firstOpenFile.backtrace
+    )
+  }
 
   override fun canonicalize(path: Path): Path {
     val canonicalPath = workingDirectory / path
@@ -114,9 +138,10 @@ class FakeFilesystem(
       throw IOException("not a file: $file")
     }
 
-    openPathsMutable += canonicalPath
+    val openFile = OpenFile(canonicalPath, Exception("file opened for reading here"))
+    openFiles += openFile
     element.access(now = clock.now())
-    return FakeFileSource(canonicalPath, Buffer().write(element.data))
+    return FakeFileSource(openFile, Buffer().write(element.data))
   }
 
   override fun sink(file: Path): Sink {
@@ -138,9 +163,10 @@ class FakeFilesystem(
     val parent = requireDirectory(canonicalPath.parent)
     parent.access(now, true)
 
-    openPathsMutable += canonicalPath
+    val openFile = OpenFile(canonicalPath, Exception("file opened for writing here"))
+    openFiles += openFile
     val regularFile = File(createdAt = existing?.createdAt ?: now)
-    val result = FakeFileSink(canonicalPath, regularFile)
+    val result = FakeFileSink(openFile, regularFile)
     if (append && existing is File) {
       result.buffer.write(existing.data)
       regularFile.data = existing.data
@@ -175,11 +201,11 @@ class FakeFilesystem(
     requireDirectory(canonicalTarget.parent)
     if (windowsLimitations) {
       // Windows-only constraints.
-      if (canonicalSource in openPathsMutable) {
-        throw IOException("source is open $source")
+      openFileOrNull(canonicalSource)?.let {
+        throw IOException("source is open $source", it.backtrace)
       }
-      if (canonicalTarget in openPathsMutable) {
-        throw IOException("target is open $target")
+      openFileOrNull(canonicalTarget)?.let {
+        throw IOException("target is open $target", it.backtrace)
       }
     } else {
       // UNIX-only constraints.
@@ -200,8 +226,10 @@ class FakeFilesystem(
       throw IOException("non-empty directory")
     }
 
-    if (windowsLimitations && path in openPathsMutable) {
-      throw IOException("file is open $path")
+    if (windowsLimitations) {
+      openFileOrNull(canonicalPath)?.let {
+        throw IOException("file is open $path", it.backtrace)
+      }
     }
 
     if (elements.remove(canonicalPath) == null) {
@@ -271,9 +299,18 @@ class FakeFilesystem(
     abstract val metadata: FileMetadata
   }
 
+  private fun openFileOrNull(canonicalPath: Path): OpenFile? {
+    return openFiles.firstOrNull { it.canonicalPath == canonicalPath }
+  }
+
+  internal class OpenFile(
+    val canonicalPath: Path,
+    val backtrace: Throwable
+  )
+
   /** Reads data from [buffer], removing itself from [openPathsMutable] when closed. */
   internal inner class FakeFileSource(
-    private val path: Path,
+    private val openFile: OpenFile,
     private val buffer: Buffer
   ) : Source {
     private var closed = false
@@ -288,13 +325,15 @@ class FakeFilesystem(
     override fun close() {
       if (closed) return
       closed = true
-      openPathsMutable -= path
+      openFiles -= openFile
     }
+
+    override fun toString() = "source(${openFile.canonicalPath})"
   }
 
   /** Writes data to [path]. */
   internal inner class FakeFileSink(
-    private val path: Path,
+    private val openFile: OpenFile,
     private val file: File
   ) : Sink {
     val buffer = Buffer()
@@ -318,8 +357,10 @@ class FakeFilesystem(
       closed = true
       file.data = buffer.snapshot()
       file.access(now = clock.now(), modified = true)
-      openPathsMutable -= path
+      openFiles -= openFile
     }
+
+    override fun toString() = "sink(${openFile.canonicalPath})"
   }
 
   override fun toString() = "FakeFilesystem"

diff --git a/okio/src/commonMain/kotlin/okio/-Platform.kt b/okio/src/commonMain/kotlin/okio/-Platform.kt
@@ -25,6 +25,8 @@ expect class ArrayIndexOutOfBoundsException(message: String?) : IndexOutOfBounds
 
 internal expect inline fun <R> synchronized(lock: Any, block: () -> R): R
 
-expect open class IOException(message: String? = null) : Exception
+expect open class IOException(message: String?, cause: Throwable?) : Exception {
+  constructor(message: String? = null)
+}
 
 expect open class EOFException(message: String? = null) : IOException
diff --git a/okio/src/commonTest/kotlin/okio/FakeFilesystemTest.kt b/okio/src/commonTest/kotlin/okio/FakeFilesystemTest.kt
@@ -152,6 +152,51 @@ abstract class FakeFilesystemTest internal constructor(
     assertEquals(accessedAt, metadata.lastAccessedAt)
   }
 
+  @Test
+  fun checkNoOpenFilesThrowsOnOpenSource() {
+    val path = base / "check-no-open-files-open-source"
+    path.writeUtf8("hello, world!")
+    val exception = filesystem.source(path).use { source ->
+      assertFailsWith<IllegalStateException> {
+        fakeFilesystem.checkNoOpenFiles()
+      }
+    }
+
+    assertEquals(
+      """
+      |expected 0 open files, but found:
+      |    $path
+      """.trimMargin(),
+      exception.message
+    )
+    assertEquals("file opened for reading here", exception.cause?.message)
+
+    // Now that the source is closed this is safe.
+    fakeFilesystem.checkNoOpenFiles()
+  }
+
+  @Test
+  fun checkNoOpenFilesThrowsOnOpenSink() {
+    val path = base / "check-no-open-files-open-sink"
+    val exception = filesystem.sink(path).use { source ->
+      assertFailsWith<IllegalStateException> {
+        fakeFilesystem.checkNoOpenFiles()
+      }
+    }
+
+    assertEquals(
+      """
+      |expected 0 open files, but found:
+      |    $path
+      """.trimMargin(),
+      exception.message
+    )
+    assertEquals("file opened for writing here", exception.cause?.message)
+
+    // Now that the source is closed this is safe.
+    fakeFilesystem.checkNoOpenFiles()
+  }
+
   @Test
   fun createDirectoriesForVolumeLetterRoot() {
     val path = "X:\\".toPath()

diff --git a/okio/src/nonJvmMain/kotlin/okio/-Platform.kt b/okio/src/nonJvmMain/kotlin/okio/-Platform.kt
@@ -29,6 +29,11 @@ actual open class ArrayIndexOutOfBoundsException actual constructor(
 
 internal actual inline fun <R> synchronized(lock: Any, block: () -> R): R = block()
 
-actual open class IOException actual constructor(message: String?) : Exception(message)
+actual open class IOException actual constructor(
+  message: String?,
+  cause: Throwable?
+) : Exception(message, cause) {
+  actual constructor(message: String?) : this(message, null)
+}
 
 actual open class EOFException actual constructor(message: String?) : IOException(message)