websockets #72

bgwdotdev · 2024-05-11T23:34:19Z

Here's a first pass, scrappy implementation of mists websockets into wisp. #10

Still figuring out if there's a better way to do this and discussing with rawhat a little on approaches.

This currently works though. Feedback on approach most welcome.

Option was only added due to the test stuff, not sure how do deal with that yet but maybe that's solving the wrong problem.

lpil

Thank you! This couples Wisp very tightly to Mist, which it carefully does not do so far. We'll need to decouple it, I've left some notes inline.

src/wisp.gleam

bgwdotdev · 2024-05-26T22:05:13Z

I've now created a pure wisp public api for this which mostly wraps the mist types.

Docs (and likely type names/api) still need work but wanted to verify if the approach is more suitable for wisp before attempting to polish things.

Hopefully the Socket type would allow wrapping various connection types that may be required by potentially different backends as appropriate?

Let me know if I'm on the right track or missing a beat.

Thanks :)

lpil

Nice!! I like how this is coming along.

There's still some dependency of Mist that needs to be removed. Soon the Mist specific code will be moved to another package, so there can be no dependency here.

Another problem to solve is how do we make it so a Wisp application can only upgrade to a websocket connection if the webserver being used supports it? For example, Elli and CGI don't support websockets, so the programmer must not have the ability to construct a websocket response in those cases. Perhaps it could be that there's some secondary capability value that gets passed in by websocket capable servers, and that is need to upgrade within the application. Whatever the solution the same pattern will need to be usable for server sent events too.

Another thing is how do we write tests for websocket using applications too.

Is there any other problems to solve or other considerations?

src/wisp.gleam

bgwdotdev · 2024-06-18T02:13:12Z

Okay. Had a big stab at moving all the mist stuff into a separate module.

I've fully removed the import mist from the main wisp.gleam file.

Websockets are also now implemented with a reference example in the test file.

I've had to make a couple things in the wisp.gleam file public that I'm not sure we want to keep that way (mostly around make_connection).

Everything needs a tidy, documentation and the test cases fleshed out but hopefully it's looking more like how we want it to be ^^?

Definitely not finished working on it (want to try dump mist from internal somehow) but keen to take any feedback generally at this stage!

gleam.toml

bgwdotdev · 2024-06-25T23:14:50Z

Okay! I think I'm feeling happy with the general api design and split of things now and was thinking of moving on to docs.

I was thinking of creating a websocket error type within wisp as well for when wisp_mist.send fails, though looking at the errors provided from mist via glisten, I'm thinking we might want to just cover important ones like Closed and Timeout maybe and then the others can fall under some more generic socket error that left for the user to handle? (here's the variants for reference https://hexdocs.pm/glisten/3.0.0/glisten/socket.html#SocketReason)

bgwdotdev · 2024-06-26T21:20:15Z

left to do:

update examples to use wisp_mist.handler
write example using websocket

done?

lpil

Nice! I've left a handful of notes inline.

It's getting a bit hard to follow this PR. If we want to have the Mist code split out into a new module I think it would be good to have that in a first PR and then the websocket stuff in a second. One change-per-PR makes it a lot easier to review and merge

gleam.toml

src/wisp.gleam

lpil · 2024-07-13T11:21:00Z

src/wisp/internal.gleam

@@ -0,0 +1,84 @@
+import gleam/bit_array


Delete this module please

This is currently required: https://github.com/gleam-wisp/wisp/pull/72/files/d4d8770e86202b381bfecc431de039fa513a6f15#diff-67b4a7a613b4f6cd4f38f925c25569c25c072d354d2ce4768e111b6efffb8871R82

src/wisp/wisp_mist.gleam

test/mist_test.gleam

src/wisp.gleam

lpil · 2024-07-13T11:30:34Z

src/wisp.gleam

+// Websockets
+//
+
+/// The messages possible to receive to and from a websocket handler.


Don't these only come from the client?

No, the WsCustom type is called when messages are sent to the optional subject/selector created during the websocket handler on_init. This is how you generally send messages from within your application to a connected websocket.

For example, your server might have a broadcast_clients function that sends a message to all websocket handlers subject, which it will receive on WsCustom, and then maybe you forward that message onto the connected websocket(s) themselves! If that makes sense?

