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.

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

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

bjhham

Looks like she's good to go, pending the kotlinx-io release.

bjhham · 2024-06-13T06:49:55Z

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

+/**
+ * Sequential (non-concurrent) byte channel implementation
+ */
+public class ByteChannel(public val autoFlush: Boolean = false) : ByteReadChannel, BufferedByteWriteChannel {


Looks like we can drop this argument here.

bjhham · 2024-06-13T06:57:54Z

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

+public class ByteChannel(public val autoFlush: Boolean = false) : ByteReadChannel, BufferedByteWriteChannel {
+    private val _closedCause = atomic<CloseToken?>(null)
+    private val slot = AwaitingSlot()
+    private val flushBuffer: Buffer = Buffer()


Is the intent with this class to eventually drop the three buffers and flush orchestration in favour of just using a single buffer with some synchronization? Seems like we could just have a single bounded buffer that blocks writes until there's space.

The problem is that not all operations writing this buffer are suspend, so you need actually block the thread

Anyway let's discuss this idea

mgroth0 · 2024-06-28T14:01:39Z

Great work. Looking forward to this integration!

Thomas-Vos · 2024-07-11T17:42:52Z

ktor-network/nix/src/io/ktor/network/sockets/DatagramSendChannel.kt

+        val buffer = Buffer()
        try {
-            DefaultDatagramChunkBufferPool.useInstance { buffer ->
-                element.packet.copy().readAvailable(buffer)
-
-                val bytes = element.packet.copy().readBytes()
-                val bytesWritten = sento(element, bytes)
-
-                result = when (bytesWritten) {
-                    0 -> throw IOException("Failed writing to closed socket")
-                    -1 -> {
-                        if (errno == EAGAIN) {
-                            false
-                        } else {
-                            throw PosixException.forErrno()
-                        }
+            element.packet.copy().readAvailable(buffer)
+
+            val bytes = element.packet.copy().readBytes()
+            val bytesWritten = sento(element, bytes)
+
+            result = when (bytesWritten) {
+                0 -> throw kotlinx.io.IOException("Failed writing to closed socket")
+                -1 -> {
+                    if (errno == EAGAIN) {
+                        false
+                    } else {
+                        throw PosixException.forErrno()
                    }
-                    else -> true
                }
+
+                else -> true
            }
        } finally {
+            buffer.close()


While trying to fix another issue I noticed that buffer may not actually be used. The packet is copied into buffer, but then never read. I guess the buffer can just be removed, right? Or use the buffer properly instead of readBytes.

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 force-pushed the e5l/kotlinx-io-2 branch from 0604889 to 52c6575 Compare May 28, 2024 12:44

Goooler mentioned this pull request May 30, 2024

Add UnknownServiceException Kotlin/kotlinx-io#333

Closed

bjhham mentioned this pull request Jun 10, 2024

Fixes for kotlinx-io byte channel handling #4070

Merged

fzhinkin mentioned this pull request Jun 11, 2024

Better segment pools Kotlin/kotlinx-io#311

Open

7 tasks

bjhham approved these changes Jun 13, 2024

View reviewed changes

westnordost mentioned this pull request Jun 14, 2024

Performance issue for large data / kotlinx-io pdvrieze/xmlutil#217

Open

e5l force-pushed the e5l/kotlinx-io-2 branch from 7e2f0e7 to 27c3453 Compare July 3, 2024 11:57

e5l force-pushed the e5l/kotlinx-io-2 branch 2 times, most recently from bb406bf to 93b6a55 Compare July 11, 2024 09:27

Thomas-Vos reviewed Jul 11, 2024

View reviewed changes

e5l added 6 commits July 15, 2024 09:07

Migrate to kotlinx-io

e8d3ee7

WIP

45fc580

Drop created

8e8ef4c

fixup! WIP

7495966

Make reader and writer await for nested operations

788e54b

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

0e12d9a

This reverts commit c03c0f0.

bjhham and others added 26 commits July 15, 2024 09:07

Code formatting

25eb87a

Update kotlinx-io snapshot

e51f80e

Update kotlinx-io version

f6c866e

Bump kotlinx-io version

1cdb087

Fix import

3293a3a

Fix yarn lock

aea3202

Fix exception type

f27589a

Update to kotlin 2.0.0

8fcb78c

Update atomicfu

d4818dd

Remove allocation in NettyHttpResponsePipeline

fad96cb

Reduce allocations in CIO writer and FileChannels

a62bb77

Reduce allocations count

0325c18

Update configuration

bc34b51

Inline awaiting slot

bd6cba0

Exclude corner cases from suspension

e33877a

Split read and write; Make suspension inline

2213968

Fix netty and jetty allocations

f40ca0a

Rollback reusable continuation

9230cac

Update yarn lock

74c4ca2

Fix response validator test condition

02777ea

Fix testPingPongTimeout for macos

afcef95

Add missing wasm actuals

3697e48

Drop header allocations

945653d

Fix js request cancellation

21e6e6c

Fix code style

6a8627e

Update kotlinx-io to 0.5.0

4e73c23

e5l force-pushed the e5l/kotlinx-io-2 branch from 93b6a55 to 4e73c23 Compare July 15, 2024 07:43

e5l merged commit 40042dc into main Jul 15, 2024
10 of 17 checks passed

e5l deleted the e5l/kotlinx-io-2 branch July 15, 2024 08:35

osipxd mentioned this pull request Sep 27, 2024

Use targets hierarchy API #4349

Merged

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

Loading

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

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

bjhham left a comment

bjhham Jun 13, 2024

bjhham Jun 13, 2024

e5l Jun 13, 2024 •

edited

Loading

e5l Jun 13, 2024

mgroth0 commented Jun 28, 2024

Thomas-Vos Jul 11, 2024 •

edited

Loading


		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

KTOR-6030 Migrate Ktor to kotlinx-io #4032

Conversation

e5l commented May 15, 2024 • edited Loading

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

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

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

e5l Jun 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgroth0 commented Jun 28, 2024

Thomas-Vos Jul 11, 2024 • edited Loading

Choose a reason for hiding this comment

e5l commented May 15, 2024 •

edited

Loading

e5l Jun 13, 2024 •

edited

Loading

Thomas-Vos Jul 11, 2024 •

edited

Loading