kn: checksum bindings #121
Conversation
|
| /** | ||
| * A mixin class used to ensure CRT is initialized before the class is invoked | ||
| */ | ||
| public open class WithCrt { | ||
| init { | ||
| CRT.initRuntime { } | ||
| } | ||
| } |
There was a problem hiding this comment.
Question: I'm concerned with this approach for two reasons:
- Kotlin doesn't support true mixin classes so this pattern prevents inheritors from using any other superclass. Given that this functionality isn't truly polymorphic it seems prudent to leave the option open.
- This initializes the CRT runtime at object instantiation rather than when CRT functionality is actually necessary. If some code just instantiates a new
Crc32but never uses any methods on it, we'll've wasted cycles unnecessarily.
Could we move the initialization closer to the site of use?
There was a problem hiding this comment.
Could we move the initialization closer to the site of use?
Sure, we'll just have to take care to initialize CRT everywhere we use it, which we currently don't do.
There was a problem hiding this comment.
If some code just instantiates a new Crc32 but never uses any methods on it, we'll've wasted cycles unnecessarily.
This is true but in practice CRT is only initialized once, subsequent calls to CRT.initRuntime { } are no-ops. I'm assuming users of aws-sdk-kotlin Kotlin/Native will need to initialize CRT at some point
There was a problem hiding this comment.
We spoke offline and agreed to continue this approach for now. Ideally we would create a singleton object CrtLib and use cinterop to generate extensions for it (like CrtLib.aws_checksums_crc32(...)). This CrtLib object would ensure CRT.initRuntime { ... } is called before invoking the actual C function.
| if (input.isEmpty() || length == 0) { | ||
| return | ||
| } | ||
|
|
There was a problem hiding this comment.
Question: Why is this necessary? What breaks if we just pass an empty blob or a 0 length down to CRT?
There was a problem hiding this comment.
The request would never make it to CRT, the following code snippet fails with an ArrayIndexOutOfBoundsException exception:
val offsetInput = input.usePinned {
it.addressOf(offset)
}
Description of changes:
WithCrtmixin class used to ensureCRT.initRuntime { }is calledIndexOutOfBoundsbug caused by passing empty inputs to hash functionsBy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.