Add Grace to gracefully close WebSocket connections

[nhooyr]

Closes #199

[nhooyr]

cc @coadler

[nhooyr]

cc @ammario would appreciate your thoughts as well.

[FZambia]

@nhooyr hello, just my thoughts on this.

Are you sure this should be inside a library core? This pr includes a lot of implementation specifics (I can imagine one including myself to add bounded semaphore for shutdown process, use custom close reason etc). Also in my app I already have a hub that keeps all connections (I still using Gorilla WebSocket in my apps though).

It seems that the same is possible to do from outside the core package. Of course it will still be possible to do from outside even if you merge this as Grace is optional. Or you can make Grace configurable enough to fit all needs. But at least in this case you will always keep in mind that library is flexible enough to allow custom shutdown process from outside the core.

I think I mostly worry about using context to pass Grace instance to automatically register connections. Maybe this should live in separate package and users should register connections manually in Grace helper after successful accept?

[nhooyr]

Appreciate the feedback and thoughts @FZambia ❤️

I can imagine one including myself to add bounded semaphore for shutdown process etc).

Could you elaborate on how a bounded semaphore would be used here?

use custom close reason

Considered that but I couldn't think of a good reason for another close reason that would be useful.

Also in my app I already have a hub that keeps all connections (I still using Gorilla WebSocket in my apps though).

Exactly, everyone does that. I've written Grace probably 5x in my own servers and in the tests for this library because net/http doesn't have support for shutting down hijacked connections easily.

As it's something net/http provides for normal connections, I felt it was appropriate to include in this package as a core feature as well.

It seems that the same is possible to do from outside the core package. Of course it will still be possible to do from outside even if you merge this as Grace is optional. Or you can make Grace configurable enough to fit all needs. But at least in this case you will always keep in mind that library is flexible enough to allow custom shutdown process from outside the core.

The library will always be flexible enough to allow custom shutdown. Grace is optional but will cover 99.9% of user use cases for shutting down WebSockets gracefully.

I think I mostly worry about using context to pass Grace instance to automatically register connections. Maybe this should live in separate package and users should register connections manually in Grace helper after successful accept?

I agree, it is definitely not ideal and I did consider that but I like that users don't have to think about adding and removing connections from Grace, the automatic nature makes its very ergonomic and easy to use. Furthermore, this enables Accept to reject WebSocket handshakes entirely with http.StatusServiceUnavailable versus accepting the WebSocket and then closing it when adding to grace.

[nhooyr]

Ah nvm I see what you mean, you might only want to shutdown connections that are idle respective to your WebSocket protocol, i.e not in a game yet.

[FZambia]

I see what you mean, you might only want to shutdown connections that are idle respective to your WebSocket protocol, i.e not in a game yet.

Not sure I understand what you mean saying this.

Could you elaborate on how a bounded semaphore would be used here?

I mean that in case of many connections you may want to limit concurrency when gracefully closing lots of connections on server shutdown as it involves sending close frame to each client and can overhelm server a bit.

Considered that but I couldn't think of a good reason for another close reason that would be useful.

In my case I use custom close reasons to pass reconnect advice which clients can parse and handle.

Furthermore, this enables Accept to reject WebSocket handshakes entirely with http.StatusServiceUnavailable versus accepting the WebSocket and then closing it when adding to grace.

I supposed that this could also be handled on user level - i.e. do not Accept if shutdown in process. Yes, this is more work to do for users but I believe that users that care about proper server shutdown can easily elaborate how to achieve the same with helper middleware.

I like that users don't have to think about adding and removing connections from Grace, the automatic nature makes its very ergonomic and easy to use.

Maybe this is true, in general I am fine with any solution that does not prevent making the same from outside and does not introduce a lot of overhead. Hopefully you will get more opinions from contributors to decide.

[nhooyr]

Not sure I understand what you mean saying this.

From the perspective of the library, any connection alive is not idle. However, you might consider a connection idle if it isn't actively being used. Like for a game server, the user is connected but in a game at the moment. At which point it'd be safe to close the connection.

However, I realized this is orthogonal to Grace and can be implemented by the application as it doesn't require tracking all connections, just setting some sort of signal that shutdown is in progress.

I mean that in case of many connections you may want to limit concurrency when gracefully closing lots of connections on server shutdown as it involves sending close frame to each client and can overhelm server a bit.

Did not consider that but I noticed net/http doesn't bother with it for http2. http2 has a similar close process where a goaway frame must be sent.

In my case I use custom close reasons to pass reconnect advice which clients can parse and handle.

Interesting, why send reconnect advice vs hard coding an exponential reconnect?

I supposed that this could also be handled on user level - i.e. do not Accept if shutdown in process. Yes, this is more work to do for users but I believe that users that care about proper server shutdown can easily elaborate how to achieve the same with helper middleware.

Maybe this is true, in general I am fine with any solution that does not prevent making the same from outside and does not introduce a lot of overhead. Hopefully you will get more opinions from contributors to decide.

Yea, I agree it's a little janky but I feel like it's the best compromise for DX. Will wait to see what other people's thoughts are of course.

[FZambia]

Interesting, why send reconnect advice vs hard coding an exponential reconnect?

Advice can contain a flag to not reconnect at all, it's not very useful for server shutdown but helps in other situations - but existing protocol contract is the same for all codes/reasons so I prefer to follow it even for shutdown process. Though clients are smart enough to reconnect with exponential backoff if they received unknown close code or malformed reason.

[nhooyr]

Going to merge for now just cause I'm using it internally in tests and whatnot and don't want to have to deal with diverging changes.

Will not include in a release until further feedback though, might just make it internal for now.

[nhooyr]

@coadler brought up a great point. If I were to change the API to manually register connections, then it'd work client side too.

[nhooyr]

Another problem is it doesn't wait for the individual goroutines but just for the connection to close.

I've got a much better idea in the works. Update soon.