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

Websocket channels #1639

Closed
wants to merge 65 commits into from
Closed

Conversation

the10thWiz
Copy link
Collaborator

@the10thWiz the10thWiz commented May 13, 2021

This is an actual Websocket implementation. Channels, and the ability to easily send messages to a large group of clients has not been fully implemeneted. There are a few issues which need to be resolved before it can be considered complete.

  • Incomming message type
  • Outgoing message type
  • Join & Leave events
    • Does Leave fire when the server initiates the close?
  • Channel descriptor type
    • How to subscribe a client to a channel?
    • How are channels created?
  • Add message size limit
  • Stream messages

Types

Incomming messages are converted to a Data type, and can be converted using any normal FromData type. Outgoing messages must implement IntoMessage, which requires them to identify text vs binary, and convert it into bytes.

IntoMessage currently has a generic implementation for any type that implements AsyncRead. This is likely a bad idea, since it makes implementing IntoMessage for types in the standard library much harder.

Join & Leave

Should the user be able to specify actions for when a user connects or leaves? This would most likely be implemented via specifying a function to run in the websocket macro.

These will be seperate macros, specifically a join and leave macro, along with renaming the websocket macro to message. Join and Leave will be optional, although I like the idea of requiring a join event, perhaps with a parameter in the message macro.

Channels

The current implementation just uses a String, and doesn't implement any type of hierarchy. I think a final implementation needs to have a hierarchy, analogous to Phoenix's topics and subtopics, and I have a few ideas on how this could be acomplished.

- String, with optional : as a seperator for topics and subtopics. The only thing this has going for it is that Phoenix does it.
- Path, or a Uri of some type. This has much better support within Rocket already, but may not be ideal.
- A trait, which could perhaps be implemented on user created structs via a derive macro. I like this option the best, however it would require to most work. See Channel Derive below.
- A new struct. I don't like this option becuase it's basically like the Path or Uri option, but without any existing code or support.

Subscribing to a topic is done via the subscribe method on a Websocket object, but it could also be implemented as part of the websocket macro. Phoenix requires a client to be subscribed to a topic to be able to send messages to a specific topic, but I don't think it's an absolute neccessity for Rocket, since it would be fairly trivial to check before sending it. To make this easier, we could add a subscribed_to method, which checks whether a client is subscribed to a specific topic.

I'm looking into creating channels based on the URI used to connect. This would remove the type paramter from Broker (to more closely match Sergio's example), and Broker is due for a complete rewrite anyway.

This will also likely require a custom client-side js library to multiplex connections, since supporting Websockets over HTTP/2 is blocked on h2 supporting the headers we need. This could actually solve the issues with IntoMessage, by enabling us to handle some of the issues on the client-side.

@the10thWiz
Copy link
Collaborator Author

I have finsished my implementation of Phoenix like channels, using a ChannelDescriptor trait, as outlined above. I have not yet written a derive macro for it, but I don't expect it to be very complicated. I have implemented ChannelDescriptor for a set of types in the standard library, so it can already be used without creating a new struct. The implementation allows a simple example where Strings are sufficent to be created using just a String.

There are only a few more things to work on. Join & Leave events, and a potential feature gate are the big ticket item, although a few minor improvements are noted with TODO tags in the code.

@SergioBenitez
Copy link
Member

For those watching: design discussions are taking place on Matrix. Here is my proposed API for websockets, by example:

//! A multiroom chat application with a special `echo` room which echoes its
//! messages to `lobby`.

// `&Channel`, `&Broker` are channel guards. `&Broker` is also a request guard.
//
// A `Channel` is:
//   1) A bidrectional channel between two endpoints: client and server.
//   2) Linked to a topic URI, for broadcasting to that topic.
//
// A `Broker` knows about all channels.

type UserList<'r> = State<'r, ConcurrentHashSet<String>>;

