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

New API, FileSystem.close() #1525

Merged
merged 6 commits into from
Sep 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions okio-fakefilesystem/api/okio-fakefilesystem.api
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ public final class okio/fakefilesystem/FakeFileSystem : okio/FileSystem {
public fun atomicMove (Lokio/Path;Lokio/Path;)V
public fun canonicalize (Lokio/Path;)Lokio/Path;
public final fun checkNoOpenFiles ()V
public fun close ()V
public fun createDirectory (Lokio/Path;Z)V
public fun createSymlink (Lokio/Path;Lokio/Path;)V
public fun delete (Lokio/Path;Z)V
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,12 @@ import okio.fakefilesystem.FakeFileSystem.Operation.WRITE
* Programs that do not attempt any of the above operations should work fine on both UNIX and
* Windows systems. Relax these constraints individually or call [emulateWindows] or [emulateUnix];
* to apply the constraints of a particular operating system.
*
* Closeable
* ---------
*
* This file system cannot be used after it is closed. Closing it does not close any of its open
* streams; those must be closed directly.
*/
class FakeFileSystem(
@JvmField
Expand All @@ -71,6 +77,9 @@ class FakeFileSystem(
/** Files that are currently open and need to be closed to avoid resource leaks. */
private val openFiles = mutableListOf<OpenFile>()

/** Forbid all access after [close]. */
private var closed = false

/**
* An absolute path with this file system's current working directory. Relative paths will be
* resolved against this directory when they are used.
Expand Down Expand Up @@ -218,6 +227,7 @@ class FakeFileSystem(

/** Don't throw [FileNotFoundException] if the path doesn't identify a file. */
private fun canonicalizeInternal(path: Path): Path {
check(!closed) { "closed" }
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All other functions call through here

return workingDirectory.resolve(path, normalize = true)
}

Expand Down Expand Up @@ -764,5 +774,9 @@ class FakeFileSystem(
override fun toString() = "FileHandler(${openFile.canonicalPath})"
}

override fun close() {
closed = true
}

override fun toString() = "FakeFileSystem"
}
Original file line number Diff line number Diff line change
Expand Up @@ -24,4 +24,5 @@ class NodeJsFileSystemTest : AbstractFileSystemTest(
allowClobberingEmptyDirectories = Path.DIRECTORY_SEPARATOR == "\\",
allowAtomicMoveFromFileToDirectory = false,
temporaryDirectory = FileSystem.SYSTEM_TEMPORARY_DIRECTORY,
closeBehavior = CloseBehavior.DoesNothing,
)
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ abstract class AbstractFileSystemTest(
val allowClobberingEmptyDirectories: Boolean,
val allowAtomicMoveFromFileToDirectory: Boolean,
val allowRenameWhenTargetIsOpen: Boolean = !windowsLimitations,
val closeBehavior: CloseBehavior,
temporaryDirectory: Path,
) {
val base: Path = temporaryDirectory / "${this::class.simpleName}-${randomToken(16)}"
Expand Down Expand Up @@ -2553,6 +2554,110 @@ abstract class AbstractFileSystemTest(
}
}

@Test
fun readAfterFileSystemClose() {
val path = base / "file"

path.writeUtf8("hello, world!")

when (closeBehavior) {
CloseBehavior.Closes -> {
fileSystem.close()

assertFailsWith<IllegalStateException> {
fileSystem.canonicalize(path)
}
assertFailsWith<IllegalStateException> {
fileSystem.exists(path)
}
assertFailsWith<IllegalStateException> {
fileSystem.metadata(path)
}
assertFailsWith<IllegalStateException> {
fileSystem.openReadOnly(path)
}
assertFailsWith<IllegalStateException> {
fileSystem.source(path)
}
}

CloseBehavior.DoesNothing -> {
fileSystem.close()
fileSystem.canonicalize(path)
fileSystem.exists(path)
fileSystem.metadata(path)
fileSystem.openReadOnly(path).use {
}
fileSystem.source(path).use {
}
}

CloseBehavior.Unsupported -> {
assertFailsWith<UnsupportedOperationException> {
fileSystem.close()
}
}
}
}

@Test
fun writeAfterFileSystemClose() {
val path = base / "file"

when (closeBehavior) {
CloseBehavior.Closes -> {
fileSystem.close()

assertFailsWith<IllegalStateException> {
fileSystem.appendingSink(path)
}
assertFailsWith<IllegalStateException> {
fileSystem.atomicMove(path, base / "file2")
}
assertFailsWith<IllegalStateException> {
fileSystem.createDirectory(base / "directory")
}
assertFailsWith<IllegalStateException> {
fileSystem.delete(path)
}
assertFailsWith<IllegalStateException> {
fileSystem.openReadWrite(path)
}
assertFailsWith<IllegalStateException> {
fileSystem.sink(path)
}
if (supportsSymlink()) {
assertFailsWith<IllegalStateException> {
fileSystem.createSymlink(base / "symlink", base)
}
}
}

CloseBehavior.DoesNothing -> {
fileSystem.close()

fileSystem.appendingSink(path).use {
}
fileSystem.atomicMove(path, base / "file2")
fileSystem.createDirectory(base / "directory")
fileSystem.delete(path)
fileSystem.sink(path).use {
}
fileSystem.openReadWrite(path).use {
}
if (supportsSymlink()) {
fileSystem.createSymlink(base / "symlink", base)
}
}

CloseBehavior.Unsupported -> {
assertFailsWith<UnsupportedOperationException> {
fileSystem.close()
}
}
}
}

protected fun supportsSymlink(): Boolean {
if (fileSystem.isFakeFileSystem) return fileSystem.allowSymlinks
if (windowsLimitations) return false
Expand Down
22 changes: 22 additions & 0 deletions okio-testing-support/src/commonMain/kotlin/okio/CloseBehavior.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
/*
* Copyright (C) 2024 Square, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package okio

enum class CloseBehavior {
Closes,
DoesNothing,
Unsupported,
}
Original file line number Diff line number Diff line change
Expand Up @@ -24,4 +24,5 @@ class WasiFileSystemTest : AbstractFileSystemTest(
allowClobberingEmptyDirectories = Path.DIRECTORY_SEPARATOR == "\\",
allowAtomicMoveFromFileToDirectory = false,
temporaryDirectory = "/tmp".toPath(),
closeBehavior = CloseBehavior.DoesNothing,
)
4 changes: 3 additions & 1 deletion okio/api/okio.api
Original file line number Diff line number Diff line change
Expand Up @@ -448,7 +448,7 @@ public final class okio/FileMetadata {
public fun toString ()Ljava/lang/String;
}

public abstract class okio/FileSystem {
public abstract class okio/FileSystem : java/io/Closeable {
public static final field Companion Lokio/FileSystem$Companion;
public static final field RESOURCES Lokio/FileSystem;
public static final field SYSTEM Lokio/FileSystem;
Expand All @@ -462,6 +462,7 @@ public abstract class okio/FileSystem {
public static synthetic fun appendingSink$default (Lokio/FileSystem;Lokio/Path;ZILjava/lang/Object;)Lokio/Sink;
public abstract fun atomicMove (Lokio/Path;Lokio/Path;)V
public abstract fun canonicalize (Lokio/Path;)Lokio/Path;
public fun close ()V
public fun copy (Lokio/Path;Lokio/Path;)V
public final fun createDirectories (Lokio/Path;)V
public final fun createDirectories (Lokio/Path;Z)V
Expand Down Expand Up @@ -504,6 +505,7 @@ public abstract class okio/ForwardingFileSystem : okio/FileSystem {
public fun appendingSink (Lokio/Path;Z)Lokio/Sink;
public fun atomicMove (Lokio/Path;Lokio/Path;)V
public fun canonicalize (Lokio/Path;)Lokio/Path;
public fun close ()V
public fun createDirectory (Lokio/Path;Z)V
public fun createSymlink (Lokio/Path;Lokio/Path;)V
public final fun delegate ()Lokio/FileSystem;
Expand Down
1 change: 1 addition & 0 deletions okio/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ kotlin {
}

val nonWasmTest by creating {
dependsOn(commonTest)
dependencies {
implementation(libs.kotlin.time)
implementation(projects.okioFakefilesystem)
Expand Down
17 changes: 16 additions & 1 deletion okio/src/commonMain/kotlin/okio/FileSystem.kt
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,20 @@ package okio
* because the `Paths.get()` function automatically uses the default (ie. system) file system.
* In Okio's API paths are just identifiers; you must use a specific `FileSystem` object to do
* I/O with.
*
* Closeable
* ---------
*
* Implementations of this interface may need to be closed to release resources.
*
* It is the file system implementor's responsibility to document whether a file system instance
* must be closed, and what happens to its open streams when the file system is closed. For example,
* the Java NIO FileSystem closes all of its open streams when the file system is closed.
*
* The built-in `FileSystem.SYSTEM` implementation does not need to be closed and closing it has no
* effect.
*/
expect abstract class FileSystem() {
expect abstract class FileSystem() : Closeable {

/**
* Resolves [path] against the current working directory and symlinks in this file system. The
Expand Down Expand Up @@ -376,6 +388,9 @@ expect abstract class FileSystem() {
@Throws(IOException::class)
abstract fun createSymlink(source: Path, target: Path)

@Throws(IOException::class)
override fun close()

companion object {
/**
* Returns a writable temporary directory on [SYSTEM].
Expand Down
9 changes: 9 additions & 0 deletions okio/src/commonMain/kotlin/okio/ForwardingFileSystem.kt
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,10 @@ import kotlin.jvm.JvmName
* **This class forwards only the abstract functions;** non-abstract functions delegate to the
* other functions of this class. If desired, subclasses may override non-abstract functions to
* forward them.
*
* ### Closeable
*
* Closing this file system closes the delegate file system.
*/
abstract class ForwardingFileSystem(
/** [FileSystem] to which this instance is delegating. */
Expand Down Expand Up @@ -238,5 +242,10 @@ abstract class ForwardingFileSystem(
delegate.createSymlink(source, target)
}

@Throws(IOException::class)
override fun close() {
delegate.close()
}

override fun toString() = "${this::class.simpleName}($delegate)"
}
5 changes: 4 additions & 1 deletion okio/src/jsMain/kotlin/okio/FileSystem.kt
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import okio.internal.commonExists
import okio.internal.commonListRecursively
import okio.internal.commonMetadata

actual abstract class FileSystem {
actual abstract class FileSystem : Closeable {
actual abstract fun canonicalize(path: Path): Path

actual fun metadata(path: Path): FileMetadata = commonMetadata(path)
Expand Down Expand Up @@ -84,6 +84,9 @@ actual abstract class FileSystem {

actual abstract fun createSymlink(source: Path, target: Path)

actual override fun close() {
}

actual companion object {
actual val SYSTEM_TEMPORARY_DIRECTORY: Path = tmpdir.toPath()
}
Expand Down
14 changes: 13 additions & 1 deletion okio/src/jvmMain/kotlin/okio/FileSystem.kt
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import okio.internal.commonExists
import okio.internal.commonListRecursively
import okio.internal.commonMetadata

actual abstract class FileSystem {
actual abstract class FileSystem : Closeable {
@Throws(IOException::class)
actual abstract fun canonicalize(path: Path): Path

Expand Down Expand Up @@ -125,6 +125,10 @@ actual abstract class FileSystem {
@Throws(IOException::class)
actual abstract fun createSymlink(source: Path, target: Path)

@Throws(IOException::class)
actual override fun close() {
}

actual companion object {
/**
* The current process's host file system. Use this instance directly, or dependency inject a
Expand All @@ -150,13 +154,21 @@ actual abstract class FileSystem {
* In applications that compose multiple class loaders, this holds only the resources of
* whichever class loader includes Okio classes. Use [ClassLoader.asResourceFileSystem] for the
* resources of a specific class loader.
*
* This file system does not need to be closed. Calling its close function does nothing.
*/
@JvmField
val RESOURCES: FileSystem = ResourceFileSystem(
classLoader = ResourceFileSystem::class.java.classLoader,
indexEagerly = false,
)

/**
* Closing the returned file system will close the underlying [java.nio.file.FileSystem].
*
* Note that the [default file system][java.nio.file.FileSystems.getDefault] is not closeable
* and calling its close function will throw an [UnsupportedOperationException].
*/
@JvmName("get")
@JvmStatic
fun JavaNioFileSystem.asOkioFileSystem(): FileSystem = NioFileSystemWrappingFileSystem(this)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -187,5 +187,9 @@ internal class NioFileSystemWrappingFileSystem(private val nioFileSystem: NioFil
source.resolve().createSymbolicLinkPointingTo(target.resolve())
}

override fun close() {
nioFileSystem.close()
}

override fun toString() = nioFileSystem::class.simpleName!!
}
3 changes: 3 additions & 0 deletions okio/src/jvmTest/kotlin/okio/FileHandleFileSystemTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ class FileHandleFileSystemTest : AbstractFileSystemTest(
allowClobberingEmptyDirectories = Path.DIRECTORY_SEPARATOR == "\\",
allowAtomicMoveFromFileToDirectory = false,
temporaryDirectory = FileSystem.SYSTEM_TEMPORARY_DIRECTORY,
closeBehavior = CloseBehavior.DoesNothing,
) {
/**
* A testing-only file system that implements all reading and writing operations with
Expand Down Expand Up @@ -75,6 +76,7 @@ class FileHandleNioJimFileSystemWrapperFileSystemTest : AbstractFileSystemTest(
allowClobberingEmptyDirectories = true,
allowAtomicMoveFromFileToDirectory = true,
temporaryDirectory = FileSystem.SYSTEM_TEMPORARY_DIRECTORY,
closeBehavior = CloseBehavior.Closes,
)

class FileHandleNioDefaultFileSystemWrapperFileSystemTest : AbstractFileSystemTest(
Expand All @@ -87,4 +89,5 @@ class FileHandleNioDefaultFileSystemWrapperFileSystemTest : AbstractFileSystemTe
allowAtomicMoveFromFileToDirectory = false,
allowRenameWhenTargetIsOpen = Path.DIRECTORY_SEPARATOR != "\\",
temporaryDirectory = FileSystem.SYSTEM_TEMPORARY_DIRECTORY,
closeBehavior = CloseBehavior.Unsupported,
)
Loading