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

Add NSInputStream.source() and BufferedSource.inputStream() functions for Apple's NSInputStream #1123

Open
wants to merge 28 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
e0548f2
NSInputStream.source() and BufferedSource.inputStream(): NSInputStream
jeffdgr8 Jun 21, 2022
5578fd8
Set streamError and return -1 on read failure
jeffdgr8 Jun 21, 2022
b95de57
Implement NSInputStream.getBuffer()
jeffdgr8 Jun 21, 2022
ddb66c3
convert() native types
jeffdgr8 Jun 21, 2022
a6c3e16
Test NSInputStream.close()
jeffdgr8 Jun 21, 2022
7a8c292
Additional test assertions
jeffdgr8 Jun 22, 2022
b8d1ba9
appleTest TestUtil
jeffdgr8 Jun 22, 2022
c319152
Add doc comments
jeffdgr8 Jun 22, 2022
506fa4f
Resolve checks
jeffdgr8 Jun 22, 2022
aa0b6d3
Avoid data copy & read source to buffer
jeffdgr8 Jun 25, 2022
9ac4e4d
Resolve checks
jeffdgr8 Jun 27, 2022
7e9cbfa
Merge branch 'master' into nsinputstream
jeffdgr8 Jul 22, 2022
72efc44
Override open() as no-op
jeffdgr8 Jul 22, 2022
0f74ac7
Rename variable to avoid ambiguity
jeffdgr8 Jul 22, 2022
3191c97
Move private functions in class
jeffdgr8 Jul 22, 2022
031a3a1
Code review feedback
jeffdgr8 Jul 25, 2022
3571012
Replace additional RealBufferedSource.commonExhausted() logic
jeffdgr8 Jul 25, 2022
41c38a5
Remove variable
jeffdgr8 Jul 25, 2022
c59ea79
Keep buffer pinned for getBuffer() caller
jeffdgr8 Jul 25, 2022
fb883f9
null pinnedBuffer after unpin()
jeffdgr8 Jul 25, 2022
e554a72
Merge branch 'master' into nsinputstream
jeffdgr8 Jul 27, 2022
f917c2e
Merge remote-tracking branch 'upstream/master' into nsinputstream
jeffdgr8 Mar 1, 2023
a40e1c7
Fix lint errors
jeffdgr8 Mar 1, 2023
97192bb
Merge remote-tracking branch 'upstream/master' into nsinputstream
jeffdgr8 Aug 9, 2023
bbddc69
Add NSOutputStream extensions
jeffdgr8 Aug 10, 2023
8b3fe7e
Fix from https://github.com/Kotlin/kotlinx-io/issues/215
jeffdgr8 Aug 24, 2023
f5d63c0
Remove comment
jeffdgr8 Aug 24, 2023
4b7600f
Merge branch 'master' into nsinputstream
jeffdgr8 Oct 1, 2024
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
125 changes: 125 additions & 0 deletions okio/src/appleMain/kotlin/okio/BufferedSource.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,125 @@
/*
* Copyright (C) 2020 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

import kotlinx.cinterop.CPointer
import kotlinx.cinterop.CPointerVar
import kotlinx.cinterop.UnsafeNumber
import kotlinx.cinterop.addressOf
import kotlinx.cinterop.convert
import kotlinx.cinterop.pointed
import kotlinx.cinterop.reinterpret
import kotlinx.cinterop.usePinned
import kotlinx.cinterop.value
import platform.Foundation.NSData
import platform.Foundation.NSError
import platform.Foundation.NSInputStream
import platform.Foundation.NSLocalizedDescriptionKey
import platform.Foundation.NSUnderlyingErrorKey
import platform.darwin.NSInteger
import platform.darwin.NSUInteger
import platform.darwin.NSUIntegerVar
import platform.posix.memcpy
import platform.posix.uint8_tVar

fun BufferedSource.inputStream(): NSInputStream = BufferedSourceInputStream(this)

/** Returns an input stream that reads from this source. */
@OptIn(UnsafeNumber::class)
private class BufferedSourceInputStream(
private val bufferedSource: BufferedSource
) : NSInputStream(NSData()) {

private var error: NSError? = null

override fun streamError(): NSError? = error

override fun open() {
// no-op
}

override fun read(buffer: CPointer<uint8_tVar>?, maxLength: NSUInteger): NSInteger {
try {
val internalBuffer = bufferedSource.buffer

if (bufferedSource is RealBufferedSource) {
if (bufferedSource.closed) throw IOException("closed")

if (internalBuffer.size == 0L) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please rewrite this if clause as

if (bufferedSource.exhausted()) return 0

That will guarantee the buffer has at least one byte in it after!

Copy link
Author

Choose a reason for hiding this comment

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

Oh, nice. I see RealBufferedSource.commonExhausted() is this exact logic. I went ahead and replaced this same logic in other places as well 3571012. Let me know if you'd rather have that reverted though.

val count = bufferedSource.source.read(internalBuffer, Segment.SIZE.toLong())
if (count == -1L) return 0
}
}

val toRead = minOf(maxLength.toInt(), internalBuffer.size).toInt()
return internalBuffer.readNative(buffer, toRead).convert()
} catch (e: Exception) {
error = e.toNSError()
return -1
}
}

