-
Notifications
You must be signed in to change notification settings - Fork 427
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
Web Sockets for netty-loom #3675
Conversation
@adamw Can I get a pre-review? The implementation passes all our WS tests, has great latency in perf tests (a few ms on p99.999 on my machine, comparing to 10-20 for other backends). There's no documentation and little comments, probably some missing error handling? Would be great to get some comments 🙇 |
server/netty-server/loom/src/test/scala/sttp/tapir/server/netty/loom/NettyIdServerTest.scala
Outdated
Show resolved
Hide resolved
...netty-server/loom/src/main/scala/sttp/tapir/server/netty/loom/internal/ox/OxDispatcher.scala
Outdated
Show resolved
Hide resolved
.../loom/src/main/scala/sttp/tapir/server/netty/loom/internal/reactivestreams/OxProcessor.scala
Outdated
Show resolved
Hide resolved
...netty-server/loom/src/main/scala/sttp/tapir/server/netty/loom/internal/ox/OxDispatcher.scala
Outdated
Show resolved
Hide resolved
...c/main/scala/sttp/tapir/server/netty/loom/internal/reactivestreams/ChannelSubscription.scala
Show resolved
Hide resolved
.../loom/src/main/scala/sttp/tapir/server/netty/loom/internal/reactivestreams/OxProcessor.scala
Outdated
Show resolved
Hide resolved
.../loom/src/main/scala/sttp/tapir/server/netty/loom/internal/reactivestreams/OxProcessor.scala
Outdated
Show resolved
Hide resolved
...oom/src/main/scala/sttp/tapir/server/netty/loom/internal/ws/OxSourceWebSocketProcessor.scala
Outdated
Show resolved
Hide resolved
@adamw PR no longer in draft mode, re-requesting a review :) |
...c/main/scala/sttp/tapir/server/netty/loom/internal/reactivestreams/ChannelSubscription.scala
Outdated
Show resolved
Hide resolved
...c/main/scala/sttp/tapir/server/netty/loom/internal/reactivestreams/ChannelSubscription.scala
Outdated
Show resolved
Hide resolved
.../loom/src/main/scala/sttp/tapir/server/netty/loom/internal/reactivestreams/OxProcessor.scala
Outdated
Show resolved
Hide resolved
.../loom/src/main/scala/sttp/tapir/server/netty/loom/internal/reactivestreams/OxProcessor.scala
Outdated
Show resolved
Hide resolved
...tty-server/loom/src/main/scala/sttp/tapir/server/netty/loom/NettySyncServerInterpreter.scala
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nicely done :)
Btw. I presume the commented out tests pass as well?
I checked one of them separately already (failing pipe), and the other tests should be rather related to shared netty features (ping/pong stuff). However, I will merge them today (#3676) because I finally found the one causing flakiness, so this will give a final confirmation.\ Update: test merged, and pass |
Closes #3666
This PR adds support for Web Sockects in the
tapir-netty-loom
backend, using Ox for efficient and safe concurrency.From the user's perspective, creating a WS endpoint requires defining a pipe function of type
Ox ?=> Source[REQ] => Source[RESP]
, whereSource
isox.channels.Source
.For example:
The implementation has been tested with our performance tests and it's latency is really good comparing to other fast WS-supporting backends. The tests were run with all bells and whistles enabled (frame concatenation, very frequent auto ping - 40ms, auto pong on ping, etc).