Sorry, I mean this says the handler sends these, but it receives them

Oh right, I see what you mean, of course! Corrected this now :)

lpil · 2024-07-13T11:32:00Z

src/wisp/wisp_mist.gleam

+/// }
+/// ```
+pub fn handler(
+  handler: fn(wisp.Request, wisp.Ws(mist.Connection)) -> wisp.Response,


Mist specific types leak into the application here. A Wisp websocket application should be able to be run on any web socket capable web server without modification, but in this case the type would need to be changed.

I can wrap the mist.Connection in a wisp_mist.Connection type to hide the mist implementation but I don't think it's possible to remove this altogether as this holds the http socket information which, when a request is made to a websocket, it uses to upgrade the socket to a websocket and then hold onto it to keep the connection alive (to my understanding).

This socket connection implementation will likely be unique per websocket server's approach to managing the connection.

Unless I'm missing something, there's no way to hide this without at least wisp/internal depending on mist/whatever-other-server.

My thought was if you need to swap wisp_mist for whatever, this would involve changing.

wisp.Ws(wisp_mist.Connection) => wisp.Ws(other_server.Connection)

or maybe if they were imported with an alias, it wouldn't change the api :?

import wisp_mist as server => import other_server as server wisp.Ws(server.Connection)

This is still specific server implementations leaking into Wisp core, making Wisp applications no longer web server agnostic. It's a hard requirement for the same application to be deployable on different web servers with the same capabilities.

Ok, no worries! I will see what I can figure out :)!

Latest changes now stop these types from leaking but I can't find a way to do it without the handler function at least returning a function which takes in the required functions to start a websocket actor:

fn(wisp.Request, wisp.WebsocketHandler(a,b)) -> wisp.Response

The problem with this design is you can then only have one solid type for your applications websockets that you need to specific in your handler function.

Spent a lot of time on this and can't seem to find a way around it. Might just need to sleep on it but if you have any ideas, it'd be very welcome ^^.

Any other option I think of just seem to lead to mist leaks :<

outside of using coerce, which works but obvious is theoretically unsafe

lpil · 2024-07-13T11:32:53Z

src/wisp.gleam

+type WebsocketConnection(c) =
+  internal.WebsocketConnection(c)
+
+/// For web socket capable servers to connect to clients


I don't understand what this is from the documentation, could it be expanded please 🙏

The name isn't very clear either. It doesn't appear to be a websocket?

hopefully makes more sense now ^^? (I do think these need renamed but not figured good names out yet)

test/mist_test.gleam

bgwdotdev · 2024-07-14T13:08:51Z

Thanks for the feedback+review, been busy with work so not been able to progress it, off next week though so will try push this along to finish line and address all your notes :)! Much appreciated.

bgwdotdev · 2024-07-23T22:54:15Z

the mist module split only has now been created as another pr ahead of the work here if we want to tackle that bit first :)

refactor: update types and add otp explicity feat: remove mist from all public types feat!: move mist func to seperate module, remove direct mist dep, add WS tidy: remove dead code feat: remove socket from wisp side feat: remove last mist dependency from wisp api feat: change send to use a wisp type as the interface

note: the client doesn't seem to return any responses for some reason when running under gleeunit

… to be hidden from docs

bgwdotdev · 2024-07-25T14:22:36Z

just as a note, rebase off main now so diff should be easier

lpil

Thank you! I've left a series of questions

lpil · 2024-08-24T12:14:19Z

examples/12-websocket-chatroom/src/app/websocket.gleam

@@ -0,0 +1,92 @@
+import app/chatroom


Remove this example please. It works but it's not the best way to implement this and has some failure cases which can leak memory or otherwise go wrong, and I wouldn't want people to copy it rather than studying OTP to figure out a more reliable way.

Good point, makes sense, deleted this. Perhaps worth us revisiting at a later date to make a quality example as seems lots of folks look for a reference on this type of thing :)

lpil · 2024-08-24T12:22:17Z

src/wisp.gleam

+// Websockets
+//
+
+/// The messages possible to receive to and from a websocket handler.


Sorry, I mean this says the handler sends these, but it receives them

