Separate Common and JVM-only code

[luca992]

No description provided.

[ccjernigan]

Thanks so much for the effort that went into this. The changes are clear and a huge step forward to making this project capable of supporting multiplatform targets 💪.

I realize there's a lot of text in my comments, but mostly it's questions that are non-blocking.

I approved the CI job, which failed with two things we'll need to resolve to keep the build green going forward. These should be super quick:

• For the ktlint warnings, those should be quickly fixable with a ./gradlew ktlintFormat
• For the Detekt warnings, I recommend using best judgement on when to fix versus when to suppress. Often that means fix, but sometimes we suppress with a comment explaining why the rule isn't helpful in some specific case. You can run ./gradlew detektAll locally to see what it complains about.

I think it's a good idea to deal with ktlint first, since the tools have some overlap in what they complain about.

On the code changes, I had a few questions:

• Should we make the new classes in the common and crypto packages internal? I think they're implementation details and probably shouldn't be a public API. I recognize that previously FallbackProvider and Pbkdf2Sha512 were public, and I suspect that was by accident when initially implemented. Making those two internal classes would technically be a public API change. Should we change them now? Should we keep them all public for now, then file an issue to make them all consistently internal as a followup?
• I have a non-blocking question inline about performance of SecureRandom.

[ccjernigan]

This is not a blocker for merging your PR, and I would like your input on a forward looking question.

We have a pre-existing issue to improve performance of the library, specifically in that it can trigger a strict mode violation on Android by performing disk IO on the main thread. Constructing SecureRandom is the culprit under the JVM/Android. On native or JS, the slow operation might be at a different point in time.

Because of these potential cross-platform differences, I'd be interested to get your input.

Since we can't have suspend on constructors, I think long-term we either need something like:

// This solves the problem of SecureRandom construction being slow on JVM
actual class SecureRandom private constructor(val secureRandom: java.security.SecureRandom) {
      actual fun nextBytes(bytes: ByteArray) {
         secureRandom.nextBytes(bytes)
     }
    companion object {
        suspend fun new() {
            val jvmSecureRandom: java.security.SecureRandom = withContext(Dispatchers.IO) { java.security.SecureRandom() }
            return secureRandom(jvmSecureRandom)
        }
    }
}

OR

// This solves problem of both SecureRandom construction being slow on JVM, while also
// providing a template for other platforms where getting the next bytes is actually the slow part
actual class SecureRandom private constructor() {
    private val jvmSecureRandom by lazy {java.security.SecureRandom()}
    suspend fun nextBytes() {
       return withContext() {
           jvmSecureRandom.nextBytes()
       }
    }
      actual fun nextBytes(bytes: ByteArray) {
         secureRandom.nextBytes(bytes)
     }
    companion object {
        suspend fun new() {
            val jvmSecureRandom: java.security.SecureRandom = withContext(Dispatchers.IO) { java.security.SecureRandom() }
            return secureRandom(jvmSecureRandom)
        }
    }
}

Again, not something we need to solve to merge your PR. But what are your thoughts on those two approaches? Are there other approaches you might have in mind? What happens if our public API has a suspend function and then needs to be consumed by an iOS target?

[luca992]

On native I'm getting nextBytes with read /dev/urandom which won't block so suspend shouldn't be needed. and BCryptGenRandom on windows (but I can't find info on whether or not that could block)

On JS I'm currently using crypto.getRandomValues(bytes) which isn't a promise. so suspend isn't needed there.
But if you think generateKey needs to be used instead
, that returns a promise. Which means that function would need to be a suspend

and for jvm: it looks like it could technically block not the thread ... or it could https://stackoverflow.com/questions/137212/how-to-deal-with-a-slow-securerandom-generator

If it were me. I wouldn't make it a suspend if it doesn't need to be

[luca992]

Or both suspending and non-suspending versions could be made. Or keep none of them suspending and make a library level suspending initialize function similar to https://github.com/ionspin/kotlin-multiplatform-libsodium

[ccjernigan]

Thanks for the input—let's leave this as-is for now. It is something to keep incubating on as we move forward.

What I've been doing to date is wrapping the usage in a withContext() which works fine for the time being. I like the idea of a suspending public API eventually because it better signals to clients that the call might be slower.

[ccjernigan]

Is this filename right?

[luca992]

yeah the class matches the filename: ReadmeExamplesTestJvm
I moved that test out because use is only available on jvm

[ccjernigan]

I filed a followup issue to use the Kotlin language feature one it becomes available #160

[luca992]

Should we make the new classes in the common and crypto packages internal? I think they're implementation details and probably shouldn't be a public API. I recognize that previously FallbackProvider and Pbkdf2Sha512 were public, and I suspect that was by accident when initially implemented. Making those two internal classes would technically be a public API change. Should we change them now? Should we keep them all public for now, then file an issue to make them all consistently internal as a followup?

I fixed the formatting issues. The probably should be internal. But up to you, I can make them all internal if you want

[ccjernigan]

Should we make the new classes in the common and crypto packages internal? I think they're implementation details and probably shouldn't be a public API. I recognize that previously FallbackProvider and Pbkdf2Sha512 were public, and I suspect that was by accident when initially implemented. Making those two internal classes would technically be a public API change. Should we change them now? Should we keep them all public for now, then file an issue to make them all consistently internal as a followup?

I fixed the formatting issues. The probably should be internal. But up to you, I can make them all internal if you want

Let's go ahead and make them all internal (including the two pre-existing classes of FallbackProvider and Pbkdf2Sha512 that should probably have been internal all along), which I think is a good improvement. After that, I think this will be good to go! The CI job already passed, so that's looking good too.

[luca992]

@ccjernigan all made internal. It wasn't possible to make an actual internal typealias so I changed it to internal actual interface Closeable : java.io.Closeable

[ccjernigan]

@ccjernigan all made internal. It wasn't possible to make an actual internal typealias so I changed it to internal actual interface Closeable : java.io.Closeable

Great, thank you for figuring out a good option for this!