KTOR-6030 Migrate Ktor to kotlinx-io #4032

e5l · 2024-05-15T08:59:30Z

Fix KTOR-6030 Migrate to new kotlinx.io library

bjhham

A glorious amount of deleted code!

Seems that compilation is broken for common code in the CI. I also can't seem to build on my machine, I might need some instruction if there is any config changes or missing gradle configs.

bjhham · 2024-05-21T07:46:35Z

ktor-client/ktor-client-cio/jvm/src/io/ktor/client/engine/cio/ConnectionPipeline.kt

@@ -138,6 +138,7 @@ internal actual class ConnectionPipeline actual constructor(
                if (shouldClose) break
            }
        } finally {
+            @Suppress("DEPRECATION")


Are these close deprecation warnings something we should follow up on? Maybe create a task in YT? The recommendation seems to be using either flushAndClose or cancel.

ktor-client/ktor-client-cio/jvm/src/io/ktor/client/engine/cio/ConnectionPipeline.kt

...or-client-plugins/ktor-client-logging/jvm/test/io/ktor/client/plugins/logging/LoggingTest.kt

bjhham · 2024-05-21T08:28:05Z

ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/HttpTimeoutTest.kt

@@ -214,7 +216,7 @@ class HttpTimeoutTest : ClientLoader() {
                parameter("delay", 500)
            }.body<ByteReadChannel>()

-            assertFailsWith<HttpRequestTimeoutException> {


Maybe we should consider wrapping this once again with the HttpRequestTimeoutException... Since we don't have checked exceptions, this could cause some tough-to-diagnose issues for consumers that rely on the current timeout exceptions.

ktor-client/ktor-client-tests/jvm/src/io/ktor/client/tests/utils/ClientLoaderJvm.kt

bjhham · 2024-05-21T08:52:25Z

ktor-io/common/src/io/ktor/utils/io/ByteChannelUtils.kt

+@Suppress("UnusedReceiverParameter", "UNUSED_PARAMETER")
+public fun ByteChannel.attachJob(job: Job) {
+    job.invokeOnCompletion {
+        if (it != null) {
+            cancel(it)
+        }
+    }
+}
+
+public fun ByteChannel.attachJob(job: ChannelJob) {
+    attachJob(job.job)
+}
+
+public fun ByteChannel(@Suppress("UNUSED_PARAMETER") block: (Throwable?) -> Throwable?): ByteChannel = ByteChannel()


Looks like these functions need some documentation.

thanks, will add

bjhham · 2024-05-21T09:07:10Z

ktor-io/common/src/io/ktor/utils/io/SourceByteReadChannel.kt

+
+internal class SourceByteReadChannel(private val source: Source) : ByteReadChannel {
+
+    val created = Exception()


Another created exception property?

thanks, will fix

bjhham · 2024-05-21T09:23:31Z

ktor-io/common/src/io/ktor/utils/io/core/Memory.kt

+    this[index + 1] = (value shr 16).toByte()
+    this[index + 2] = (value shr 8).toByte()
+    this[index + 3] = value.toByte()
+}


Would be cool if kotlinx.io provided these kinds of random-access extensions for ByteArray too since they're all implemented for Buffer.

as far as I see, it's used only in WebSocket implementation and can be rather easily replaced with operations on kotlinx-io primitives.
IMO, there is no need to have such functions exposed nor in ktor, nor in kotlinx-io, as more high level primitives should be used.
Additionally, there will be something like this for unsafe access described in Kotlin/kotlinx-io#135 (comment)

bjhham · 2024-05-21T09:40:56Z

ktor-io/common/src/io/ktor/utils/io/internal/AwaitingSlot.kt

    }

    /**
     * Resume waiter.
     */
    fun resume() {
-        suspension.getAndSet(null)?.complete()
+        val continuation = suspension.getAndUpdate {
+            if (it == CLOSED) CLOSED else null


Shouldn't this be it as? ClosedSlot so that we include the cause / new instance from line 44 val closeContinuation = if (cause != null) ClosedSlot(cause) else CLOSED.

bjhham · 2024-05-21T10:01:28Z

ktor-utils/jvm/src/io/ktor/util/cio/FileChannels.kt

    val fileLength = length()
-    return CoroutineScope(coroutineContext).writer(CoroutineName("file-reader") + coroutineContext, autoFlush = false) {
+    val randomAccessFile = RandomAccessFile(this@readChannel, "r")
+    val writer = CoroutineScope(coroutineContext).writer(CoroutineName("file-reader") + coroutineContext, autoFlush = false) {
        require(start >= 0L) { "start position shouldn't be negative but it is $start" }
        require(endInclusive <= fileLength - 1) {
            "endInclusive points to the position out of the file: file size = $fileLength, endInclusive = $endInclusive"
        }

        @Suppress("BlockingMethodInNonBlockingContext")
-        RandomAccessFile(this@readChannel, "r").use { file ->
+        randomAccessFile.use { file ->
            val fileChannel: FileChannel = file.channel
            fileChannel.writeToScope(this, start, endInclusive)
        }
-    }.channel
+    }
+
+    writer.invokeOnCompletion {
+        randomAccessFile.close()
+    }
+
+    return writer.channel


Why not read directly with something like:

val source = SystemFileSystem.source(Path(absolutePath)) SourceByteReadChannel(source.buffered())

?

e5l · 2024-05-27T06:15:10Z

Hey @bjhham, thank you for the review. Indeed looks like compilation is broken since rebase. I'll check it and address the comments!

whyoleg

This is really a big step forward for the ecosystem!

Mostly skimmed though public declarations

whyoleg · 2024-05-27T06:41:48Z

ktor-io/common/src/io/ktor/utils/io/core/Closeable.kt

+/*
+ * Copyright 2014-2024 JetBrains s.r.o and contributors. Use of this source code is governed by the Apache 2.0 license.
+ */
+
 package io.ktor.utils.io.core

 public expect interface Closeable {


should it be replaced with AutoCloseable (with a deprecated typealias, same for use function) as it's stable in Kotlin 2.0?

Yep, will replace this with 2.0.0 bump

whyoleg · 2024-05-27T06:43:06Z

ktor-io/common/src/io/ktor/utils/io/core/Copy.kt

-}
+@Deprecated(
+    "Use transferTo instead",
+    ReplaceWith("output.transferTo(this)", "kotlinx.io.transferTo"),


replace with is not the same as implementation transferFrom vs transferTo

whyoleg · 2024-05-27T06:46:58Z

ktor-io/common/src/io/ktor/utils/io/core/Memory.kt

+    this[index + 1] = (value shr 16).toByte()
+    this[index + 2] = (value shr 8).toByte()
+    this[index + 3] = value.toByte()
+}


as far as I see, it's used only in WebSocket implementation and can be rather easily replaced with operations on kotlinx-io primitives.
IMO, there is no need to have such functions exposed nor in ktor, nor in kotlinx-io, as more high level primitives should be used.
Additionally, there will be something like this for unsafe access described in Kotlin/kotlinx-io#135 (comment)

whyoleg · 2024-05-27T06:48:59Z

ktor-io/common/src/io/ktor/utils/io/errors/Exceptions.kt

+
+package io.ktor.utils.io.errors
+
+public typealias IOException = kotlinx.io.IOException


may be those should be deprecated, so users will migrate to kotlinx.io.* types?

Thank you, good catch! Will add

whyoleg · 2024-05-27T06:54:33Z

ktor-io/common/src/io/ktor/utils/io/pool/ByteArrayPool.kt

+private const val DEFAULT_POOL_ARRAY_SIZE = 4096
+private const val DEFAULT_POOL_CAPACITY = 128
+
+public val ByteArrayPool: ObjectPool<ByteArray> = object : DefaultPool<ByteArray>(DEFAULT_POOL_CAPACITY) {


can it be deprecated and replaced with kotlinx-io APIs (I mean, creating Buffer and using it)

Sure, it can be. Let me check

It needs a huge rewrite in the other ktor modules. I prefer to keep it in the separate PR for stability

Better to create YT issue and link it here?

Sure, will add

ktor-io/js/src/io/ktor/utils/io/charsets/TextEncoder.js.kt

ktor-io/jvm/src/io/ktor/utils/io/core/BufferPrimitivesJvm.kt

bjhham · 2024-05-28T13:14:49Z

Looks like ktor-benchmarks is missing the required repository for the snapshot of kotlinx-io?

14:56:11     Execution failed for task ':compileKotlin'.
14:56:11     > Could not resolve all files for configuration ':compileClasspath'.
14:56:11        > Could not find org.jetbrains.kotlinx:kotlinx-io-core:0.3.2-SNAPSHOT.
14:56:11          Searched in the following locations:
14:56:11            - file:/home/teamcity/.m2/repository/org/jetbrains/kotlinx/kotlinx-io-core/0.3.2-SNAPSHOT/maven-metadata.xml
14:56:11            - file:/home/teamcity/.m2/repository/org/jetbrains/kotlinx/kotlinx-io-core/0.3.2-SNAPSHOT/kotlinx-io-core-0.3.2-SNAPSHOT.pom
14:56:11            - https://repo.maven.apache.org/maven2/org/jetbrains/kotlinx/kotlinx-io-core/0.3.2-SNAPSHOT/maven-metadata.xml
14:56:11            - https://repo.maven.apache.org/maven2/org/jetbrains/kotlinx/kotlinx-io-core/0.3.2-SNAPSHOT/kotlinx-io-core-0.3.2-SNAPSHOT.pom
14:56:11            - https://maven.pkg.jetbrains.space/kotlin/p/kotlin/dev/org/jetbrains/kotlinx/kotlinx-io-core/0.3.2-SNAPSHOT/maven-metadata.xml
14:56:11            - https://maven.pkg.jetbrains.space/kotlin/p/kotlin/dev/org/jetbrains/kotlinx/kotlinx-io-core/0.3.2-SNAPSHOT/kotlinx-io-core-0.3.2-SNAPSHOT.pom
14:56:11            - https://maven.pkg.jetbrains.space/public/p/ktor/eap/org/jetbrains/kotlinx/kotlinx-io-core/0.3.2-SNAPSHOT/maven-metadata.xml
14:56:11            - https://maven.pkg.jetbrains.space/public/p/ktor/eap/org/jetbrains/kotlinx/kotlinx-io-core/0.3.2-SNAPSHOT/kotlinx-io-core-0.3.2-SNAPSHOT.pom
14:56:11          Required by:
14:56:11              project : > io.ktor:ktor-server-core:1.0.0-BENCHMARKS > io.ktor:ktor-server-core-jvm:1.0.0-BENCHMARKS > io.ktor:ktor-utils:1.0.0-BENCHMARKS > io.ktor:ktor-utils-jvm:1.0.0-BENCHMARKS > io.ktor:ktor-io:1.0.0-BENCHMARKS > io.ktor:ktor-io-jvm:1.0.0-BENCHMARKS

This reverts commit c03c0f0.

e5l requested review from bjhham, marychatte and Stexxe May 15, 2024 08:59

e5l self-assigned this May 15, 2024

e5l force-pushed the e5l/kotlinx-io-2 branch from d24d409 to ec472cf Compare May 15, 2024 09:45

e5l changed the title ~~Migrate Ktor to kotlinx-io~~ KTOR-6030 Migrate Ktor to kotlinx-io May 15, 2024

e5l force-pushed the e5l/kotlinx-io-2 branch from ec472cf to 0604889 Compare May 15, 2024 11:53

e5l marked this pull request as ready for review May 17, 2024 08:16

bjhham requested changes May 21, 2024

View reviewed changes

westnordost mentioned this pull request May 23, 2024

Multiplatform file access streetcomplete/StreetComplete#5656

Draft

whyoleg reviewed May 27, 2024

View reviewed changes

e5l added 2 commits May 28, 2024 09:48

Migrate to kotlinx-io

06cf5ac

WIP

52c6575

e5l force-pushed the e5l/kotlinx-io-2 branch from 0604889 to 52c6575 Compare May 28, 2024 12:44

e5l added 3 commits May 28, 2024 15:45

Drop created

eabafe1

fixup! WIP

4b68beb

Make reader and writer await for nested operations

b3c0bf7

Goooler mentioned this pull request May 30, 2024

Add UnknownServiceException Kotlin/kotlinx-io#333

Closed

Revert "Use KotlinHierarchyTemplate for structuring source sets (#4045)"

fc8fe1c

This reverts commit c03c0f0.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KTOR-6030 Migrate Ktor to kotlinx-io #4032

KTOR-6030 Migrate Ktor to kotlinx-io #4032

e5l commented May 15, 2024 •

edited

bjhham left a comment

bjhham May 21, 2024

bjhham May 21, 2024

bjhham May 21, 2024

e5l May 28, 2024

bjhham May 21, 2024

e5l May 28, 2024

bjhham May 21, 2024

whyoleg May 27, 2024

bjhham May 21, 2024

bjhham May 21, 2024

e5l commented May 27, 2024

whyoleg left a comment

whyoleg May 27, 2024

e5l May 28, 2024

whyoleg May 27, 2024

whyoleg May 27, 2024

whyoleg May 27, 2024

e5l May 28, 2024

whyoleg May 27, 2024

e5l May 28, 2024

e5l May 29, 2024

whyoleg May 29, 2024

e5l May 29, 2024

bjhham commented May 28, 2024


		internal class SourceByteReadChannel(private val source: Source) : ByteReadChannel {

		val created = Exception()


		package io.ktor.utils.io.errors

		public typealias IOException = kotlinx.io.IOException

KTOR-6030 Migrate Ktor to kotlinx-io #4032

Are you sure you want to change the base?

KTOR-6030 Migrate Ktor to kotlinx-io #4032

Conversation

e5l commented May 15, 2024 • edited

bjhham left a 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

e5l commented May 27, 2024

whyoleg left a 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

Choose a reason for hiding this comment

bjhham commented May 28, 2024

e5l commented May 15, 2024 •

edited