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

WebSockets for netty-cats #3628

Merged
merged 31 commits into from
Apr 3, 2024
Merged

WebSockets for netty-cats #3628

merged 31 commits into from
Apr 3, 2024

Conversation

kciesielski
Copy link
Member

@kciesielski kciesielski commented Mar 20, 2024

This PR adds support for Web Socket endpoints in tapir-netty-cats, with all the sub-features like autoPongOnPing, ignorePong, decodeCloseRequests, decodeCloseResponses, and autoPing.

Some remarks:

  • The logic for handling control frames (Close, pings/pongs) happens mostly in common tapir-netty code and should work for other Netty-based backends.
  • The Cats Effect - specific part is Fs2StreamCompatible, and WebSocketPipeProcessor. Especially the latter, which creates a Reactive Streams Processor[Req, Resp] from user-provided fs2.Pipe[F, Req, Resp].
  • The WebSocketPipeProcessor has a workaround to properly handle exceptions thrown in the pipeline. It needs to prevent underlying cancelation mechanisms and instead signal back an error to the Netty channel handler, so that the server properly sends a Close frame. Otherwise the channel closes silently, which also happens in some other servers, like Vert.X, so tests for error handling run only when failingPipe is set to true in ServerWebSocketTests.
  • I also added tests for ping/pong handling, which are selectively disabled for some interpreters which don't support them.

In performance tests, latency is on par with our fastest backends (like http4s):

image

@kciesielski kciesielski marked this pull request as ready for review March 27, 2024 08:12
@kciesielski kciesielski requested review from adamw and ghik March 27, 2024 08:12
@kciesielski kciesielski changed the title [WIP] WebSockets for netty-cats WebSockets for netty-cats Mar 27, 2024
@kciesielski kciesielski requested review from adamw and ghik March 28, 2024 16:01
@kciesielski
Copy link
Member Author

@adamw Adding proper handling for non-ws responses in ws endpoints made me think that it's better to remove ReactiveWebSocketHandler entirely. It would realize a majority of logic done by the standard NettyServerHandler plus handshake handling. Instead, I put handshake handling as an additional case in NettyServerHandler. Maybe it's worth extracting the body of initWsPipeline to a function in an object located in the ws package, but I'm not sure about that. WDYT?

@kciesielski kciesielski requested a review from adamw March 29, 2024 14:34
@adamw
Copy link
Member

adamw commented Apr 2, 2024

@kciesielski yeah if that's feasible, sounds good, might clean up the code :) Not sure about extracting the code to a WS package, you would loose the possibility to call the 500/400 response-generators then? But maybe these should be simply put in yet another "common" object, NettyErrorResponses or such

@kciesielski
Copy link
Member Author

Hmm such extractions require further refactoring, because response handlers are internal implicit classes referencing fields of NettyServerHandler (like request, isShuttingDown, etc.).

@adamw
Copy link
Member

adamw commented Apr 2, 2024

@kciesielski let's skip them then ;)

@adamw
Copy link
Member

adamw commented Apr 2, 2024

otherwise this PR will end up being gigantic feature + refactor

@kciesielski
Copy link
Member Author

kciesielski commented Apr 2, 2024

I'm totally on the same page on this 😁

@ghik
Copy link
Contributor

ghik commented Apr 2, 2024

I'm reading through the codebase to get a better mental model of this, but since I'm generally not familiar with it (nor with websockets in detail, for that matter), it might take me some time until I can submit some non-superficial review. Not sure if there's any point in waiting for me :)

@kciesielski
Copy link
Member Author

Thank @ghik, your comments were helpful :)
@adamw waiting for your approval then.

@adamw adamw merged commit 79239eb into master Apr 3, 2024
28 checks passed
@adamw adamw deleted the netty-ws branch April 3, 2024 08:04
@adamw
Copy link
Member

adamw commented Apr 3, 2024

I think in the end code is even simpler now. Great work - let's ship it :)

@kciesielski kciesielski linked an issue Apr 5, 2024 that may be closed by this pull request
@kciesielski kciesielski added this to the Netty / ease of use milestone Apr 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Netty websockets for Cats Effect
3 participants