override fun getBuffer(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don’t think this is a big deal for now, but eventually it might be a problem .. . .

Using usePinned to get the address of a byte within a buffer is fine, but I worry that the caller might need usePinned when they later read from the buffer.

Copy link
Collaborator

@swankjesse swankjesse Jul 25, 2022

Choose a reason for hiding this comment

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

(This is only an issue if Kotlin’s GC ever learns to relocate objects)

Copy link
Author

Choose a reason for hiding this comment

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

I wondered if this could be problematic, but couldn't think of a way to pass the unpinning to the caller. I figure typical usage would read the buffer immediately after calling the function and wouldn't hold the reference longer. Still a minute chance GC occurs right between unpinning and reading though, and even tinier chance the address becomes invalid.

Copying the buffer would nullify the usefulness of the API and violate the documented behavior of returning in 0(1). I figured having it implemented is probably preferable to not, but alternatively we could just return false.

I wonder what the behavior would be to pin() the address without unpin()ing it. Would GC still collect it once the reference is released, just disallow relocating it? Or would this create a memory leak?

val pinned = s.data.pin()
buffer?.pointed?.value = pinned.addressOf(s.pos).reinterpret()
length?.pointed?.value = (s.limit - s.pos).convert()
return true

Copy link
Collaborator

Choose a reason for hiding this comment

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

The memory leak question is also a good one. At the moment this API doesn't tell the GC about this reference to the ByteArray, so it could be collected (or recycled) before the caller gets to use it.

Maybe we should pin it here, and/or take other steps to avoid recycling, then unpin either on the next call to this method or on close(). That would work and it wouldn't leak.

Copy link
Author

Choose a reason for hiding this comment

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

Great idea! I'll make that change.

buffer: CPointer<CPointerVar<uint8_tVar>>?,
length: CPointer<NSUIntegerVar>?
): Boolean {
if (bufferedSource.buffer.size > 0) {
bufferedSource.buffer.head?.let { s ->
s.data.usePinned {
buffer?.pointed?.value = it.addressOf(s.pos).reinterpret()
length?.pointed?.value = (s.limit - s.pos).convert()
return true
}
}
}
return false
}

override fun hasBytesAvailable(): Boolean = bufferedSource.buffer.size > 0

override fun close() = bufferedSource.close()

override fun description(): String = "$bufferedSource.inputStream()"

private fun Exception.toNSError(): NSError {
return NSError(
"Kotlin",
0,
mapOf(
NSLocalizedDescriptionKey to message,
NSUnderlyingErrorKey to this
)
)
}

private fun Buffer.readNative(sink: CPointer<uint8_tVar>?, maxLength: Int): Int {
val s = head ?: return 0
val toCopy = minOf(maxLength, s.limit - s.pos)
s.data.usePinned {
memcpy(sink, it.addressOf(s.pos), toCopy.convert())
}

s.pos += toCopy
size -= toCopy.toLong()

if (s.pos == s.limit) {
head = s.pop()
SegmentPool.recycle(s)
}

return toCopy
}
}
66 changes: 66 additions & 0 deletions okio/src/appleMain/kotlin/okio/Source.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
/*
* Copyright (C) 2020 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

import kotlinx.cinterop.UnsafeNumber
import kotlinx.cinterop.addressOf
import kotlinx.cinterop.convert
import kotlinx.cinterop.reinterpret
import kotlinx.cinterop.usePinned
import platform.Foundation.NSInputStream
import platform.darwin.UInt8Var

/** Returns a source that reads from `in`. */
fun NSInputStream.source(): Source = NSInputStreamSource(this)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this file should be called source.kt (lowercase name) . . . we’ve been doing lowerCamelFileNames for files with free functions, UpperCamelFileNames when the file matches the primary declaration class.

Copy link
Author

Choose a reason for hiding this comment

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

Should BufferedSource.kt and ByteString.kt, also in this directory, be renamed bufferedSource.ktand byteString.kt as well? Or perhaps BufferedSource.kt should be inputStream.kt?

Copy link
Author

Choose a reason for hiding this comment

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

@swankjesse after merging the latest upstream changes, the linter doesn't like the lowercase filename source.kt, wanting PascalCase. I renamed this file NSInputStreamSource.kt. Let me know if there's a preferable name.