enum Event<'r> {
    Join(&'r str),
    Message(&'r str),
    Leave(&'r str),
}

/// A new client has connected to a room. Inform the room of this, say hello to
/// the client with a "join" message.
#[join("/<_>", data = "<username>")]
fn join(channel: &Channel, username: &str, users: UserList<'_>) {
    if users.contains_or_add(username) {
        channel.send(Event::Leave("username taken")).await;
        return channel.close();
    }

    channel.local_cache(|| username.to_string());
    channel.send(Event::Join("hello!")).await;
    channel.broadcast(Event::Join(username)).await;
}

/// A message sent to a room that's not `echo`.
#[message("/<_>", data = "<msg>")]
fn room(channel: &Channel, msg: &str) {
    channel.broadcast(Event::Message(msg)).await;
}

/// A message sent to `/echo`: send it to `/echo` and `/lobby` concurrently.
#[message("/echo", data = "<msg>")]
fn echo(broker: &Broker, channel: &Channel, msg: &str) {
    let lobby_send = channel!(broker, room("/lobby")).broadcast(Event::Message(msg));
    let room_send = channel.broadcast(Event::Message(msg));
    join!(lobby_send, room_send).await;
}

/// A user has left a room. Let the room know. `Username` retrieves the username
/// stored in `join` in `channel-local cache` via an elided channel guard impl.
#[leave("/<_>")]
fn leave(channel: &Channel, username: &Username, users: UserList<'_>) {
    users.remove(username);
    channel.broadcast(Event::Leave(username)).await;
}

@the10thWiz
Copy link
Collaborator Author

I've done some work on this, and I will be adding it soon. I'm also going to be updating the top message to make it better match the current plan.

@the10thWiz
Copy link
Collaborator Author

the10thWiz commented Jun 2, 2021

I think the Rocket side has a more-or-less completed API. There are only a few changes I want to make, which should be fairly minor. However, the client library has not been written yet. I will need help to make that happen.

  • Seperate Channel Guards from Request Guards
  • Implement cross-channel broadcasts
  • Autobahn testing
  • Leave events
  • Documentation
  • Panics & internal improvements

Channel Guards should become a seperate type from Request Guards, but we should provide a defualt implementation for T: FromRequest.

The internals for cross-channel broadcasts already exist, but an API needs to be created. Sergio's example treats Channel and Broker as seperate types, but my current code makes Broker a member of Channel.

Unfortunatly, in order to implement the ability to stream messages, I had to write a significant amount of the implementation in Rocket itself. In order to test performance and correctness, we should run the Autobahn test suite against the Rocket implementation.

At the moment, a Leave event is fired even if the server initiated the close. I think this is the desired behaviour, but I want to make sure.

Documentation is sorely lacking right now. I have been putting off writing in depth documentation and examples, especially since the Rocket API was still undergoing major changes. I would like to make an attempt to stablize the API provided before delving into writing documentation.

A finalized version of this implementation should never panic, although I cannot say for sure that it doesn't right now. Testing with Autobahn and writing examples are likely to find some edge cases, but I, and someone else, should manually review the code to find any location where a panic is possible. I would also like to make internal improvements, to either make the implementation more effiecent, or easier to read. There is a large select! statement that is hard to work with. There are some sematics around select! that provide some subtle pitfalls, which should be documented and/or worked around.

@SergioBenitez, Would you mind reviewing the completed portion of this implementation? I would also appreciate any advice or guidance on writing the JS client library for multiplexing connections. I have documented the protocol at the end of channels::router, but it needs a permanent home. I suspect it should be added to the guide, but I haven't gotten to adding websockets to the guide yet.

@the10thWiz
Copy link
Collaborator Author

the10thWiz commented Jun 8, 2021

I think this PR is feature complete. It has all of the features I want to include, let me know if there are any omissions that need to be addressed.

There are a number of changes that need to take place before this is ready, however they should be internal or minor changes, such as explicitly handling more situations, or changing the api.
I'm not ruling out breaking changes yet, but I would like to validate that all of the required features are present.

Specific notes

Panics

I need to explicitly handle panics in user handlers

Autobahn testing

I have run the Autobahn test suite agaisnt the websocket implementation in Rocket, and there are a number of tests that fail. See the channels module documetation for more information.

Rocket-Multiplex

I have written a full specification of the rocket-multiplex protocol, in the rocket_multiplex module. I have also implemented it in Rocket, and a client JS library (It's in the websocket-multiplex example right now).

It does currently prevent the use of any other subprotocol, but I also haven't implemented it on the server side, so I don't think it will be an issue.

Guide

I have written a single page for the guide, but it's not complete in any way. It was nice just to make notes and put it in writing, and it helped me find pain points in the API.

IntoMessage

I have chosen to switch to individual IntoMessage implementations, rather than a blanket implementation on T: AsyncRead. This makes it possible to implement it on Json Values, as well as String and other types in the standard library.

Websocket Macros

There are changes need to the websocket macros. I will likely need to change some of the call signatures and codegen, as well as the macros parameters.

@OvermindDL1
Copy link

Is there going to be a rust client built on web-sys, native, or others as well? It would be very useful to be able to access it directly from within wasm apps in the browser for consistency and speed or via native clients as javascript doesn't really exist there.

@schrieveslaach
Copy link
Contributor

schrieveslaach commented Jun 16, 2021

@OvermindDL1, are you looking for something like wasm-sockets? I think it would be a nice test to check if this PR and wasm-sockets are compatible.

@the10thWiz
Copy link
Collaborator Author

I've made some major changes.

  • WebSocket authentication code is now part of the PR
    • Join events no longer have Data
  • WebSocket vs Channel has been resolved there is now just one type (Channel)
  • Leave events can now use the close status as Data
  • IntoMessage is no longer needed, Responder is now a supported alternative

Overall, I think this is in a much better place. Some review would be nice, to see if I'm on the
right track. To make this easier, I'm going to spend some time writing better examples and
information for the guide.

Concerns:

I have a few concerns with the code I've written. I've found a way to avoid the need for IntoMessage,
specifially by supporting Responder instead. Basically, I just extract the ContentType to determine
whether the type is binary or text, and then forward the body to the client. This does just discard
headers and status, and I'm not sure the implications of this.

Major TODOs:

  • Documentation
  • Examples
  • Broker effiency
  • Investigate Multiplexing

The changes to Broker
would be entirely internal, and would not effect the external API. Multiplexing is largely off the table
at this point, and I have no specific plans for it. For now, I'm treating Multiplexing as an enhancement
rather than a core feature.

@SergioBenitez I think this is ready for a review.

I would like to draw your attention specifically to the Channel type, and the send method. I've noted
it above in my concerns.

I hope the API here can be stablized, and the only concern is performance, documentation and enhancements.
I think the next major step is writing actual code using this, and trying to find any issues.

Includes code to actually get a handle to the running Rocket
installation
@nicoburns
Copy link

Is it possible to get raw access to a websocket with this PR (and do your own auth and protocol/channel management)? The channels API sounds great, but I imagine a lot of people wanting to use websockets with Rocket will have an existing protocol implementation that they need to conform to, and won't necessarily have the luxury of converting to Rocket's system.

@SergioBenitez
Copy link
Member

Rocket won't impose any kind of protocol by default. A "channel" is just the name for an internal handle that represents a topic and a client.

@OvermindDL1
Copy link

I think @nicoburns is more referencing when one is forced to use a JS library that does their own client websocket handling, which is more annoyingly common than one would hope...

@the10thWiz
Copy link
Collaborator Author

I think @nicoburns is more referencing when one is forced to use a JS library that does their own client websocket handling, which is more annoyingly common than one would hope...

Yes, but there are also situations when you may be forced to make it work with an existing client (i.e. migrating a server to Rocket from another framework).

As far as how much this will work: There are limitations. Implementing your own auth isn't hard, although I may want to bring back the above idea of a matcher to make that work better. I don't expect protocol and channel management to be exposed to the user code.

There are a few considerations: Rocket's system is largely server-side, and I'm specifically designing the code to work with the browser based JS WebSocket library. It's a pretty restrictive client, especially when it comes to authentication, which is why the authentication system is designed the way it is. Out of curiosity, @nicoburns what type of authentication are you using? The two most common types of authentication I've seen are both token based, and the only difference is how the tokens are passed to the websocket connection. One option is a query parameter, but Rocket considers that to be part of the topic, and the other option is a temporary URI (i.e. the entire URI is the token). An alternative to Rocket's WebSocket Token would be to parse the token from a query param, stick it into the request's local cache, and remove it from the URI in a fairing. Then, the route would have a request guard that checks for the existence of the token in the cache.

Custom channel management is somewhat difficult to talk about, because I'm not quite sure what you mean by channel. Rocket thinks of a channel as a set of connections, identified by a topic (the origin URI used to connect). It may be possible to add a few options that would allow for the creation of a separate broker, but I don't think it's worthwhile. If there is some flexibility you are looking for from the Broker, let me know. I'm not very happy with the implementation, and I feel that there must be a better way.

@HarmoGlace
Copy link

Is there any update?

@toxeus
Copy link
Contributor

toxeus commented Sep 22, 2021

I can recommend taking a look at axum to those that require built-in websocket support (needs to be enabled via a feature flag). It has similar usage ergonomics like Rocket and is developed and maintained by the tokio team. Further key advantages are that axum is modular, it takes advantage of easy plug&play with tower middleware, it's not a one-man show and less opinionated than Rocket.

@OvermindDL1
Copy link

@toxeus That doesn't really seem appropriate to post here? Especially as the reason a lot of people like using rocket is due to how its path resolution and guards work, which last I checked axum has neither functionality of?

@toxeus
Copy link
Contributor

toxeus commented Oct 1, 2021

@OvermindDL1 I don't think there's anything inappropriate about my post.

I'm a rocket user myself but I cannot use it in services where I need built-in websocket support. Axum is a good choice for such use-cases. I think it's just fair to share about options with the community while they are waiting for this PR to land.

@OvermindDL1
Copy link

@toxeus The community tends to pretty well know about about axum at this point though, I keep hearing about it all over the place. ^.^;

What would be nice is showing how to use axum and rocket together, to use axum as the websocket interface but rocket for everything else. I 'think' that's possible with hyper as the backend.

@toxeus
Copy link
Contributor

toxeus commented Oct 1, 2021

@OvermindDL1 interesting! I've heard about axum just recently. That's why I felt it's worth sharing about it after I experienced how well designed it is.

What would be nice is showing how to use axum and rocket together, to use axum as the websocket interface but rocket for everything else. I 'think' that's possible with hyper as the backend.

Nope, rocket doesn't allow for that. As I said before, rocket is an opinionated project :)

@OvermindDL1
Copy link

Another use-case I just hit with rocket, needing to allow graphql subscriptions at a certain route, and well no websockets, of which the graphql layer needs to take over as it has its own API language to speak (async-graphql) over it to pre-existing web and non-web clients where I cannot change how they work. I reduced that project to axum for now but I really hope that rocket gets some way to work with websockets directly so I can uncomment all that original code...

Nope, rocket doesn't allow for that. As I said before, rocket is an opinionated project :)

I was thinking the other way, axum underneath that delegates to rocket on anything it doesn't handle (well, hyper would do that specifically, not axum). I'm pretty sure rocket has a way for that but I've not tested yet, wanted to get the above thing done pretty asap so I didn't feel like experimenting yet.

@toxeus
Copy link
Contributor

toxeus commented Oct 20, 2021

@OvermindDL1 that is exactly my use-case and I had also to reach for axum to make it work :)

With that said, I feel it's slightly off-topic to discuss ideas on how to make rocket work with axum/hyper in this PR. I assume that people who have subscribed to this PR are mostly interested in the progress of the PR. But please ping me in case you find a solution ;)

@toxeus toxeus mentioned this pull request Dec 7, 2021
@EgMan
Copy link

EgMan commented Feb 14, 2022

Is there any status updates on this PR? Lack of WS support really seems like the single biggest sticking point for consumers of this framework.

What still needs to be done?
@SergioBenitez @the10thWiz

@the10thWiz
Copy link
Collaborator Author

@EgMan

Is there any status updates on this PR? Lack of WS support really seems like the single biggest sticking point for consumers of this framework.

What still needs to be done? @SergioBenitez @the10thWiz

It's somewhat complicated. To be honest, I don't think this PR is going to be the one to bring WS to Rocket, at least not without some major rewrites. The WS API design should definitely be revisited, and I'm not sure if this PR is going in the right direction. In the near future, I'm going to take another look at designing an API from scratch that fits within the Rocket framework.

Since I don't think I've organized my thoughts anywhere, I'm going to do that here. Forcing the WS protocol to act/look like HTTP, as a way to help the WS API fit in with Rocket doesn't quite work. The biggest obstacle here is arguably the JS browser-based WS client, which Rocket almost certainly needs to work with.

The first issue I would like to highlight it the message format - WS differentiates between text and binary, but not any other format. HTTP has full mime-type specification, and doesn't make a distinction between text and binary. Sadly, I don't think believe we can simply choose one or the other, since text is restricted to UTF8, while binary is difficult to work with on the client side. The JS client will read text messages as strings, while binary messages are read as blobs or similar.

The second issue is server side initiation: Rocket is designed for REST APIs, where the server is always responding to a request from the client, rather than the server initiating a message. I think this issue can be resolved, but this also depends on the message format resolution (at least with how I've laid out the message format in this PR).

The third issue is authentication. Rocket is supposed to be secure and easy to use, so the WS API needs an authentication method that sort of just works. Ideally, it should be accomplished through request guards, especially since this would allow WS to use any existing HTTP authentication (as the RFC claims), but many WS clients (notably the browser's JS client) don't provide the flexibility required to make it work. However, I think this can be treated as a largely solved problem, since the most common method appears to be using a token obtained from an HTTP API. The JS client only provides the option to specify a URL, and a list of accepted protocols (which are actually just a suggestion anyway), so the token is typically part of the URL. In Rocket, this would require the user to create a WS authentication route (with the appropriate request guards), and return a WS token object. The WS token would then be submitted to a special route defined by Rocket (potentially user configurable), and the token object could be used as a request guard on the WS routes.

The fourth issue is channels. Segio has expressed a desire to include a channels-like feature, and I think this is a reasonable way to solve the second issue: by providing a channel API, messages would be submitted to a channel, and any clients subscribed to a channel would receive the message. However, the plain WS protocol doesn't provide this, and the WS protocol completely takes over the TCP connection. If Hyper supported the necessary psuedo headers (or Rocket switched to a different HTTP backend that did), we could simply suggest that the client use HTTP/2 to multiplex the WS connections. This PR creates a protocol on top of WS to enable multiplexing. I don't know if this is a sustainable solution, and it may be worth revisiting so see if we can avoid it.

A WS API for Rocket needs to be ergonomic, fast, and safe. Adding a WS route to Rocket should be simple and easy (Segio has said it should be four lines to create the route), and ideally it should not take a significant amount of code to extend the API to more complex use cases. Performance could be an issue, although I suspect that it should be fine. Safety is definitely more complicated. Rocket uses the request lifetime to guarantee the safety and security of request guards, and WS kind of breaks that. I think the current iteration of this PR avoids using unsafe code, or making a copy of the request, but it been a little while since I've worked on it.

TL;DR: The WS API needs to be ironed out before we can really move forward.

- Undoes the unsafe hack to make json work
- Adds `pub(crate) apply_delta` to CookieJar
- Adds `ws_cache` to Request
  - Makes several more properties pub(crate)
- Removes clone of request in favor of constructing a WsRequest
- Add note on alternatives to rocket-multiplex
@the10thWiz
Copy link
Collaborator Author

After a little bit of searching, I've come across some articles on how to implement cookie based authentication for websockets. It seems like most browsers do send cookies along with the Websocket upgrade request, which means the could be used as is, and any auth implementation that held the logged in state with a cookie would just work out of the box.

However, they also note that the Same Origin Policy doesn't apply to Websockets - so auth cookies get sent with any request to your server, even if it was initiated by another page. The recommended solution is to either use token based auth, or to check the Host header sent by the client. The Host header does reveal where the script is running from, and allows us to verify that the browser is connecting from a script hosted on the same server.

Whether or not we decide to support cookie based authentication, the fact that websockets don't respect the SOP needs to be dealt with. The simple solution would be to just disable cookies (in some fashion) for websocket connections, but this might require some major API changes. I don't think we can avoid this, unless we require the user to specify what hosts are permitted to connect.

As a proof of concept, I've implemented an additional parameter for the route macro: allowed_origins. This is a whitelist of domains that the host header MUST match to call this route. Additionally, localhost and 127.0.0.1 are always allowed, (although they can be specified in the parameter), to enable local testing. For websocket connections, this should be required for a guard to access any cookies. I've implemented some basic pattern matching, but it's very strict. I use * to indicate that a section can be replaced with anything, but it must be a full part of the domain name (i.e. *dev.test.com is invalid since the star doesn't match the whole subdomain), and the final two subdomains cannot be wildcards. If the browser didn't send a host header, (i.e. req.host() returns None), no pattern matches. This does mean that HTTP 1.0 clients probably can't access routes protected with allowed_origins, but I think this is an acceptable compromise.

This feature was motivated by the websocket use case, but I don't see any reason why we shouldn't offer it for http routes. This can already be implemented via a request guard, but bringing this into Rocket allows it to take special action (enabling/disabling cookies in websocket routes) based on whether the host matches.

- Add allowed_origins parameter to routes
- Make `Channel::request()` public for codegen
@the10thWiz the10thWiz closed this Aug 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants