reverseproxy: Cleaner close of websocket connections (and SSE)

[mholt]

Based on info from gorilla/websocket#316 (comment), we can probably be closing web sockets more cleanly.

[WeidiDeng]

I read reverse_proxy code and findout if response is 101, reverse_proxy will do a hijack and take it from there.. Per http.Server documentation, reverse_proxy should register a shutdown function. I know how to close websocket connection, but have no experience with sse or h2c.

Maybe reverse_proxy needs to keep track of hijacked connections and send a goaway frame to websocket at least. I don't know how other protocol does graceful shutdown. Protocol upgrade can be found in the upgrade header.

[francislavoie]

Yeah exactly, that's my same reading of it, we need to keep track of websocket connections in its own list and send close frames. I'm not sure how best to write the close frame logic though so we could use some help to get that right.

We can do this via Cleanup() on the reverse proxy module, which is called when the config is being shut down or reloaded.

I also think we should consider adding a grace timeout config in the reverse proxy module itself to close off other hijacked connections that are of unknown type.

Maybe for SSE we can just always close it right away because it should rarely be harmful to do so, but it could hurt if we close other kinds of streams where the frontend/backend of the app doesn't have logic to recover gracefully.

[deepto98]

Can I pick this up?

[francislavoie]

Sure, PRs always welcome!

[mholt]

Yeah, we'd love the assistance!

[mholt]

I've made progress on this to the point where I think I have it working. I was not experiencing any hanging config reloads before my changes, so if anyone is having that with WebSockets, I'd like to know. What I did experience, however, was that WebSocket connections were not closed on config reloads, which I think they should be.

So I have implemented that Caddy will send its own Close control messages when a config reload or shutdown is taking place. In my testing, this cleanly closes the connections with the client and the backend, whereas before the connections would have an abnormal close on exit. (It is a best-effort attempt though, I don't wait for a returned Close frame...)

As an aside, grace_period seemed irrelevant in my testing. Probably because http.Server.Shutdown() doesn't do anything with hijacked connections.

[mholt]

I've pushed my current efforts (which is the result of rewriting it about 3 times) to #4895. @WeidiDeng if you're interested, please feel free to try it out!

In my testing, the change successfully closes the connection that was left open before. It is closed between client and backend. For WebSocket connections specifically, the Close frame is now sent by Caddy to give the backend and the client and opportunity to close gracefully (code 1001 instead of 1006).

If interested parties could please try out my changes in staging and production, that would be great!