Avoid unsafely running effects when handling WS requests #2852
Conversation
| case Some(promise) => promise.unsafe.done(Exit.fail(cause)) | ||
| case None => () |
There was a problem hiding this comment.
I also noticed this bug where the promise wasn't being fulfilled with the exception as it returned an effect which wasn't then being run
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2852 +/- ##
==========================================
+ Coverage 64.51% 64.57% +0.05%
==========================================
Files 156 156
Lines 9085 9103 +18
Branches 1586 1590 +4
==========================================
+ Hits 5861 5878 +17
- Misses 3224 3225 +1 ☔ View full report in Codecov by Sentry. |
# Conflicts: # zio-http/jvm/src/main/scala/zio/http/netty/server/ServerInboundHandler.scala
| // IMPORTANT: Offering to the queue must be run synchronously to avoid messages being added in the wrong order | ||
| // Since the queue is unbounded, this will not block the event loop | ||
| // TODO: We need to come up with a design that doesn't involve running an effect to offer to the queue | ||
| zExec.unsafeRunSync(queue.offer(event.map(frameFromNetty))) | ||
| onComplete match { | ||
| case Some(promise) if close => promise.unsafe.done(Exit.succeed(ChannelState.Invalid)) | ||
| case _ => () | ||
| } |
There was a problem hiding this comment.
This is the most important change, we need to ensure that messages are offered to the queue synchronously to avoid them being added in the wrong order
When handling websocket connections, we currently impurely run the ZIO effect within a region that is already running in ZIO's executor. This can be quite bad as it can lead to a deadlock if the effect contains an async computation and there are no
ZSchedulerworkers available to execute the effect.This has already introduced some flakiness to Caliban's test suite (see here)This PR fixes it by changing the
upgradeToWebSocketmethod to return an effect instead, which is then composed with the main effect that is running the request.EDIT: After some further investigation, it appears that the flakiness issue experienced in Caliban's CI was not related to running the effect unsafely (although I kept that change because I think it's better to do it this way). The issue appears that it was related to the
dispatchmethod inWebSocketAppHandler. SinceNettyRuntime#runUninterruptibleforks the effect, in some rare cases messages would be offered to the queue in the wrong order.