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

Add autoPing for web sockets (ember server). #7348

Open
wants to merge 10 commits into
base: series/0.23
Choose a base branch
from

Conversation

kamilkloch
Copy link

@kamilkloch kamilkloch commented Dec 25, 2023

autoPing flag is added to WebSocketBuilder2 (auto-ping should be arguably configurable per web socket connection).

Hovewer, actual ping is not materialized in WebSocketBuilder2.build, for performance reasons: concurrent merging of 2 fs2 streams (business logic and pings) is much more expensive than writing the ping frames directly into the socket.

WebSocketBuilder2.build provides a default (albeit less performant) implementation of auto-ping. Servers have the ability to override this implementation with a more performant direct write into the socket.

Closes #7347.

Copy link
Member

@armanbilge armanbilge left a comment

Choose a reason for hiding this comment

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

Hovewer, actual ping is not materialized in WebSocketBuilder2.build, for performance reasons:

Hmm, is this a compatibility issue: if we release this feature without a default implementation, what happens to all the servers that have not implemented yet?

If possible, we should implement the feature directly in WebSocketBuilder2.build. If there is a measurable difference in performance, then we can override it in servers implementations with a more efficient implementation.

@kamilkloch
Copy link
Author

kamilkloch commented Dec 26, 2023

Hovewer, actual ping is not materialized in WebSocketBuilder2.build, for performance reasons:

Hmm, is this a compatibility issue: if we release this feature without a default implementation, what happens to all the servers that have not implemented yet?

If possible, we should implement the feature directly in WebSocketBuilder2.build. If there is a measurable difference in performance, then we can override it in servers implementations with a more efficient implementation.

As for performace difference:

val n = 10_000
val m = 100
val s = Stream(1).covary[IO].repeatN(n)
val s2 = s.mergeHaltL(Stream.empty)

s.compile.last.replicateA_(m).timed.map(_._1.toMillis).flatMap(IO.println) >>
s2.compile.last.replicateA_(m).timed.map(_._1.toMillis).flatMap(IO.println)
130
105394

Also, please take a look at this PR , where replacing mergeHaltL with a manual implementation from typelevel/fs2#3363 (BTW, ping! :) ) significantly reduced latency and reduced CPU pressure 5x: softwaremill/tapir#3393

Baking a default implementation of auto-ping into WebSocketBuilder2.build might be tricky - once we include the frames in receiveSend, there is no way back. It has to be done in some other way.

@kamilkloch
Copy link
Author

@armanbilge please take a look: a default auto-ping implementation in WebSocketBuilder2, which can be overriden by the servers. WebSocketBuilder2 now needs Temporal, which breaks bincompat.

@armanbilge
Copy link
Member

WebSocketBuilder2 now needs Temporal, which breaks bincompat.

What if we collect the Temporal[F] on the withAutoPing method? Then we don't need to break bincompat.

@kamilkloch
Copy link
Author

kamilkloch commented Dec 27, 2023

WebSocketBuilder2 now needs Temporal, which breaks bincompat.

What if we collect the Temporal[F] on the withAutoPing method? Then we don't need to break bincompat.

How to do a natural transformation Temporal[F] ~> Temporal[G]? (needed in WebSocketBuilder2#imapK)

EDIT: done.

@kamilkloch
Copy link
Author

@armanbilge What do you think?

@armanbilge armanbilge self-requested a review February 21, 2024 23:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add optional autoPing for web sockets
2 participants