@OptIn(UnsafeNumber::class)
private open class NSInputStreamSource(
private val input: NSInputStream
) : Source {

init {
input.open()
}

override fun read(sink: Buffer, byteCount: Long): Long {
if (byteCount == 0L) return 0L
require(byteCount >= 0L) { "byteCount < 0: $byteCount" }
val tail = sink.writableSegment(1)
val maxToCopy = minOf(byteCount, Segment.SIZE - tail.limit)
val bytesRead = tail.data.usePinned {
val bytes = it.addressOf(tail.limit).reinterpret<UInt8Var>()
input.read(bytes, maxToCopy.convert()).toLong()
}
if (bytesRead < 0) throw IOException(input.streamError?.localizedDescription)
if (bytesRead == 0L) {
if (tail.pos == tail.limit) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice

// We allocated a tail segment, but didn't end up needing it. Recycle!
sink.head = tail.pop()
SegmentPool.recycle(tail)
}
return -1
}
tail.limit += bytesRead.toInt()
sink.size += bytesRead
return bytesRead.convert()
}

override fun close() = input.close()

override fun timeout() = Timeout.NONE

override fun toString() = "source($input)"
}
115 changes: 115 additions & 0 deletions okio/src/appleTest/kotlin/okio/AppleBufferedSourceTest.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
/*
* Copyright (C) 2020 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

import kotlinx.cinterop.CPointerVar
import kotlinx.cinterop.UnsafeNumber
import kotlinx.cinterop.addressOf
import kotlinx.cinterop.alloc
import kotlinx.cinterop.convert
import kotlinx.cinterop.get
import kotlinx.cinterop.memScoped
import kotlinx.cinterop.ptr
import kotlinx.cinterop.reinterpret
import kotlinx.cinterop.usePinned
import kotlinx.cinterop.value
import platform.Foundation.NSInputStream
import platform.darwin.NSUIntegerVar
import platform.darwin.UInt8Var
import kotlin.test.Test
import kotlin.test.assertEquals
import kotlin.test.assertFalse
import kotlin.test.assertNotNull
import kotlin.test.assertTrue

@OptIn(UnsafeNumber::class)
class AppleBufferedSourceTest {
@Test fun bufferInputStream() {
val source = Buffer()
source.writeUtf8("abc")
testInputStream(source.inputStream())
}

@Test fun realBufferedSourceInputStream() {
val source = Buffer()
source.writeUtf8("abc")
testInputStream(RealBufferedSource(source).inputStream())
}

private fun testInputStream(nsis: NSInputStream) {
nsis.open()
val byteArray = ByteArray(4)
byteArray.usePinned {
val cPtr = it.addressOf(0).reinterpret<UInt8Var>()

byteArray.fill(-5)
assertEquals(3, nsis.read(cPtr, 4))
assertEquals("[97, 98, 99, -5]", byteArray.contentToString())

byteArray.fill(-7)
assertEquals(0, nsis.read(cPtr, 4))
assertEquals("[-7, -7, -7, -7]", byteArray.contentToString())
}
}

@Test fun nsInputStreamGetBuffer() {
val source = Buffer()
source.writeUtf8("abc")

val nsis = source.inputStream()
nsis.open()
assertTrue(nsis.hasBytesAvailable)

memScoped {
val bufferPtr = alloc<CPointerVar<UInt8Var>>()
val lengthPtr = alloc<NSUIntegerVar>()
assertTrue(nsis.getBuffer(bufferPtr.ptr, lengthPtr.ptr))

val length = lengthPtr.value
assertNotNull(length)
assertEquals(3.convert(), length)

val buffer = bufferPtr.value
assertNotNull(buffer)
assertEquals('a'.code.toUByte(), buffer[0])
assertEquals('b'.code.toUByte(), buffer[1])
assertEquals('c'.code.toUByte(), buffer[2])
}
}

@Test fun nsInputStreamClose() {
val buffer = Buffer()
buffer.writeUtf8("abc")
val source = RealBufferedSource(buffer)
assertFalse(source.closed)

val nsis = source.inputStream()
nsis.open()
nsis.close()
assertTrue(source.closed)

val byteArray = ByteArray(4)
byteArray.usePinned {
val cPtr = it.addressOf(0).reinterpret<UInt8Var>()

byteArray.fill(-5)
assertEquals(-1, nsis.read(cPtr, 4))
assertNotNull(nsis.streamError)
assertEquals("closed", nsis.streamError?.localizedDescription)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice

assertEquals("[-5, -5, -5, -5]", byteArray.contentToString())
}
}
}
2 changes: 2 additions & 0 deletions okio/src/appleTest/kotlin/okio/AppleByteStringTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,15 @@
*/
package okio

import kotlinx.cinterop.UnsafeNumber
import platform.Foundation.NSData
import platform.Foundation.NSString
import platform.Foundation.NSUTF8StringEncoding
import platform.Foundation.dataUsingEncoding
import kotlin.test.Test
import kotlin.test.assertEquals

@OptIn(UnsafeNumber::class)
class AppleByteStringTest {
@Test fun nsDataToByteString() {
val data = ("Hello" as NSString).dataUsingEncoding(NSUTF8StringEncoding) as NSData
Expand Down
Loading