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

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 · 2024-11-26T17:06:47Z

Hey @whyoleg, we've merged commits fixing tests flakiness to 3.1.0-eap branch. Could you rebase this PR one more time?

…tests

This reverts commit 33c00f8.

whyoleg · 2024-11-26T17:41:12Z

Sure, @osipxd, rebased!

whyoleg · 2024-11-26T17:48:25Z

@osipxd: TC builds failed because of Canceled by Osip Fatkullin: Wrong agent requirements - is it something expected ? :)

osipxd · 2024-11-26T18:16:31Z

is it something expected ? :)

Yes, it was because of me broken CI exactly at that moment. Should be fixed now. Restarted the build chain

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

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

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 2 times, most recently from d0ced88 to 1cc5abf Compare November 26, 2024 11:55

whyoleg added 8 commits November 26, 2024 19:29

Make server-test-host depend on cio in common

fe6f98a

Move WebSocketEngineSuite to common

2e6a272

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

e1b0647

…tests

CIO server Js/WasmJs support

1e1a6a2

WIP: fix CIO server tests for web

42b6baa

fix style

dc21f66

Revert "WIP: fix CIO server tests for web"

59e3d83

This reverts commit 33c00f8.

move socket address util to nonJvm

339695c

whyoleg force-pushed the commonize-cio-server branch from d96a40b to 339695c Compare November 26, 2024 17:40

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

osipxd commented Nov 26, 2024

whyoleg commented Nov 26, 2024

whyoleg commented Nov 26, 2024

osipxd commented Nov 26, 2024

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

osipxd commented Nov 26, 2024

whyoleg commented Nov 26, 2024

whyoleg commented Nov 26, 2024

osipxd commented Nov 26, 2024

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