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

Gracefully shutting down WebSocket connections on shutdown #2622

Open
samsp-msft opened this issue Oct 4, 2024 · 6 comments
Open

Gracefully shutting down WebSocket connections on shutdown #2622

samsp-msft opened this issue Oct 4, 2024 · 6 comments
Assignees
Labels
Type: Enhancement New feature or request
Milestone

Comments

@samsp-msft
Copy link
Contributor

A request came in from a partner team, that they want YARP to gracefully drain and shutdown connections when the process gets a SIGTERM from k8s.
I think we drain existing inbound connections and not allow new ones to connect, but I don't know what we do upstream/downstream, especially with websocket and http2 connections?

@samsp-msft samsp-msft added the Type: Bug Something isn't working label Oct 4, 2024
@samsp-msft
Copy link
Contributor Author

@timmydo for FYI.

@MihaZupan MihaZupan added Type: Enhancement New feature or request and removed Type: Bug Something isn't working labels Oct 7, 2024
@timmydo
Copy link

timmydo commented Oct 8, 2024

For context, we were seeing traffic drops/spikes/errors during rollouts because connections were not being drained to other instances.

I don't know if this is more for YARP or for Kestrel itself. As a temporary workaround, we have added logic like this to our YARP instance:

        public HttpResponseMessage TransformResponseMessage(HttpContext httpContext, HttpResponseMessage httpResponseMessage)
        {
            if (this.applicationLifetime.ApplicationStopping.IsCancellationRequested)
            {
                httpContext.Connection.RequestClose();

                if (HttpProtocol.IsHttp11(httpContext.Request.Protocol))
                {
                    httpContext.Response.Headers.Connection = "close";
                }
            }
            [...]
         }

I think this logic might make more sense at the Kestrel level. Generally speaking, we would want a web server to tell requests on existing connections to create a new connection so the load balancer would send it to a new instance.

The other point touched on by Sam was web sockets. Is there a way we could signal to YARP on ApplicationStopping that it tries to shutdown the existing websocket connections as gracefully as possible?

Thanks,

@samsp-msft
Copy link
Contributor Author

The other point touched on by Sam was web sockets. Is there a way we could signal to YARP on ApplicationStopping that it tries to shutdown the existing websocket connections as gracefully as possible?

@MihaZupan ??

@MihaZupan
Copy link
Member

The functionality doesn't exist in YARP yet, but it's possible to add if there's demand.

YARP currently treats WebSocket streams as opaque data streams, we just copy bytes back and forth.
Depending on how graceful you want to be, you could either close the underlying transport or try to parse the WebSocket stream to inject a close frame.

The second involves a bit more overhead to intercept the stream to keep track of frames, but it's relatively cheap, and YARP already has most of the logic for it in the WS telemetry implementation.

@samsp-msft
Copy link
Contributor Author

@timmydo is this a formal ask for websockets closure?

@timmydo
Copy link

timmydo commented Oct 10, 2024

Yes please. Currently we have some custom code trying to track the HttpContext objects using web sockets. On shutdown we try to abort them all but we would prefer a softer close.

@MihaZupan MihaZupan added this to the YARP 2.3 milestone Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants