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

YARP should support gracefully closing WebSocket connections to the backend during shutdown #1756

Open
halter73 opened this issue Jun 6, 2022 · 1 comment
Labels
Type: Idea This issue is a high-level idea for discussion.
Milestone

Comments

@halter73
Copy link
Member

halter73 commented Jun 6, 2022

What should we add or change to make your life better?

When YARP shuts down while proxying a WebSocket to the backend, the backend has no way to observer that the proxy wants to gracefully close the connection. This means that YARP will always hit the Host's ShutdownTimeout which was recently increased to 30 seconds before exiting. Because HttpForwarder proxies WebSockets using IHttpUpgradeFeature rather than IHttpWebSocketFeature, it has no mechanism to notify the backend that it wants to try gracefully closing the connection. If it used the WebSocket feature instead, it could initiate a WebSocket closing handshake which should speed up YARP shutdown and restarts dramatically in this situation.

Parsing and redoing the WebSocket framing will have some minor performance implications and injecting a close handshake into a proxied WebSocket could be behavior people don't want, so this should probably be made opt-in. Although, I do think the fast majority of customers would prefer this behavior.

Related: I filed dotnet/aspnetcore#42057 so Kestrel exposes HTTP/2 and HTTP/3 GOAWAY frames to apps on the back which should also help the proxy close long running HTTP/2 and HTTP/3 requests to Kestrel backends. This is a little safer though because YARP can continue sending data to the backend after sending a GOAWAY. This is not possible after starting the WebSocket closing handshake.

@halter73 halter73 added the Type: Idea This issue is a high-level idea for discussion. label Jun 6, 2022
@karelz
Copy link
Member

karelz commented Jun 7, 2022

Triage: Unclear if we want to go this route.
We do it in Telemetry and the overhead there is very small -- we should be able to use the same pattern here if we decide to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Idea This issue is a high-level idea for discussion.
Projects
None yet
Development

No branches or pull requests

2 participants