lpil · 2024-08-24T12:22:34Z

src/wisp.gleam

+  /// Upgrades the socket to a websocket, is only used internally by the
+  /// web server and should not be called directly.
+  ///
+  Websocket(process.Selector(process.ProcessDown))


Why does it contain process.Selector(process.ProcessDown)?

Honestly, I'm not sure -- I don't really understand the underlying otp stuff going on here ^^; I'll try to look into it to understand it more but all I know is that if you remove it, the websocket just automatically closes after the first message

What do you mean? Types don't change runtime behaviour like that.

Hm, not sure, could be bad memory on my part, I'll give it another test and see :)

src/wisp.gleam

lpil · 2024-08-24T12:24:25Z

src/wisp.gleam

+/// This type will need to be passed to your webserver of choice websocket
+/// function, such as `wisp_mist.websocket`.
+///
+pub type WebsocketHandler(state, msg) {


In some places its Ws, some it's Websocket. Which do we want? Normally we don't abbreviate in Gleam, but on the other hand this word would be used a lot.

Apologies, I've now standardised this to WsXyz format. I'm happy to go either way on this though. I feel WebsocketXyz get really cumbersome though that may just be because I've been writing a lot of variations of it while trying to figure out an api hah.

I would entirely defer to your preference on this and happy to change as appropriate. The only other though I has was moving the websocket stuff into a wisp/websocket.gleam module and perhaps removing Ws/Websocket entirely from all the names?

src/wisp.gleam

lpil · 2024-08-24T12:30:40Z

src/wisp.gleam

+/// active websocket (`WebsocketConnection`).
+///
+pub type WsCap(state, msg) {
+  WsCap(fn(Request, WebsocketHandler(state, msg)) -> Response)


What's this inner value for? I couldn't see it used anywhere

Apologies, the examples had fallen out of line with the new api, it was present in the test but I've actually added a new wisp.websocket function which I think better shows how this is used. I've now updated all the test/examples to use this

src/wisp.gleam

bgwdotdev · 2024-08-26T14:56:23Z

Ok, I think I've updated everything! docs, examples, tests hopefully all covered :)

Assuming there was no further changes required, the only issue left I think would be how we want to do errors. I'm not happy with the error handling I've added but also not sure we want to copy all the errors available in the mist implementation. If you have an idea on how you'd prefer this was present to the user, happy to follow that :)

otherwise if just translating over all the glisten socket errors makes sense then happy to do that as well:
https://hexdocs.pm/glisten/3.0.0/glisten/socket.html#SocketReason

lpil · 2024-09-12T16:08:06Z

src/wisp.gleam

+  /// Upgrades the socket to a websocket, is only used internally by the
+  /// web server and should not be called directly.
+  ///
+  Websocket(process.Selector(process.ProcessDown))


What do you mean? Types don't change runtime behaviour like that.

lpil · 2024-09-12T16:08:26Z

src/wisp.gleam

+/// send data to the client via `WsSendText` or `WsSendBinary`
+///
+type WsConnection =
+  fn(WsSend) -> Result(Nil, WsError)


Why is a connection a function?

This was changed into a callback to avoid leaking the mist websocket connection type or creating a mist specific implementation of send due to the types required.

To avoid this it is we return a callback from our mist on_init wrapper that has the connection hidden away inside it, so you just pass your data to the WsConnection type and it takes care of attaching the mist (or otherwise) connection type, on your behalf.

Maybe another name would make more sense? I'm not able to see another api design to get around this problem if we don't want it to be a function unfortunately

lpil · 2024-09-12T16:08:49Z

src/wisp.gleam

+/// function. It is required to turn a http connection into an
+/// active websocket (`WsConnection`).
+///
+pub type WsCapability(state, msg) {


Should the internals of this be public? How would this dissuade users from constructing it directly?

Why does the capability have type parameters and contain the handler? Do you want to restrict the websocket processes to have specific messages and state for other implementations?

Should the internals of this be public?

Due to a circular import issue, I was unable to put this into internal (as then internal would depend on request and response from wisp.gleam and wisp.gleam would depend on internal.gleam to export it as an opaque type) but it is required to be construct-able by mist_wisp or other server implementations -- I'll look into seeing if I can create another internal module and have it act as a middle man or diamond configuration or whatever the correct term for that is ^^;

How would this dissuade users from constructing it directly?

In the current implementation, to construct it you need to import internal which I was hoping would be enough to dissuade folks.

Why does the capability have type parameters and contain the handler? Do you want to restrict the websocket processes to have specific messages and state for other implementations?

This ending up being an unfortunate side-effect of being the only way I could find to remove any mist types leaking due to the need to store the mist connection info for the socket. This way we can essentially hide the mapping of wisp.Request -> mist.Connection -> wisp.Response but it means the capability needs to return a callback which takes in the single extra arg of the websocket handler.

I actually don't really want it to work like this at all as it causes awkwardness if you want to use more than one websocket handler in your wisp server at once but I'm unable to see any other approach to dealing with this in the wisp_mist.handler

lpil · 2024-09-12T16:09:48Z

src/wisp.gleam

+  WsSendBinary(binary: BitArray)
+}
+
+/// TODO: Placeholder error type, flesh out.


Is this done?

from previous message for your consideration (as I feel like you wouldn't want to copy all those socket errors but I'm not sure, if you have a quick ye/naa confirmation that works for me)

the only issue left I think would be how we want to do errors. I'm not happy with the error handling I've added but also not sure we want to copy all the errors available in the mist implementation. If you have an idea on how you'd prefer this was present to the user, happy to follow that :)

otherwise if just translating over all the glisten socket errors makes sense then happy to do that as well:
https://hexdocs.pm/glisten/3.0.0/glisten/socket.html#SocketReason

lpil · 2024-09-12T16:10:14Z

src/wisp.gleam

+  WsErrClosed
+  WsErrTimeout
+  WsErrTerminated
+  WsErrOther(String)


No abbreviations please. Put Error at the end of the names.

What's Other for? Document these please.

No abbreviations please. Put Error at the end of the names.

no bother, will do

What's Other for? Document these please.

for above noted discussion on error approach, will adjust accordingly

lpil · 2024-09-12T16:11:38Z

src/wisp/wisp_mist.gleam

 pub fn handler(
-  handler: fn(wisp.Request) -> wisp.Response,
+  handler: fn(wisp.Request, wisp.WsCapability(state, msg)) -> wisp.Response,


No breaking changes to this API please. We said we'd have a new function here.

Oh, apologies, I had it in my backwards in my head for whatever reason, will fix

lpil · 2024-09-12T16:13:17Z

src/wisp/internal.gleam

+/// A type used purely to block the user constructing a `wisp.WsCapability`
+/// via the public api as it should only be constructed by the web server if it
+/// supports websockets.
+pub type WsCapability {


This should be opaque

As noted earlier due to issues of circular import and needing to construct this in wisp_mist.gleam it is not opaque, will see if I can shuffle the module around to allow this

lpil · 2024-09-12T16:14:08Z

src/wisp.gleam

+/// "pong" |> wisp.WsSendText |> conn()
+/// ```
+///
+pub type WsSend {


Is there a reason not to always sent bit arrays?

No reason I'm aware of, will give it a test and see :)

lpil · 2024-10-09T12:30:33Z

Have just been giving this a try. How does one create more than one webhook handler? The type parameter means they have to all have the same types, so I can't create a second one for another route.

bgwdotdev · 2024-10-10T21:26:31Z

Yeah, this is exactly the big problem and sticking point I can't find away around in this ( or any I can think of :( ) design without leaking mist types into wisp/user applications.

Essentially, you'd have to make each websockets type present in a single sum type for each the message and state as we're currently forced to lock in the WsCapability type in the handler.

I explored "safely" casting the types under the hood which while it worked and I believe was safe from being able to be misused, as you've previously stated and I do agree with, it does undermine the type system and a solution away from this would be the way to go.

Unfortunately I can't seem to find a way to set this type any later or earlier than the handler without leaking mists connection. It's probably a skill issue but I've gone over it and over it many times to no avail sadly.

lpil reviewed May 20, 2024

View reviewed changes

src/wisp.gleam Outdated Show resolved Hide resolved

src/wisp.gleam Show resolved Hide resolved

src/wisp.gleam Outdated Show resolved Hide resolved

lpil reviewed May 29, 2024

View reviewed changes

src/wisp.gleam Outdated Show resolved Hide resolved

src/wisp.gleam Outdated Show resolved Hide resolved

src/wisp.gleam Outdated Show resolved Hide resolved

lpil reviewed Jun 18, 2024

View reviewed changes

gleam.toml Outdated Show resolved Hide resolved

lpil reviewed Jul 13, 2024

View reviewed changes

bgwdotdev mentioned this pull request Jul 23, 2024

Mist module split #87

Merged

bgwdotdev added 12 commits July 25, 2024 15:08

test: add futher implementation test/reference server

024a553

feat: change wssupport and ws to be a single thing and add docs

85ba328

docs: add additional documentation and websocket server/client testing

6a8a14b

note: the client doesn't seem to return any responses for some reason when running under gleeunit

refactor: move various functions to internal to allow make_connection…

12b8b07

… to be hidden from docs

build: move stratus to dev dependencies

81a1a10

tidy: remove debugs, update docs, add type alias

c3e27bf

test: update dependencies to fix and finalise websocket test

ed9e417

doc(example): add websocket client & server ping/pong

c02b784

doc(example): add websocket chatroom server

1b66441

doc(example): tidy up chatroom server example

7c9b51d

test: tidy websocket test

a2e88a1

bgwdotdev force-pushed the feat/websocket branch from cdcffe1 to a2e88a1 Compare July 25, 2024 14:21

feat: remove server specific types from leaking into wisp

3fcbc22

lpil reviewed Aug 24, 2024

View reviewed changes

lpil marked this pull request as draft August 24, 2024 12:34

bgwdotdev added 2 commits August 24, 2024 16:12

doc(example): remove websocket chatroom server

d5aec52

fix: update all names to WsXyz format and fix examples

598305c

bgwdotdev added 3 commits August 24, 2024 17:59

docs: fix doc description for WsMessage

d6c4644

doc(example): update existing examples handler functions

577cb6d

fix: remove the ability for users to contruct websocket types

0e8a405

bgwdotdev force-pushed the feat/websocket branch from 896eacb to 0e8a405 Compare August 24, 2024 19:28

bgwdotdev added 2 commits August 26, 2024 15:49

doc: update CHANEGLOG

d8e266e

fix: add glisten as a direct dependency for socket errors

d37375a

lpil reviewed Sep 12, 2024

View reviewed changes

websockets #72

Are you sure you want to change the base?

websockets #72

Conversation

bgwdotdev commented May 11, 2024 • edited Loading

lpil left a comment

Choose a reason for hiding this comment

bgwdotdev commented May 26, 2024

lpil left a comment

Choose a reason for hiding this comment

bgwdotdev commented Jun 18, 2024 • edited Loading

bgwdotdev commented Jun 25, 2024 • edited Loading

bgwdotdev commented Jun 26, 2024 • edited Loading

lpil left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgwdotdev Jul 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgwdotdev Jul 23, 2024 • edited Loading

Choose a reason for hiding this comment

bgwdotdev commented Jul 14, 2024

bgwdotdev commented Jul 23, 2024

bgwdotdev commented Jul 25, 2024

lpil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpil Aug 24, 2024 • edited Loading

Choose a reason for hiding this comment

bgwdotdev Aug 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgwdotdev Aug 24, 2024 • edited Loading

Choose a reason for hiding this comment

bgwdotdev commented Aug 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpil commented Oct 9, 2024

bgwdotdev commented Oct 10, 2024

bgwdotdev commented May 11, 2024 •

edited

Loading

bgwdotdev commented Jun 18, 2024 •

edited

Loading

bgwdotdev commented Jun 25, 2024 •

edited

Loading

bgwdotdev commented Jun 26, 2024 •

edited

Loading

lpil left a comment •

edited

Loading

bgwdotdev Jul 25, 2024 •

edited

Loading

bgwdotdev Jul 23, 2024 •

edited

Loading

lpil Aug 24, 2024 •

edited

Loading

bgwdotdev Aug 24, 2024 •

edited

Loading

bgwdotdev Aug 24, 2024 •

edited

Loading