kn: implement `SdkBuffer` #1214

lauzadis · 2025-01-09T20:14:26Z

Issue #

Description of changes

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

…n-buffer

lauzadis · 2025-01-09T21:49:54Z

runtime/runtime-core/common/src/aws/smithy/kotlin/runtime/io/SdkSource.kt

+public suspend fun SdkSource.readToByteArray(): ByteArray = withContext(SdkDispatchers.IO) {
+    use { it.buffer().readByteArray() }
+}


note: the only reason I could see these were expect/actual is because of Dispatchers.IO, but we already have an expect/actual version of that called SdkDispatchers, so I've moved these to common and deleted the JVM/Native implementations

lauzadis · 2025-01-09T21:51:19Z

runtime/runtime-core/native/src/aws/smithy/kotlin/runtime/io/internal/SdkDispatchersNative.kt

 @InternalApi
 public actual object SdkDispatchers {
    /**
     * The CoroutineDispatcher that is designed for offloading blocking IO tasks to a shared pool of threads.
     */
    public actual val IO: CoroutineDispatcher
-        get() = TODO("Not yet implemented")
+        get() = Dispatchers.IO
 }


note: we made this expect/actual because Native didn't have Dispatchers.IO at the time. It does now, so we could remove SdkDispatchers and just use Dispatchers.IO everywhere... but I think we'll need it again in the future when we support platforms like JS and WASM.

Unless JetBrains implements dispatchers on those platforms first! 😁

ianbotsf · 2025-01-09T21:55:31Z

...th/aws-signing-default/common/src/aws/smithy/kotlin/runtime/auth/awssigning/Canonicalizer.kt

 import aws.smithy.kotlin.runtime.time.TimestampFormat
+import kotlinx.coroutines.IO
 import kotlinx.coroutines.withContext


Question: Why was this import added when nothing else changed in this file?

I was working in this file earlier, this import is unused now and I'm surprised ktlint didn't complain

ianbotsf · 2025-01-09T22:00:29Z

runtime/runtime-core/common/src/aws/smithy/kotlin/runtime/io/BuffereredSourceAdapter.kt

+/**
+ * Used to wrap calls to Okio, catching Okio exceptions (e.g. okio.EOFException) and throwing our own (e.g. aws.smithy.kotlin.runtime.io.EOFException).
+ */
+internal inline fun <T> SdkBufferedSource.wrapOkio(block: SdkBufferedSource.() -> T): T = try {
+    block()
+} catch (e: okio.EOFException) {
+    throw EOFException("Okio operation failed", e)
+} catch (e: okio.IOException) {
+    throw IOException("Okio operation failed", e)
+}


Suggestion: Consider adding the Okio exception message to the wrapped exception message:

throw EOFException("Okio operation failed: ${e.message}", e)

Different loggers and exception dumpers may print an entire exception including the cause but some won't and users may only have a top-level exception message to work with in some contexts.

ianbotsf · 2025-01-09T22:12:18Z

runtime/runtime-core/native/src/aws/smithy/kotlin/runtime/io/ExceptionsNative.kt

+public actual open class EOFException actual constructor(message: String?, cause: Throwable?) : IOException(message) {
+    public actual constructor() : this(null, null)
+    public actual constructor(message: String?) : this(message, null)
 }


Correctness: Pass the constructor cause parameter to the superclass constructor:

class EOFException actual constructor(message: String?, cause: Throwable?) : IOException(message, cause)

ianbotsf · 2025-01-09T22:15:05Z

runtime/runtime-core/native/src/aws/smithy/kotlin/runtime/io/SdkBufferNative.kt

-    actual override fun readShortLe(): Short {
-        TODO("Not yet implemented")
+    actual override fun emit() {
+        wrapOkio { inner.emit() }
    }


Question: Same as JVM implementation? Can this be moved to common?

ianbotsf · 2025-01-09T22:17:08Z

runtime/runtime-core/native/src/aws/smithy/kotlin/runtime/io/SdkBufferNative.kt

+    override fun equals(other: Any?): Boolean {
+        if (this === other) return true
+        if (other !is SdkBuffer) return false
+        return inner == other.inner
    }


Question: Same as JVM implementation? Can this be moved to common?

ianbotsf · 2025-01-09T22:18:25Z

runtime/runtime-core/native/src/aws/smithy/kotlin/runtime/io/internal/SdkDispatchersNative.kt

 @InternalApi
 public actual object SdkDispatchers {
    /**
     * The CoroutineDispatcher that is designed for offloading blocking IO tasks to a shared pool of threads.
     */
    public actual val IO: CoroutineDispatcher
-        get() = TODO("Not yet implemented")
+        get() = Dispatchers.IO
 }


Unless JetBrains implements dispatchers on those platforms first! 😁

ianbotsf · 2025-01-09T22:19:06Z

runtime/runtime-core/native/src/aws/smithy/kotlin/runtime/io/SdkBufferNative.kt

@@ -4,157 +4,102 @@
 */
 package aws.smithy.kotlin.runtime.io

-import okio.Buffer
+import aws.smithy.kotlin.runtime.io.internal.*

 public actual class SdkBuffer :


Question: Do we still need platform-specific implementations of SDK buffer? Seems like basically everything is commonized or commonizable at this point.

The only difference I see is JVM has inputStream and outputStream methods. I'll see if this can be made common.

I think at some point we were considering using CRT for Native I/O. Okio is multiplatform so I was thinking we should just use that?

Agreed. Their IO model matches ours better than CRT iirc.

github-actions · 2025-01-10T16:40:52Z

Affected Artifacts

Changed in size

Artifact	Pull Request (bytes)	Latest Release (bytes)	Delta (bytes)	Delta (percentage)
http-test-jvm.jar	61,633	58,718	2,915	4.96%
crt-util-jvm.jar	21,448	20,954	494	2.36%
runtime-core-jvm.jar	824,791	812,469	12,322	1.52%
aws-signing-tests-jvm.jar	456,614	456,568	46	0.01%
test-suite-jvm.jar	96,903	97,180	-277	-0.29%

lauzadis added 8 commits January 8, 2025 15:26

Buffer implementation

0f15c3f

Buffer implementation

c8f0c65

Enable more tests

9d1ab9a

Remove okio.EOFException

be417dc

Unnecessary qualification

51f39b5

Throw the correct exception type

d8df632

Wrap some functions I missed

53deb9f

ktlint

f7550a4

lauzadis added the no-changelog Indicates that a changelog entry isn't required for a pull request. Use sparingly. label Jan 9, 2025

Merge branch 'kn-main' of github.com:smithy-lang/smithy-kotlin into k…

a2dd729

…n-buffer

This comment has been minimized.

Sign in to view

apiDump

b58aaa7

This comment has been minimized.

Sign in to view

lauzadis added the acknowledge-api-break Acknowledge that a change is API breaking and may be backwards-incompatible. Review carefully! label Jan 9, 2025

lauzadis mentioned this pull request Jan 9, 2025

misc: remove usage of SdkDispatchers awslabs/aws-sdk-kotlin#1497

Closed

This comment has been minimized.

Sign in to view

lauzadis added 2 commits January 9, 2025 16:04

Restore SdkDispatchers

7a5cbed

ktlint, apiDump

bc91650

This comment has been minimized.

Sign in to view

SdkDispatchers

a992b33

This comment has been minimized.

Sign in to view

lauzadis marked this pull request as ready for review January 9, 2025 21:48

lauzadis requested a review from a team as a code owner January 9, 2025 21:48

lauzadis commented Jan 9, 2025

View reviewed changes

ianbotsf requested changes Jan 9, 2025

View reviewed changes

Remove unused import

2fed4c3

lauzadis added 2 commits January 9, 2025 17:26

Add Okio exception message

52d3c96

Pass cause to super class

fd21fa0

This comment has been minimized.

Sign in to view

ianbotsf approved these changes Jan 10, 2025

View reviewed changes

lauzadis added 2 commits January 10, 2025 11:36

SdkBuffer -> jvmAndNative

0024934

ktlint

4dd47dc

lauzadis merged commit 2653fbf into kn-main Jan 10, 2025
18 checks passed

lauzadis deleted the kn-buffer branch January 10, 2025 18:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kn: implement `SdkBuffer` #1214

kn: implement `SdkBuffer` #1214

lauzadis commented Jan 9, 2025

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

lauzadis Jan 9, 2025

lauzadis Jan 9, 2025 •

edited

Loading

ianbotsf Jan 9, 2025

ianbotsf Jan 9, 2025

lauzadis Jan 9, 2025

ianbotsf Jan 9, 2025

ianbotsf Jan 9, 2025

ianbotsf Jan 9, 2025

ianbotsf Jan 9, 2025

ianbotsf Jan 9, 2025

ianbotsf Jan 9, 2025

lauzadis Jan 9, 2025

ianbotsf Jan 10, 2025

This comment has been minimized.

github-actions bot commented Jan 10, 2025

kn: implement SdkBuffer #1214

kn: implement SdkBuffer #1214

Conversation

lauzadis commented Jan 9, 2025

Issue #

Description of changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

lauzadis Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

github-actions bot commented Jan 10, 2025

Affected Artifacts

kn: implement `SdkBuffer` #1214

kn: implement `SdkBuffer` #1214

lauzadis Jan 9, 2025 •

edited

Loading