KTOR-9334 Fix unconfined dispatcher in handler context#5364
KTOR-9334 Fix unconfined dispatcher in handler context#5364bjhham merged 3 commits intorelease/3.xfrom
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
bjhham
force-pushed
the
bjhham/unconfined-user-context-netty-fix
branch
from
0217e48 to
6c10640
Compare
bjhham
force-pushed
the
bjhham/unconfined-user-context-netty-fix
branch
from
eca498d to
d530325
Compare
e5l
left a comment
e5l
left a comment
There was a problem hiding this comment.
please check the edge case before merging
| LOG.error("Failed to dispatch", result.exceptionOrNull()) | ||
| } catch (cause: Throwable) { | ||
| LOG.warn("Failed to dispatch; falling back to caller thread", cause) | ||
| block.run() |
There was a problem hiding this comment.
Retrying the execution block a second time can lead to unintended consequences (if there were no suspension points and the response/request was partially consumed/produced). Should we respond with an internal server error in this case and reject execution?
There was a problem hiding this comment.
I noticed that this was the approach taken in the Jetty engine, and it seems to solve a few problems with structured concurrency according to the newer sustainability tests. The main problem is that the netty executor quits before the application jobs join, so we need some kinda fallback for the shutdown sequence. If I recall correctly, the executor is shut down early because it's tied to accepting new requests. I'll try playing around with the fallback logic here to see if a secondary dispatcher would make more sense, or if we can avoid dropping the netty executor early.
There was a problem hiding this comment.
It just occurred to me that we can explicitly check if the executor is shutting down and delegate to the IO dispatcher in this case. I've updated the dispatcher to do this instead, and restored the previous behavior of logging the exception for more mysterious cases. This should avoid any blocking or problems with the shutdown sequence.
2 participants
Subsystem
Server, Netty
Motivation
KTOR-9334 Coroutines in route handlers are dispatched with Dispatchers.Unconfined since 3.2.0
Solution
Returned the netty dispatcher to the user context. This was removed in 3.2.0 to allow for some flexibility with overriding via the application's coroutine context, but when unassigned, this would lead to the engine's dispatcher taking over. The engine's dispatcher is unconfined, so you can get some unpredictability in the thread usage.
Addendum
I created a separate ticket to introduce thread affinity for Jetty, because it's always behaved this way and would require some more invasive surgery:
KTOR-9345 Jetty engine call context is unconfined