KTOR-7675 Support CIO client for wasm-js and js #4441

whyoleg · 2024-11-01T16:38:10Z

Subsystem
Client CIO

Motivation
Fixes: https://youtrack.jetbrains.com/issue/KTOR-7675/Client-CIO-engine-support-for-wasm-js-and-js
It's a preparation for server CIO support for wasm-js and js.

Solution
Most of the code in ktor-client-cio is just copied with minor changes.
The most of the changes are related to two general parts in client tests:

support for client engines registration for wasm-js and js too support automatic engine selection and allows to dynamically access registered engines in ktor-client-tests
commonize ktor-client-tests code and align logic of filtering between platforms

… CIO engine on nodejs(js+wasmJs)

whyoleg · 2024-11-01T18:15:45Z

ktor-client/ktor-client-tests/posix/src/io/ktor/client/tests/utils/ClientLoaderNative.kt

-        onlyWithEngine: String?,
-        block: suspend TestClientBuilder<HttpClientEngineConfig>.() -> Unit
-    ) {
-        if (skipEngines.any { it.startsWith("native") }) return


Note: because of this condition, some of the tests previously were skipped wrong.

E.g if f.e there were skipEngines=listOf("native:CIO"), because of this condition (startsWith("native")), the test was skipped for all engines on native.

Because it's fixed now, some of the test are failed on CI :)
I will skip again some of them - probably all other tests should be checked also for false positive - e.g if it says skip then it should fail - not sure how it will work on practice

ktor-client/ktor-client-core/jvm/src/io/ktor/client/HttpClientJvm.kt

osipxd

Thank you for the pull request!
I have just a few minor comments.

ktor-client/ktor-client-core/jsAndWasmShared/src/io/ktor/client/engine/Loader.kt

osipxd · 2024-11-05T12:45:00Z

ktor-client/ktor-client-core/jvm/src/io/ktor/client/HttpClientJvm.kt

+        HttpClientEngineContainer::class.java,
+        HttpClientEngineContainer::class.java.classLoader
+    ).iterator()
+}.toList()


toList conversion could be omitted

Suggested change

}.toList()

}

I've created a separate task to fix this ServiceLoader issue in the whole project: KTOR-7698

I will drop changes in this file from this PR.

Yeah, we could drop toList here, but overall, it's more of a safeguard for not calling ServiceLoader.load multiple times: as Iterable { ... } factory will call lambda (creating Iterator), and so ServiceLoader.load each time it's accessed.
e.g:

val engines = Iterable { ... } val a1 = engines.firstOrNull() // will create iterator val a2 = engines.firstOrNull() // will create one more iterator

Just to keep in mind, and probably in this case would be nice to document this somewhere

osipxd · 2024-11-05T13:13:52Z

ktor-client/ktor-client-tests/common/src/io/ktor/client/tests/utils/ClientLoader.kt


 /**
 * Helper interface to test client.
 */
-expect abstract class ClientLoader(timeoutSeconds: Int = 60) {
+abstract class ClientLoader(private val timeout: Duration = 1.minutes) {
    /**
     * Perform test against all clients from dependencies.
     */
    fun clientTests(
        skipEngines: List<String> = emptyList(),


Off-topic. This parameter was always confusing for me because clientTests calls usually look like clientTests(listOf("Js", "Jetty")) and I read it like "run tests on Js and Jetty clients" which is completely opposite to the truth.
If it is not only me, I'll create an issue.

I agree, it's very confusing, though, listing all engines to run tests will be worse, as it will be too wordy in most cases and probably not worth it.

As a middle ground, it's possible to add vararg hack: Unit as a first argument to enforce named arguments :)
Credits for this wild idea goes to https://github.com/typesafegithub/github-workflows-kt/blob/52701fc95016d449a150a68f57763b365a429d47/github-workflows-kt/src/main/kotlin/io/github/typesafegithub/workflows/dsl/JobBuilder.kt#L65
Not sure it's worth it, but at least it will be much more explicit.

KTOR-7723 Engine exclusion from clientTests is confusing

ktor-client/ktor-client-core/jvm/src/io/ktor/client/HttpClientJvm.kt

osipxd

LGTM! Thanks

whyoleg · 2024-11-08T12:05:22Z

@osipxd, just curious, is there anything else needed from my side, so that the PR could be merged?
I'm already ready to create the last PR in a series with server support :)

osipxd · 2024-11-08T14:10:46Z

Nothing! I was just waiting for CI and then forgot to merge this.
Thank you for pinging me!

* Support multiple client engines for JS/WasmJs * CIO client Js/WasmJs support * Enable CIO tests in client tests * Make client engines `data object` to have `toString` * Make ClientLoader work with multiple engines on js/wasmJs and so test CIO engine on nodejs(js+wasmJs)

whyoleg added 6 commits November 1, 2024 10:06

Support multiple client engines for JS/WasmJs

53153e2

CIO client Js/WasmJs support

fe05a4a

Enable CIO tests in client tests

c1e5a7b

Make client engines data object to have toString

21cd5ea

Make ClientLoader work with multiple engines on js/wasmJs and so test…

b6eede2

… CIO engine on nodejs(js+wasmJs)

fix formatting

168e06e

whyoleg changed the title ~~Support CIO client for wasm-js and js~~ KTOR-7675 Support CIO client for wasm-js and js Nov 1, 2024

whyoleg commented Nov 1, 2024

View reviewed changes

whyoleg added 2 commits November 1, 2024 20:34

fix some tests

a5ed2c5

fix code style

442a8b3

osipxd self-requested a review November 4, 2024 16:36

whyoleg commented Nov 5, 2024

View reviewed changes

ktor-client/ktor-client-core/jvm/src/io/ktor/client/HttpClientJvm.kt Outdated Show resolved Hide resolved

osipxd mentioned this pull request Nov 5, 2024

KTOR-7698 Make ServiceLoader.load call optimizable by R8 #4454

Merged

osipxd reviewed Nov 5, 2024

View reviewed changes

whyoleg added 2 commits November 5, 2024 19:16

revert ServiceLoader changes in favor of KTOR-7698

f072c64

move engines to nonJvm source set

319ed68

osipxd self-requested a review November 7, 2024 09:05

osipxd approved these changes Nov 7, 2024

View reviewed changes

osipxd merged commit 4a8a103 into ktorio:3.1.0-eap Nov 8, 2024
11 of 13 checks passed

whyoleg deleted the commonize-cio-client branch November 8, 2024 16:39

osipxd mentioned this pull request Dec 16, 2024

Cherry-pick common ClientLoader implementation from 3.1.0-eap #4550

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KTOR-7675 Support CIO client for wasm-js and js #4441

KTOR-7675 Support CIO client for wasm-js and js #4441

whyoleg commented Nov 1, 2024 •

edited

Loading

whyoleg Nov 1, 2024

osipxd left a comment

osipxd Nov 5, 2024

osipxd Nov 5, 2024

whyoleg Nov 5, 2024

osipxd Nov 5, 2024

whyoleg Nov 5, 2024 •

edited

Loading

osipxd Nov 7, 2024

osipxd left a comment

whyoleg commented Nov 8, 2024

osipxd commented Nov 8, 2024

KTOR-7675 Support CIO client for wasm-js and js #4441

KTOR-7675 Support CIO client for wasm-js and js #4441

Conversation

whyoleg commented Nov 1, 2024 • edited Loading

whyoleg Nov 1, 2024

Choose a reason for hiding this comment

osipxd left a comment

Choose a reason for hiding this comment

osipxd Nov 5, 2024

Choose a reason for hiding this comment

osipxd Nov 5, 2024

Choose a reason for hiding this comment

whyoleg Nov 5, 2024

Choose a reason for hiding this comment

osipxd Nov 5, 2024

Choose a reason for hiding this comment

whyoleg Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

osipxd Nov 7, 2024

Choose a reason for hiding this comment

osipxd left a comment

Choose a reason for hiding this comment

whyoleg commented Nov 8, 2024

osipxd commented Nov 8, 2024

whyoleg commented Nov 1, 2024 •

edited

Loading

whyoleg Nov 5, 2024 •

edited

Loading