custom close codes

[folospior]

This PR adds support for custom close codes. As defined in RFC 6455, APIs have the ability to specify their own close codes - in the 3000 range if registered with IANA, and in the 4000 if not registered.

The user of the library will have to parse these close reasons themselves in regards to their API's documentation.

In order to prevent integer overflow (we only have 2 bytes to work with), if a close code over 5000 is used or received, it will be treated like a 1000 close code. (Let me know if this should be changed, it was between this and NotProvided)

[folospior]

And maybe we should define some of the 3000 close codes, they're registered so they're defined here: https://www.iana.org/assignments/websocket/websocket.xhtml#close-code-number

Lmk, I could make a separate PR for it (or maybe add it in this one for only one version bump :)

[folospior]

@rawhat Could you take a look, please? :)

[rawhat]

Hmm... I wonder if I made a mistake just having this as a public type. I'm not super fond of the "fall back to normal error code" behavior... I wonder if it'd make more sense to make this opaque and have functions for the different types? Then the one for a custom code could return a Result(CloseReason, something). stratus could also expose functions that call into those in gramps 🤔

[folospior]

That seems sensible. Do you think this warrants a gramps/close_code module, so that calling those could be something like close_code.not_provided()?

[rawhat]

I'm kind of leaning towards just public functions in the websocket module. So you could do something like websocket.close_normal("Reason") or websocket.close()? Is that too general of a name? 🤔

[folospior]

Oh yeah, that would be cool. I think the close() function would be kind of vague - it could either mean Normal or NotProvided... I think close_normal (or close_normally) and close_no_reason would be better picks.

[folospior]

Btw, switching to a draft since it isn't production-ready really anymore

[rawhat]

Oh yeah, that would be cool. I think the close() function would be kind of vague - it could either mean Normal or NotProvided... I think close_normal (or close_normally) and close_no_reason would be better picks.

Makes sense to me :)

[folospior]

@rawhat One more thing. We discussed closing the connection ourselves with custom functions, but what about receiving close codes?

I assume we should make the on_close function be a fn(CloseReason, state), but then how would the stratus user pattern match on the close reason if the type is opaque?

Perhaps an opaque type ClientCloseReason and a transparent type ServerCloseReason?
That may have naming problems, but it seems like the best solution right now (we don't have to prefix the server reasons since client reasons are private)

[rawhat]

Hmm... currently I don't think the CloseFrame makes its way into the user handler in any capacity. Perhaps it should be passed into the builder.on_close if that's useful information... which unfortunately would be a breaking change.

So technically right now, I don't think that's a concern. But if it's something that needs to be supported at some point (which seems likely?), that is definitely worth thinking about.

I guess if it's documented that it falls back to Normal maybe that's fine... though I do think there are close frames that aren't intended / shouldn't be possible to be set by clients.

Difficult one 😬

[folospior]

Hi there, sorry for the ghosting, had a few difficult days.
I'm jumping on this PR right now, should be ready soon :)

[rawhat]

No rush :) I'm also making a number of changes in here, hopefully I don't step on your toes too much.

https://github.com/mtrudel/bandit/blob/619e53610370a9d2d16f95d5f82f7f4b0ec7fc7f/lib/bandit/websocket/connection.ex#L122 seems like bandit handles the automatic fallback as well, so maybe we should just do that?

Sorry for the confusion 😬