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

Conversation

jeffdgr8
Copy link

@jeffdgr8 jeffdgr8 commented Jun 22, 2022

Modeled after JVM's InputStream.source() and BufferedSource.inputStream(), add extension functions to similarly map to and from Apple's NSInputStream.

@JakeWharton
Copy link
Collaborator

FYI #1131 moves a Darwin-specific extension function onto the actual type's companion so that it becomes visible across the Objective-C bridge. We will want to do that same here, but that can be done in a follow-up.

@jeffdgr8
Copy link
Author

@JakeWharton excellent. I'll take a look at moving to a companion object. Looks like once #1131 is merged, we'll have to duplicate actual BufferedSource in appleMain and nonAppleMain source sets, adding the extension to the appleMain companion?

I'm assuming NSInputStream.source() would be appropriate on Source's companion. There's only a single interface defined in common though. Does it need to be converted to expect/actual across all source sets for this change? And would actuals be in jvm, nonAppleMain, and appleMain?

I have these extensions working in my KMM database bindings with a local maven build and things are working well in API tests, by the way.

Copy link
Collaborator

@swankjesse swankjesse left a comment

Choose a reason for hiding this comment

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

Love this. Thanks for your patience on my slow review.

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.

}
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

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

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.

}
}

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.

@jeffdgr8
Copy link
Author

jeffdgr8 commented Jul 27, 2022

@JakeWharton I took a crack at moving these extensions to the companion objects and testing in an iOS app with a KMM shared library with okio as a dependency. I could see the issue, where top-level extensions in the KMM library are exposed in Swift/ObjC, but okio's extensions are not, as a dependency.

The issue I ran into is that BufferedSource and Source are both interfaces. So their companion objects are not accessible on their @protocols in ObjC/Swift, as ByteString's is, as a class.

I'm not sure if there's an intuitive class these extensions could live in instead. It might make sense to create a utility object with the functions available there, but you still have the issue of needing to access the object in the Kotlin library in order for it to be exposed in ObjC/Swift (but at least this is possible, where the Kt class for top-level extensions isn't possible to access in Kotlin).

Thinking about usability in both Kotlin and ObjC/Swift, I think it could make sense to still have the top-level extension as part of the API, rather than deprecating as you did when moving to the companion. This makes using in Kotlin nicer without needing to wrap in with(Foo) { ... }. But then additionally create a utility object that can expose the extensions in ObjC/Swift as another way to access the API.

There are likely a good number of extensions that suffer from this limitation (e.g. Source.buffer()). It'd be good to have a way to expose all related extensions through a single mechanism, whatever it is. I liked the way that the companion object comes along with a class being used in Kotlin. I'm not sure what might be able to work similarly for interfaces.

Ultimately, seems like it would make sense for Kotlin to respect the gradle dependency configuration when generating the ObjC headers for transitive dependencies. If a dependency is declared as implementation, then its full API shouldn't be exposed. But if it's declared as api, then it would make sense to expose the full API, same as the parent library's. But maybe this should be reserved only for transitiveExport.

Also, what is the @CName annotation you're using do? It sounds interesting, based on its documentation. Playing around with it, I don't see a function with the externName provided though. Does this only affect C/C++ and not ObjC?

@serandel
Copy link

serandel commented Jun 2, 2023

Naive question, is this PR still in mind? I see an approval from last year, but I don't know if it needs more, or it has been discarded. Talking to Jeff, this is the only blocker to have a Kotlin Multiplatform Couchbase Lite library, which would be really awesome.

jeffdgr8 added a commit to jeffdgr8/kotbase that referenced this pull request Jun 3, 2023
jeffdgr8 added a commit to jeffdgr8/kotbase that referenced this pull request Jun 3, 2023
@swankjesse
Copy link
Collaborator

I’d like to backport this once it lands:
Kotlin/kotlinx-io#174

@jeffdgr8
Copy link
Author

I’d like to backport this once it lands: Kotlin/kotlinx-io#174

Great, I'm planning to implement the additions there on this Okio branch as well.

@jeffdgr8
Copy link
Author

I backported the changes from Kotlin/kotlinx-io#174 including support for NSOutputStream sink extensions and NSRunLoop scheduling.

@swankjesse
Copy link
Collaborator

Oh that’s awesome. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants