Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

KTOR-6004 Support TCP and Unix socket for wasm-js and js via NodeJS #4411

Merged
merged 7 commits into from
Oct 31, 2024

Conversation

whyoleg
Copy link
Contributor

@whyoleg whyoleg commented Oct 18, 2024

Subsystem
ktor-network

Motivation
Fixes https://youtrack.jetbrains.com/issue/KTOR-6004/Support-NodeJs-target-for-ktor-network

This is the first PR in a series to support ktor client/server for all targets available. Following PRs will contain CIO server and client engines support for NodeJS. The whole scope of changes can be found here.

Fully commonizing ktor-network and other modules, will also significantly simplify (from the build-logic perspective) support for wasm-wasi in ktor-network and through the whole stack of ktor

Solution
Use node:net for sockets. SelectorManager is no-op for nodejs, as there is no need for it. Still it's better to commonize it to simplify the configuration. It will probably be also needed for wasm-wasi support.
In future, more high-level API could be introduced for sockets and SelectorManager could be made internal.

@e5l
Copy link
Member

e5l commented Oct 18, 2024

Please check API and code style build. I would also check publishToMavenLocal.

@whyoleg
Copy link
Contributor Author

whyoleg commented Oct 18, 2024

apiCheck/lintKotlin/publishToMavenLocal works fine locally

Copy link
Member

@osipxd osipxd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR! Looks good to me, just a couple of comments


import io.ktor.network.sockets.*

internal external interface JsError // js.Error
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've seen we already have JsError type in another module, though it has different implementation. Should we move JsError to ktor-utils?

And I'm just wondering shouldn't Kotlin compiler map js.Error to Throwable and vice versa?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we move JsError to ktor-utils?

There is JsError in ktor-client-core, but it's a bit different. Not sure, that it makes sense to extract those.

Kotlin compiler map js.Error to Throwable

And they are - in JS target, but not for wasm.
In wasmJs, Throwable is wasm exception, while js.Error is JS exception. And so far, I don't think that there function to map from one to another out-of-the-box.
AFAIK in Kotlin/Wasm exception thrown from JS part will be represented as JsException. More info on kotlinlang.
But here, the exception is not thrown, but passed via callback. So probably to convert it we could write something like:

fun convert(error: JsError): Throwable {
  try {
    throw(error)
  } catch(cause: Throwable) {
    return cause
  }
}

private fun throw(error: JsError): Nothing = js("throw error")

I will try to experiment with it. The alternative will be to just have something like:

fun convert(error: JsError): Throwable = CustomException("Exception from js: $error")

may be @bashor nows some other way

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the explanation!

import io.ktor.network.sockets.*
import org.khronos.webgl.*

internal actual fun nodeNet(): NodeNet? = js("eval('require')('node:net')").unsafeCast<NodeNet?>()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't it lead to KTOR-6158?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it will :)
I saw the discussion around it and not sure what the best approach will be here.
Initially I was using the other approach (compatible with ES), but refactored to use this "hack" because after implementing ktor-client-cio for js/wasm-js I was failed to run ktor-client-tests module tests in browser, as with direct dependency on net via JsModule annotation it's not possible to use it from browser, and I see the error like this coming from webpack:

Module not found: Error: Can't resolve 'net' in '.../ktorio/ktor/build/js/packages/ktor-ktor-client-ktor-client-tests-test/kotlin'
  Uncaught Error: Cannot find module 'net'

Even if it will be not really used in ktor-client-tests when running in browser, the code is there and so webpack will complain.

So we probably have two options:

  1. leave it as is - then network and CIO engine will be not compatible with ES modules. Note, the same issue exists for kotlinx-io (Supoort EsModule when target in node.js Kotlin/kotlinx-io#345) and as ktor depends on kotlinx-io not sure that it will be compatible in the end. Then, after all other PR's (with CIO server, CIO client and client tests support for multiple engines on js/wasm-js) will be merged and everything will work, we could revisit this
  2. refactor ktor-client-tests somehow, so that the code run in browser and in nodejs is different somehow.

I'm more in favor of first approach now, but If you do have other ideas, feel free to suggest :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm okay with the first option if we create an issue to track compatibility of network module with ES and add this context there. @e5l what do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, the first option looks good

Copy link
Member

@osipxd osipxd Oct 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@whyoleg, could you create an issue to track ES compatibility and rebase this branch one more time? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@osipxd osipxd changed the title Support TCP and Unix socket for wasm-js and js via NodeJS KTOR-6004 Support TCP and Unix socket for wasm-js and js via NodeJS Oct 30, 2024
@osipxd osipxd merged commit 5b086cc into ktorio:3.1.0-eap Oct 31, 2024
11 of 13 checks passed
@osipxd
Copy link
Member

osipxd commented Oct 31, 2024

Thank you for your contribution! 🎉

osipxd pushed a commit that referenced this pull request Nov 8, 2024
…4411)

