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

Align file sink and source creation behavior across targets #252

Merged
merged 4 commits into from
Jan 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
32 changes: 29 additions & 3 deletions core/common/test/files/SmokeFileTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,7 @@ class SmokeFileTest {
@Test
fun readNotExistingFile() {
assertFailsWith<FileNotFoundException> {
SystemFileSystem.source(createTempPath()).buffered().use {
it.readByte()
}
SystemFileSystem.source(createTempPath())
}
}

Expand Down Expand Up @@ -372,6 +370,34 @@ class SmokeFileTest {
}
}

@Test
fun createAnEmptyFileUsingSink() {
val path = createTempPath()
assertFalse(SystemFileSystem.exists(path))

SystemFileSystem.sink(path).close()
assertTrue(SystemFileSystem.exists(path))
assertTrue(SystemFileSystem.metadataOrNull(path)!!.isRegularFile)
}

@Test
fun closeFileSinkTwice() {
val path = createTempPath()
val sink = SystemFileSystem.sink(path)
sink.close()
sink.close() // there should be no error
}

@Test
fun closeFileSourceTwice() {
val path = createTempPath()
SystemFileSystem.sink(path).close()
assertTrue(SystemFileSystem.exists(path))
val source = SystemFileSystem.source(path)
source.close()
source.close() // there should be no error
}

private fun constructAbsolutePath(vararg parts: String): String {
return SystemPathSeparator.toString() + parts.joinToString(SystemPathSeparator.toString())
}
Expand Down
53 changes: 38 additions & 15 deletions core/js/src/files/PathsJs.kt
Original file line number Diff line number Diff line change
Expand Up @@ -92,21 +92,31 @@ internal class FileSource(private val path: Path) : RawSource {
private var buffer: dynamic = null
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious, why aren't you using the Buffer type from kotlin-wrappers for the buffer instances?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's a JS buffer returned by the fs.readSync and it's incompatible with kotlinx.io.Buffer.

The way it all works could be improved (by removing that buffer altogether and using an array from a buffer passed to the readAtMostTo call as a destination buffer supplied to fs.readSync).

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it, thanks! Asked because even the require('buffer') at the top of the file is untyped and was wondering why.

private var closed = false
private var offset = 0
private val fd = open(path)

private fun open(path: Path): dynamic {
if (!(fs.existsSync(path.path) as Boolean)) {
throw FileNotFoundException("File does not exist: ${path.path}")
}
val fd = try {
fs.openSync(path.path, "r")
} catch (e: Throwable) {
throw IOException("Failed to open a file ${path.path}.", e)
}
if (fd < 0) throw IOException("Failed to open a file ${path.path}.")
return fd
}

@OptIn(ExperimentalUnsignedTypes::class)
override fun readAtMostTo(sink: Buffer, byteCount: Long): Long {
check(!closed) { "Source is closed." }
if (byteCount == 0L) {
return 0
}
if (buffer === null) {
try {
buffer = fs.readFileSync(path.toString(), null)
buffer = fs.readFileSync(fd, null)
} catch (t: Throwable) {
if (fs.existsSync(path.path) as Boolean) {
throw IOException("Failed to read data from $path", t)
}
throw FileNotFoundException("File does not exist: $path")
throw IOException("Failed to read data from ${path.path}", t)
}
}
val len: Int = buffer.length as Int
Expand All @@ -122,12 +132,27 @@ internal class FileSource(private val path: Path) : RawSource {
}

override fun close() {
closed = true
if (!closed) {
closed = true
fs.closeSync(fd)
}
}
}

internal class FileSink(private val path: Path, private var append: Boolean) : RawSink {
internal class FileSink(path: Path, append: Boolean) : RawSink {
private var closed = false
private val fd = open(path, append)

private fun open(path: Path, append: Boolean): dynamic {
val flags = if (append) "a" else "w"
val fd = try {
fs.openSync(path.path, flags)
} catch (e: Throwable) {
throw IOException("Failed to open a file ${path.path}.", e)
shanshin marked this conversation as resolved.
Show resolved Hide resolved
}
if (fd < 0) throw IOException("Failed to open a file ${path.path}.")
return fd
}

override fun write(source: Buffer, byteCount: Long) {
check(!closed) { "Sink is closed." }
Expand All @@ -142,12 +167,7 @@ internal class FileSink(private val path: Path, private var append: Boolean) : R
val buf = buffer.Buffer.allocUnsafe(segmentBytes)
buf.fill(head.data, head.pos, segmentBytes)
try {
if (append) {
fs.appendFileSync(path.toString(), buf)
} else {
fs.writeFileSync(path.toString(), buf)
append = true
}
fs.writeFileSync(fd, buf)
} catch (e: Throwable) {
throw IOException("Write failed", e)
}
Expand All @@ -160,6 +180,9 @@ internal class FileSink(private val path: Path, private var append: Boolean) : R
override fun flush() = Unit

override fun close() {
closed = true
if (!closed) {
closed = true
fs.closeSync(fd)
}
}
}