Support CIO server for wasm-js and js #4466

whyoleg · 2024-11-08T17:00:20Z

Subsystem
Server CIO

Motivation
Probably fixes https://youtrack.jetbrains.com/issue/KTOR-865/Add-node.js-server-engine, though not sure, may be more specific issue should be created

Solution
Most of the code in ktor-server-cio is just copied with minor changes.

P.S. This is the last PR in a series of ktor server support for wasm-js and js! wasm-wasi will be next, probably, if I will be able to find working runtime which supports preview1 socket APIs...

osipxd · 2024-11-13T12:51:57Z

Relates to:

KTOR-2939 ApplicationEngine.start()/stop() should be suspending
KTOR-7459 Implement a suspending version of EmbeddedServer.start(wait=true)

osipxd

This is the last PR in a series of ktor server support for wasm-js and js!

Thank you, it's a huge work 🎉

Please, check JVM builds on CI, they fail after two hours with timeout because of hanging NettyHttp2ServerCommonTest.

ktor-server/ktor-server-cio/posix/src/io/ktor/server/cio/internal/CoroutineUtilsNix.kt

...ver-test-base/jsAndWasmShared/src/io/ktor/server/test/base/EngineTestBase.jsAndWasmShared.kt

...ver-cio/jsAndWasmShared/src/io/ktor/server/cio/backend/SocketAddressUtils.jsAndWasmShared.kt

osipxd · 2024-11-13T13:51:15Z

ktor-server/ktor-server-core/common/src/io/ktor/server/engine/BaseApplicationResponse.kt

@@ -179,6 +179,8 @@ public abstract class BaseApplicationResponse(
                }
            } catch (closed: ClosedWriteChannelException) {
                throw ChannelWriteException(exception = closed)
+            } finally {
+                flushAndClose()


Why do we need to call flushAndClose inside .use { } block?

TBH, I'm not fully sure if this fixes the original issue (some tests failure), so I will revert this change...
The idea behind this change is that use on ByteWriteChannel is custom and uses close which in reality calls flushAndClose, but doesn't wait for it completion.
so use{} and flushAndClose are mostly the same, except for the fact that flushAndClose really suspends

reverted in 9bbd0d3

Now JVM tests are working fine, looks like this was the reason, as this is the only change which affects JVM.
But now testErrorInBodyClosesConnection started to fail again - it shows it as flaky, so may be it's really related to this fireAndForget close method in channels

@e5l, could you take a look? Should we call flushAndClose instead of deprecated close inside of use?

The difference is in the exception: flushAndClose can throw if the flush is failed. Could you check if this is the case?

Do you think may be it will be better to create a separate issue for this to investigate? :)

I see, that the same test fails sometimes (flaky) not only for js/wasm-js. And as I reverted this change, it should not affect anything.

Yes, let's move this investigation into a separate task

...ktor-server-core/jsAndWasmShared/src/io/ktor/server/engine/EmbeddedServer.jsAndWasmShared.kt

whyoleg · 2024-11-13T18:12:41Z

@osipxd should I create an additional issue for this, or https://youtrack.jetbrains.com/issue/KTOR-865/Add-node.js-server-engine will be fine?

Relates to:

KTOR-2939 ApplicationEngine.start()/stop() should be suspending

KTOR-7459 Implement a suspending version of EmbeddedServer.start(wait=true)

Do you think it will be better to extract those changes and create a separate PR for them? If so, I will do it :)

osipxd · 2024-11-14T16:05:10Z

Should I create an additional issue for this, or https://youtrack.jetbrains.com/issue/KTOR-865/Add-node.js-server-engine will be fine?

@e5l, was this task created for some new Node.js engine or CIO fits the purpose?

Relates to:

KTOR-2939 ApplicationEngine.start()/stop() should be suspending

KTOR-7459 Implement a suspending version of EmbeddedServer.start(wait=true)

Do you think it will be better to extract those changes and create a separate PR for them? If so, I will do it :)

Yes, it would be great to discuss this separately. Probably we should deprecate the existing start/stop implementation and at some point and gradually migrate to the suspend version.

whyoleg · 2024-11-14T16:50:39Z

Yes, it would be great to discuss this separately.

Extracted to #4481

* Use stdlib `use` extension-function * Update type checks

Co-authored-by: Osip Fatkullin <[email protected]>

…torio#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

* 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)

)

) * KTOR-6632 Support receiving multipart data with Ktor client

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* Test libcurl build with conan * Take both curl and openssl from the same build * test no linux cache * Move K/N linux cache disabling to just `ktor-client-curl` module

Some services use 403 instead of 401. Changing them might be impossible. With this change Ktor can flexibly work with any broken service. --------- Co-authored-by: Osip Fatkullin <[email protected]>

* KTOR-7679 Allow disabling body decoding on server

…EmbeddedServer.stop (ktorio#4481) * Add startSuspend/stopSuspend in EmbeddedServer * Make EngineTestBase work on js/wasmJs

…tests

This reverts commit 33c00f8.

e5l · 2024-11-20T08:44:54Z

I think we can adjust the issue to include wasm

whyoleg · 2024-11-22T09:02:53Z

Hey @osipxd I've rebased PR on latest changes (e.g EmbeddedServer.startSuspend). Do you think there is something else needed from me here?

I saw, that there is some progress to improve tests stability, but those changes are only in main currently. Are you planning to merge them into 3.1.0-eap anytime soon?

osipxd

Thank you! LGTM

As we decided to move investigation of the flaky test to a separate task, it would be great to ignore this test. However, I don't think it is possible to ignore a test only on one platform. I thought about adding annotations like @JsIgnore, @JvmIgnore, etc., but it feels out of the task scope. I think we'll merge tests stabilizing changes today, so we'll check if it helps.

osipxd self-requested a review November 8, 2024 17:41

e5l force-pushed the 3.1.0-eap branch from 1b289a7 to 020ac34 Compare November 13, 2024 13:14

osipxd requested changes Nov 13, 2024

View reviewed changes

whyoleg force-pushed the commonize-cio-server branch from c94816f to b0029a1 Compare November 13, 2024 18:05

e5l force-pushed the 3.1.0-eap branch from ce83e36 to ccb920f Compare November 14, 2024 10:25

whyoleg mentioned this pull request Nov 14, 2024

KTOR-7359 Implement a suspending version of EmbeddedServer.start and EmbeddedServer.stop #4481

Merged

osipxd force-pushed the 3.1.0-eap branch from 4016419 to 336817b Compare November 16, 2024 13:38

osipxd and others added 15 commits November 19, 2024 09:30

KTOR-7606 Make Closable implement AutoCloseable (ktorio#4414)

6ac932b

* Use stdlib `use` extension-function * Update type checks

KTOR-7586 Improve parsing of supported media types (ktorio#4410)

801c881

Co-authored-by: Osip Fatkullin <[email protected]>

Version 3.1.0-SNAPSHOT

75e9683

KTOR-7586 Fix MimesTest doesn't compile on JS target (ktorio#4438)

d860c68

KTOR-7663 Add bind overload for UDPSocketBuilder (ktorio#4456)

c464129

KTOR-7620 Make Url class @serializable and JVM Serializable (ktorio#4421

fdf9ecb

)

KTOR-6632 Support receiving multipart data with Ktor client (ktorio#4458

9a28ab5

) * KTOR-6632 Support receiving multipart data with Ktor client

Fix branch after rebase

6c8739a

KTOR-7435 Add serialization for SSE (ktorio#4363)

5ed28a5

Use loadServices in ClientLoader

7f0e2e6

Update dependency io.micrometer:micrometer-core to v1.14.1 (ktorio#4472)

0733c09

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

Use static libcurl build with conan (ktorio#4445)

0267de3

* Test libcurl build with conan * Take both curl and openssl from the same build * test no linux cache * Move K/N linux cache disabling to just `ktor-client-curl` module

KTOR-7644 Make re-auth status codes configurable (ktorio#4420)

344aa54

Some services use 403 instead of 401. Changing them might be impossible. With this change Ktor can flexibly work with any broken service. --------- Co-authored-by: Osip Fatkullin <[email protected]>

e5l force-pushed the 3.1.0-eap branch from 6e0eb10 to 344aa54 Compare November 19, 2024 08:30

e5l and others added 3 commits November 19, 2024 11:07

KTOR-7679 Allow disabling body decoding on server (ktorio#4444)

65816e9

* KTOR-7679 Allow disabling body decoding on server

KTOR-7359 Implement a suspending version of EmbeddedServer.start and …

ba4d001

…EmbeddedServer.stop (ktorio#4481) * Add startSuspend/stopSuspend in EmbeddedServer * Make EngineTestBase work on js/wasmJs

Make server-test-host depend on cio in common

377b37c

whyoleg added 7 commits November 20, 2024 08:06

Move WebSocketEngineSuite to common

53a4b4b

Expose test-base from test-suites to have access to it in server-cio …

b79d087

…tests

CIO server Js/WasmJs support

9fc2e82

WIP: fix CIO server tests for web

10997d7

fix style

88a71d0

Revert "WIP: fix CIO server tests for web"

8802744

This reverts commit 33c00f8.

move socket address util to nonJvm

d96a40b

whyoleg force-pushed the commonize-cio-server branch from b0029a1 to d96a40b Compare November 20, 2024 06:16

whyoleg requested a review from osipxd November 22, 2024 09:02

osipxd approved these changes Nov 22, 2024

View reviewed changes

osipxd force-pushed the 3.1.0-eap branch from de100ab to 3900250 Compare November 22, 2024 18:46

e5l force-pushed the 3.1.0-eap branch from 3900250 to d0ced88 Compare November 25, 2024 15:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support CIO server for wasm-js and js #4466

Support CIO server for wasm-js and js #4466

whyoleg commented Nov 8, 2024 •

edited

Loading

osipxd commented Nov 13, 2024 •

edited

Loading

osipxd left a comment •

edited

Loading

osipxd Nov 13, 2024

whyoleg Nov 13, 2024

whyoleg Nov 14, 2024

osipxd Nov 14, 2024 •

edited

Loading

e5l Nov 20, 2024

whyoleg Nov 22, 2024 •

edited

Loading

osipxd Nov 22, 2024

whyoleg commented Nov 13, 2024 •

edited

Loading

osipxd commented Nov 14, 2024

whyoleg commented Nov 14, 2024

e5l commented Nov 20, 2024

whyoleg commented Nov 22, 2024

osipxd left a comment

Support CIO server for wasm-js and js #4466

Are you sure you want to change the base?

Support CIO server for wasm-js and js #4466

Conversation

whyoleg commented Nov 8, 2024 • edited Loading

osipxd commented Nov 13, 2024 • edited Loading

osipxd left a comment • edited Loading

Choose a reason for hiding this comment

osipxd Nov 13, 2024

Choose a reason for hiding this comment

whyoleg Nov 13, 2024

Choose a reason for hiding this comment

whyoleg Nov 14, 2024

Choose a reason for hiding this comment

osipxd Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

e5l Nov 20, 2024

Choose a reason for hiding this comment

whyoleg Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

osipxd Nov 22, 2024

Choose a reason for hiding this comment

whyoleg commented Nov 13, 2024 • edited Loading

osipxd commented Nov 14, 2024

whyoleg commented Nov 14, 2024

e5l commented Nov 20, 2024

whyoleg commented Nov 22, 2024

osipxd left a comment

Choose a reason for hiding this comment

whyoleg commented Nov 8, 2024 •

edited

Loading

osipxd commented Nov 13, 2024 •

edited

Loading

osipxd left a comment •

edited

Loading

osipxd Nov 14, 2024 •

edited

Loading

whyoleg Nov 22, 2024 •

edited

Loading

whyoleg commented Nov 13, 2024 •

edited

Loading