* Drop `jvmAndNix` shared source set
* Commonize `ktor-network` and `ktor-network-tls`
* Support TCP and Unix sockets for wasm-js and js on Node
* Move `supportsUnixDomainSockets` to posix and use Platform instead of expect/actual
e5l pushed a commit that referenced this pull request Nov 13, 2024
…4411)

* Drop `jvmAndNix` shared source set
* Commonize `ktor-network` and `ktor-network-tls`
* Support TCP and Unix sockets for wasm-js and js on Node
* Move `supportsUnixDomainSockets` to posix and use Platform instead of expect/actual
e5l pushed a commit that referenced this pull request Nov 14, 2024
…4411)

* Drop `jvmAndNix` shared source set
* Commonize `ktor-network` and `ktor-network-tls`
* Support TCP and Unix sockets for wasm-js and js on Node
* Move `supportsUnixDomainSockets` to posix and use Platform instead of expect/actual
osipxd pushed a commit that referenced this pull request Nov 16, 2024
…4411)

* Drop `jvmAndNix` shared source set
* Commonize `ktor-network` and `ktor-network-tls`
* Support TCP and Unix sockets for wasm-js and js on Node
* Move `supportsUnixDomainSockets` to posix and use Platform instead of expect/actual
e5l pushed a commit that referenced this pull request Nov 19, 2024
…4411)

* Drop `jvmAndNix` shared source set
* Commonize `ktor-network` and `ktor-network-tls`
* Support TCP and Unix sockets for wasm-js and js on Node
* Move `supportsUnixDomainSockets` to posix and use Platform instead of expect/actual
osipxd pushed a commit that referenced this pull request Nov 22, 2024
…4411)

* Drop `jvmAndNix` shared source set
* Commonize `ktor-network` and `ktor-network-tls`
* Support TCP and Unix sockets for wasm-js and js on Node
* Move `supportsUnixDomainSockets` to posix and use Platform instead of expect/actual
e5l pushed a commit that referenced this pull request Nov 25, 2024
…4411)

* Drop `jvmAndNix` shared source set
* Commonize `ktor-network` and `ktor-network-tls`
* Support TCP and Unix sockets for wasm-js and js on Node
* Move `supportsUnixDomainSockets` to posix and use Platform instead of expect/actual
e5l pushed a commit that referenced this pull request Nov 26, 2024
…4411)

* Drop `jvmAndNix` shared source set
* Commonize `ktor-network` and `ktor-network-tls`
* Support TCP and Unix sockets for wasm-js and js on Node
* Move `supportsUnixDomainSockets` to posix and use Platform instead of expect/actual
e5l pushed a commit that referenced this pull request Nov 27, 2024
…4411)

* Drop `jvmAndNix` shared source set
* Commonize `ktor-network` and `ktor-network-tls`
* Support TCP and Unix sockets for wasm-js and js on Node
* Move `supportsUnixDomainSockets` to posix and use Platform instead of expect/actual
e5l pushed a commit that referenced this pull request Nov 28, 2024
…4411)

* Drop `jvmAndNix` shared source set
* Commonize `ktor-network` and `ktor-network-tls`
* Support TCP and Unix sockets for wasm-js and js on Node
* Move `supportsUnixDomainSockets` to posix and use Platform instead of expect/actual
e5l pushed a commit that referenced this pull request Nov 29, 2024
…4411)

* Drop `jvmAndNix` shared source set
* Commonize `ktor-network` and `ktor-network-tls`
* Support TCP and Unix sockets for wasm-js and js on Node
* Move `supportsUnixDomainSockets` to posix and use Platform instead of expect/actual
e5l pushed a commit that referenced this pull request Dec 3, 2024
…4411)

