misc: Kotlin/Native IO implementation #96
Conversation
| private val buf = buffer ?: Allocator.Default.alloc<aws_byte_buf>() | ||
|
|
||
| public val bytes: ByteArray | ||
| get() = buf.buffer!!.readBytes(buf.capacity.toInt()) |
There was a problem hiding this comment.
discussion: This function makes a copy of the underlying bytes, I think it's the best we can do with the available CRT APIs but open to suggestions!
There was a problem hiding this comment.
From this KDoc in the class:
NOTE: Platform implementations should provide direct access to the underlying bytes
Do you think we should just not provide access to the underlying bytes?
aws-crt-kotlin/native/src/aws/sdk/kotlin/crt/io/ClientBootstrapNative.kt
Outdated
Show resolved
Hide resolved
aws-crt-kotlin/native/src/aws/sdk/kotlin/crt/io/TlsContextNative.kt
Outdated
Show resolved
Hide resolved
| private val buf = buffer ?: Allocator.Default.alloc<aws_byte_buf>() | ||
|
|
||
| public val bytes: ByteArray | ||
| get() = buf.buffer!!.readBytes(buf.capacity.toInt()) |
aws-crt-kotlin/native/src/aws/sdk/kotlin/crt/io/MutableBufferNative.kt
Outdated
Show resolved
Hide resolved
aws-crt-kotlin/native/src/aws/sdk/kotlin/crt/io/MutableBufferNative.kt
Outdated
Show resolved
Hide resolved
| */ | ||
| public actual fun of(src: ByteArray): MutableBuffer { | ||
| TODO("Not yet implemented") | ||
| public actual fun of(src: ByteArray): MutableBuffer = src.usePinned { pinnedSrc -> |
There was a problem hiding this comment.
We only appear to use this in tests in smithy-kotlin.
I wonder if we need this or if we can get away with creating an aws_byte_buf without allocating it on the heap and having to free it.
There was a problem hiding this comment.
I think I'll leave the implementation as-is and we can revisit it once we start implementing K/N in smithy-kotlin?
aws-crt-kotlin/native/src/aws/sdk/kotlin/crt/io/TlsContextNative.kt
Outdated
Show resolved
Hide resolved
| private val elg: CPointer<aws_event_loop_group> | ||
|
|
||
| override val ptr: CPointer<aws_event_loop_group> | ||
| get() = elg |
There was a problem hiding this comment.
Question: I've seen this pattern a few times in the new (to me) Kotlin/Native code. What's the benefit of a separate backing field if the ptr always delegates to it anyway?
There was a problem hiding this comment.
I think we're going to fix this soon. This stems from the original definition of CrtResource but I think we're going to just move to something like:
interface NativeHandle<T: CPointed> {
val ptr: CPointer<T>
}or perhaps name ptr to handle or nativeHandle. Either way I think we'd move to where we don't need a separate backing field.
There was a problem hiding this comment.
Yes I agree, the backing field sticks out and seems wrong, I plan to refactor after the memory management / CrtResource changes are shipped
| private val shutdownCompleteChannel = Channel<Unit>(Channel.RENDEZVOUS) | ||
| private val channelStableRef = StableRef.create(shutdownCompleteChannel) |
There was a problem hiding this comment.
Question: I don't understand what this channel is used for. It's a rendezvous channel so either send or receive suspends until the other is called, right? I see a send in onShutdownComplete but no receive call anywhere...won't this just suspend indefinitely?
There was a problem hiding this comment.
I commented elsewhere, it should be waited on in waitForShutdown.
It won't block in onShutdownComplete because it's using trySend.
aws-crt-kotlin/native/src/aws/sdk/kotlin/crt/io/MutableBufferNative.kt
Outdated
Show resolved
Hide resolved
|
3 participants
Issue #, if available:
N/A
Description of changes:
runSuspendTestwith kotlinx-coroutinesrunTestacross the entire repoEventLoopGroup,TlsContext,ClientBootstrap,HostResolver,MutableBufferBy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.