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

feat(io): implement missing IO runtime primitives #264

Merged
merged 23 commits into from
Apr 6, 2021
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
3 changes: 2 additions & 1 deletion client-runtime/client-rt-core/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ kotlin {
sourceSets {
commonMain {
dependencies {
implementation(project(":client-runtime:io"))
// io types are exposed as part of content/*
api(project(":client-runtime:io"))
// Attributes property bag is exposed as client options
api(project(":client-runtime:utils"))
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,6 @@ package software.aws.clientrt.content
* Container for wrapping a ByteArray as a [ByteStream]
*/
class ByteArrayContent(private val bytes: ByteArray) : ByteStream.Buffer() {
override val contentLength: Long? = bytes.size.toLong()
override val contentLength: Long = bytes.size.toLong()
override fun bytes(): ByteArray = bytes
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,10 @@
*/
package software.aws.clientrt.content

import software.aws.clientrt.io.Source
import software.aws.clientrt.io.SdkByteReadChannel

/**
* Represents an abstract stream of bytes
* Represents an abstract read-only stream of bytes
Copy link
Contributor

Choose a reason for hiding this comment

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

One thing that might be interesting on the comments or docs is - is this a "single read" stream or can I seek up and down / read it multiple times?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That actually depends on the variant. If it's just a ByteArray then of course it's "seekable". If it's a Reader (aka SdkByteReadChannel) then it is not seekable.

*/
sealed class ByteStream {

Expand All @@ -31,9 +31,9 @@ sealed class ByteStream {
*/
abstract class Reader : ByteStream() {
/**
* Provides [Source] to read from/consume
* Provides [SdkByteReadChannel] to read from/consume
*/
abstract fun readFrom(): Source
abstract fun readFrom(): SdkByteReadChannel
}

companion object {
Expand All @@ -52,11 +52,10 @@ sealed class ByteStream {
suspend fun ByteStream.toByteArray(): ByteArray {
return when (val stream = this) {
is ByteStream.Buffer -> stream.bytes()
is ByteStream.Reader -> stream.readFrom().readAll()
is ByteStream.Reader -> stream.readFrom().readRemaining()
}
}

@OptIn(ExperimentalStdlibApi::class)
suspend fun ByteStream.decodeToString(): String = toByteArray().decodeToString()

fun ByteStream.cancel() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ class StringContent(str: String) : ByteStream.Buffer() {
@OptIn(ExperimentalStdlibApi::class)
private val asBytes: ByteArray = str.encodeToByteArray()

override val contentLength: Long? = asBytes.size.toLong()
override val contentLength: Long = asBytes.size.toLong()

override fun bytes(): ByteArray = asBytes
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

question

In my experience InputStream/OutputStream is the most common way of dealing w/ I/O in Java. They are more flexibil than working directly with filetypes as they can be composed, among other features. Is there a reason you don't provide mappings to those in this file?

I realize we don't use these types in our SDK due to concurrency constraints but seems like at this level those concerns are not valid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I realize we don't use these types in our SDK due to concurrency constraints but seems like at this level those concerns are not valid.

I'm not sure I follow what you mean, can you clarify?

I'll double check but I don't recall seeing any utilities for dealing with Input/Output stream. The reason we can for files is asynchronous utilities have been built on top of it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean, this logic is for customers to work with data in or out of the SDK. As such we should consider thier general utility. To rephrase, in my experience in Javaland, java libraries often use Input/Output streams for handling this data. It's likely that customers will want to use JavaLibraryX which has a function doSomethingWith(InputStream: data). Unless there is a simple way of performing this mapping that I've missed, it may be of limited utility.

It's not something that needs to be addressed as part of this PR, and customer feedback is certainly warranted here.

* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
* SPDX-License-Identifier: Apache-2.0.
*/

package software.aws.clientrt.content

import software.aws.clientrt.io.SdkByteReadChannel
import software.aws.clientrt.io.copyTo
import software.aws.clientrt.io.writeChannel
import java.io.File
import java.nio.file.Path

// JVM specific extensions for dealing with ByteStream's

/**
* Create a [ByteStream] from a file
*/
fun ByteStream.Companion.fromFile(file: File): ByteStream = file.asByteStream()
Copy link
Contributor

Choose a reason for hiding this comment

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

extension function on the Companion object is that even a thing? Why have this and the File based extension function below? Is this a discoverability thing?

Copy link
Contributor Author

@aajtodd aajtodd Mar 31, 2021

Choose a reason for hiding this comment

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

extension function on the Companion object is that even a thing?

is that a real question or were you just surprised?

Why have this and the File based extension function below? Is this a discoverability thing?

Consistency. We provide Bytestream.fromXYZ() functions for a few other types.

Copy link
Contributor

Choose a reason for hiding this comment

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

k


/**
* Create a [ByteStream] from a file
*/
fun File.asByteStream(): ByteStream = FileContent(this)

/**
* Create a [ByteStream] from a path
*/
fun Path.asByteStream(): ByteStream {
val f = toFile()
require(f.isFile) { "cannot create a ByteStream from a directory: $this" }
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion

If we're guarding dir/file why not also guard if the file exists? File.exists() I think.

require(f.exists()) { "cannot create ByteStream, invalid file: $this" }
return f.asByteStream()
}

/**
* Write the contents of this ByteStream to file and close it
* @return the number of bytes written
*/
suspend fun ByteStream.writeToFile(file: File): Long {
require(file.isFile) { "cannot write contents of ByteStream to a directory: ${file.absolutePath}" }
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in this case the file doesn't need to exist since we may be creating it

val writer = file.writeChannel()
val src = when (this) {
is ByteStream.Buffer -> SdkByteReadChannel(bytes())
is ByteStream.Reader -> readFrom()
}

try {
return src.copyTo(writer)
} finally {
writer.close()
src.close()
}
}

/**
* Write the contents of this ByteStream to file at the given path
* @return the number of bytes written
*/
suspend fun ByteStream.writeToFile(path: Path): Long = writeToFile(path.toFile())
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
/*
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
* SPDX-License-Identifier: Apache-2.0.
*/

package software.aws.clientrt.content

import software.aws.clientrt.io.SdkByteReadChannel
import software.aws.clientrt.io.readChannel
import java.io.File

/**
* ByteStream backed by a local [file]
*/
public class FileContent(
public val file: File,
) : ByteStream.Reader() {

override val contentLength: Long
get() = file.length()

override fun readFrom(): SdkByteReadChannel = file.readChannel()
}
12 changes: 12 additions & 0 deletions client-runtime/io/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,16 @@ description = "IO primitives for Smithy services generated by smithy-kotlin"
extra["displayName"] = "Smithy :: Kotlin :: IO"
extra["moduleName"] = "software.aws.clientrt.io"

val ktorVersion: String by project
val coroutinesVersion: String by project

kotlin {
sourceSets {
commonMain {
dependencies {
implementation(project(":client-runtime:utils"))
implementation("io.ktor:ktor-io:$ktorVersion")
implementation("org.jetbrains.kotlinx:kotlinx-coroutines-core:$coroutinesVersion")
}
}

Expand All @@ -22,5 +27,12 @@ kotlin {
implementation(project(":client-runtime:testing"))
}
}

jvmMain {
dependencies {
// file channel utils
implementation("io.ktor:ktor-utils:$ktorVersion")
}
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
/*
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
* SPDX-License-Identifier: Apache-2.0.
*/

package software.aws.clientrt.io

import io.ktor.utils.io.bits.*
import io.ktor.utils.io.core.*

@OptIn(ExperimentalIoApi::class)
internal interface Allocator {
Copy link
Contributor

Choose a reason for hiding this comment

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

wtf is all this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so this has to do with ktor-io's Memory type that abstracts away dealing with various platform "memory". You can't instantiate the type directly in common it has to be done per/platform.

This followed in ktor-io's footsteps a bit. On JVM and JS there is no free() function but on native there is. I suspect this will go away though since they are changing the memory model.

fun alloc(size: Int): Memory
// FIXME - we should revisit this - Kotlin/Native is only place where we would actually be manually managing memory
// and that story may change to the point where a free() function isn't even necessary
fun free(instance: Memory)
}

// allocate using the most appropriate underlying platform type (e.g. ByteBuffer on JVM, ArrayBuffer on JS, etc)
internal expect object DefaultAllocator : Allocator

/**
* Round up to the next power of 2. [size] should be non-negative
*/
internal fun ceilp2(size: Int): Int {
require(size >= 0) { "must be positive integer" }
var x = size - 1
x = x or (x shr 1)
x = x or (x shr 2)
x = x or (x shr 4)
x = x or (x shr 8)
x = x or (x shr 16)
return x + 1
}

/**
* Allocate new memory of size [newSize], copy the contents of [instance] into it and free [instance]
* and return the newly allocated memory.
*
* The memory of [instance] should no longer be used after calling.
*/
@OptIn(ExperimentalIoApi::class)
internal fun Allocator.realloc(instance: Memory, newSize: Int): Memory {
require(newSize >= instance.size32)
val newInstance = alloc(newSize)
instance.copyTo(newInstance, 0, instance.size32, 0)
free(instance)
return newInstance
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
/*
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
* SPDX-License-Identifier: Apache-2.0.
*/

package software.aws.clientrt.io

// this really should live in the stdlib...
// https://youtrack.jetbrains.com/issue/KT-31066

public expect interface Closeable {
public fun close()
}

public inline fun <C : Closeable, R> C.use(block: (C) -> R): R {
var closed = false

return try {
block(this)
} catch (first: Throwable) {
try {
closed = true
close()
} catch (second: Throwable) {
first.addSuppressedInternal(second)
}

throw first
} finally {
if (!closed) {
close()
}
}
}

@PublishedApi
internal expect fun Throwable.addSuppressedInternal(other: Throwable)
Copy link
Contributor

Choose a reason for hiding this comment

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

comment

I don't understand what this function is for, maybe some docs?

Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
/*
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
* SPDX-License-Identifier: Apache-2.0.
*/

package software.aws.clientrt.io

import io.ktor.utils.io.*
import io.ktor.utils.io.core.*
import io.ktor.utils.io.ByteChannel as KtorByteChannel
import io.ktor.utils.io.ByteReadChannel as KtorByteReadChannel
import io.ktor.utils.io.ByteWriteChannel as KtorByteWriteChannel

// marker interfaces used internally for accessing the underlying ktor impl
internal interface KtorReadChannel {
val chan: KtorByteReadChannel
}

internal interface KtorWriteChannel {
val chan: KtorByteWriteChannel
}

/**
* Wrap ktor's ByteReadChannel as our own. This implements the common API of [SdkByteReadChannel]. Only
* platform specific differences in interfaces need be implemented in inheritors.
*/
internal abstract class KtorReadChannelAdapterBase(
Copy link
Contributor

Choose a reason for hiding this comment

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

nit/style

i think you can just use the shorter form her and have every property and function be one line...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can but I have found in particular that when wrapping a 3P library type that you have to be very careful or else you end up with downstream issues depending on whether you specify api vs implementation.

e.g.

fun foo() = thirdParty.blah()

You may intend to have this function be Unit, no big deal right. Except blah() actually returns third.party.MyType and you meant to hide that. This bit me several times when trying to figure out why downstream usage was complaining about missing dependencies. I realized that the type inference is making the signature more like:

fun foo(): third.party.MyType

I chose to explicitly use the longer form so that type inference wasn't part of the equation and I could guarantee I wasn't accidentally exposing details in the API. There are also some minor type differences as well that the short form can't be used for all of them. I'll take a second look though since I have a better handle on some of the sharp corners.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah it was just a nit. If the long form provides more safety, that is better.

override val chan: KtorByteReadChannel
) : SdkByteReadChannel, KtorReadChannel {

override val availableForRead: Int
get() = chan.availableForRead

override val isClosedForRead: Boolean
get() = chan.isClosedForRead

override val isClosedForWrite: Boolean
get() = chan.isClosedForWrite

override suspend fun readRemaining(limit: Int): ByteArray {
return chan.readRemaining(limit.toLong()).readBytes()
}

override suspend fun readFully(sink: ByteArray, offset: Int, length: Int) {
chan.readFully(sink, offset, length)
}

override suspend fun readAvailable(sink: ByteArray, offset: Int, length: Int): Int {
return chan.readAvailable(sink, offset, length)
}

override fun cancel(cause: Throwable?): Boolean {
return chan.cancel(cause)
}
}

/**
* Wrap ktor's ByteWriteChannel as our own. This implements the common API of [SdkByteWriteChannel]. Only
* platform specific differences in interfaces need be implemented in inheritors.
*/
internal abstract class KtorWriteChannelAdapterBase(
override val chan: KtorByteWriteChannel
) : SdkByteWriteChannel, KtorWriteChannel {
override val availableForWrite: Int
get() = chan.availableForWrite

override val isClosedForWrite: Boolean
get() = chan.isClosedForWrite

override val totalBytesWritten: Long
get() = chan.totalBytesWritten

override val autoFlush: Boolean
get() = chan.autoFlush

override suspend fun writeFully(src: ByteArray, offset: Int, length: Int) {
chan.writeFully(src, offset, length)
}

override suspend fun writeAvailable(src: ByteArray, offset: Int, length: Int): Int {
return chan.writeAvailable(src, offset, length)
}

override fun close(cause: Throwable?): Boolean {
return chan.close(cause)
}

override fun flush() {
chan.flush()
}
}

/**
* Wrap ktor's ByteChannel as our own
*/
internal class KtorByteChannelAdapter(
override val chan: KtorByteChannel
) : SdkByteChannel,
SdkByteReadChannel by KtorReadChannelAdapter(chan),
SdkByteWriteChannel by KtorWriteChannelAdapter(chan),
KtorWriteChannel,
KtorReadChannel {
override val isClosedForWrite: Boolean
get() = chan.isClosedForWrite

override fun close() { chan.close(null) }
}

internal expect class KtorReadChannelAdapter(chan: KtorByteReadChannel) : SdkByteReadChannel
internal expect class KtorWriteChannelAdapter(chan: KtorByteWriteChannel) : SdkByteWriteChannel

internal fun KtorByteReadChannel.toSdkChannel(): SdkByteReadChannel = KtorReadChannelAdapter(this)
internal fun KtorByteWriteChannel.toSdkChannel(): SdkByteWriteChannel = KtorWriteChannelAdapter(this)
internal fun KtorByteChannel.toSdkChannel(): SdkByteChannel = KtorByteChannelAdapter(this)
Loading