* Drop `jvmAndNix` shared source set
* Commonize `ktor-network` and `ktor-network-tls`
* Support TCP and Unix sockets for wasm-js and js on Node
* Move `supportsUnixDomainSockets` to posix and use Platform instead of expect/actual
e5l pushed a commit that referenced this pull request Dec 3, 2024
…4411)

* Drop `jvmAndNix` shared source set
* Commonize `ktor-network` and `ktor-network-tls`
* Support TCP and Unix sockets for wasm-js and js on Node
* Move `supportsUnixDomainSockets` to posix and use Platform instead of expect/actual
osipxd pushed a commit that referenced this pull request Dec 6, 2024
…4411)

* Drop `jvmAndNix` shared source set
* Commonize `ktor-network` and `ktor-network-tls`
* Support TCP and Unix sockets for wasm-js and js on Node
* Move `supportsUnixDomainSockets` to posix and use Platform instead of expect/actual
osipxd pushed a commit that referenced this pull request Dec 11, 2024
…4411)

* Drop `jvmAndNix` shared source set
* Commonize `ktor-network` and `ktor-network-tls`
* Support TCP and Unix sockets for wasm-js and js on Node
* Move `supportsUnixDomainSockets` to posix and use Platform instead of expect/actual
osipxd pushed a commit that referenced this pull request Dec 12, 2024
…4411)

* Drop `jvmAndNix` shared source set
* Commonize `ktor-network` and `ktor-network-tls`
* Support TCP and Unix sockets for wasm-js and js on Node
* Move `supportsUnixDomainSockets` to posix and use Platform instead of expect/actual
osipxd pushed a commit that referenced this pull request Dec 12, 2024
…4411)

* Drop `jvmAndNix` shared source set
* Commonize `ktor-network` and `ktor-network-tls`
* Support TCP and Unix sockets for wasm-js and js on Node
* Move `supportsUnixDomainSockets` to posix and use Platform instead of expect/actual
osipxd pushed a commit that referenced this pull request Dec 19, 2024
…4411)

* Drop `jvmAndNix` shared source set
* Commonize `ktor-network` and `ktor-network-tls`
* Support TCP and Unix sockets for wasm-js and js on Node
* Move `supportsUnixDomainSockets` to posix and use Platform instead of expect/actual
Stexxe pushed a commit that referenced this pull request Dec 24, 2024
…4411)

* Drop `jvmAndNix` shared source set
* Commonize `ktor-network` and `ktor-network-tls`
* Support TCP and Unix sockets for wasm-js and js on Node
* Move `supportsUnixDomainSockets` to posix and use Platform instead of expect/actual
e5l pushed a commit that referenced this pull request Jan 8, 2025
…4411)

* Drop `jvmAndNix` shared source set
* Commonize `ktor-network` and `ktor-network-tls`
* Support TCP and Unix sockets for wasm-js and js on Node
* Move `supportsUnixDomainSockets` to posix and use Platform instead of expect/actual
osipxd pushed a commit that referenced this pull request Jan 9, 2025
…4411)

* Drop `jvmAndNix` shared source set
* Commonize `ktor-network` and `ktor-network-tls`
* Support TCP and Unix sockets for wasm-js and js on Node
* Move `supportsUnixDomainSockets` to posix and use Platform instead of expect/actual
osipxd pushed a commit that referenced this pull request Jan 9, 2025
…4411)

* Drop `jvmAndNix` shared source set
* Commonize `ktor-network` and `ktor-network-tls`
* Support TCP and Unix sockets for wasm-js and js on Node
* Move `supportsUnixDomainSockets` to posix and use Platform instead of expect/actual
osipxd pushed a commit that referenced this pull request Jan 9, 2025
…4411)

* Drop `jvmAndNix` shared source set
* Commonize `ktor-network` and `ktor-network-tls`
* Support TCP and Unix sockets for wasm-js and js on Node
* Move `supportsUnixDomainSockets` to posix and use Platform instead of expect/actual
osipxd pushed a commit that referenced this pull request Jan 9, 2025
…4411)

* Drop `jvmAndNix` shared source set
* Commonize `ktor-network` and `ktor-network-tls`
* Support TCP and Unix sockets for wasm-js and js on Node
* Move `supportsUnixDomainSockets` to posix and use Platform instead of expect/actual
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants