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

Integration with frameworks #70

Open
vickenty opened this issue Sep 8, 2019 · 7 comments
Open

Integration with frameworks #70

vickenty opened this issue Sep 8, 2019 · 7 comments

Comments

@vickenty
Copy link

vickenty commented Sep 8, 2019

I would like to be able to use tungstenite to add websocket support to an existing web-app, using the same tcp listener for both WS and regular HTTP requests.

This means that both web-server frameworks and WS libraries need to be able to inter-operate. Some pieces are already in place: hyper, for example, supports HTTP upgrade in a generic way, which allows servers to perform websocket handshake; and tokio-tungstenite provides WebSocketStream::from_raw_socket, which can take over after the handshake. An example of this can be found in gotham-rs/gotham#354.

The missing bit is the handshake itself. tungstenite does HTTP parsing itself from the byte streams, something that can not be used in a framework based app: once the route handler is invoked, the request was already parsed; the response typically must come as some high-level 'Response` object too, rather than a stream of bytes.

So the question is, would it make sense to extend tungstenite to support this use case? Ideally, user would supply request headers in a high-level form, and receive result, indicating whether websocket connection can be initiated and what the response should be.

@daniel-abramov
Copy link
Member

Good question. I think we already have (or at least had) a similar question. I do agree that the integration with web frameworks is very desirable and we would also like to be able to use such things in production. There are however some frameworks which were able to make a nice integration. One such example is warp. They use tungstenite-rs under the hood for WebSocket support and this framwork is actually an example of what I expect from web-framework integration as they work quite nice together.

My idea was to analyse that part inside warp and make our API a bit more flexible, so that warp can use tokio-tungstenite instead of writing its own layer on top of tungstenite-rs, which actually looks very similar to our tokio-tungstenite implementation. I'm not sure about other web frameworks, I think we'll prioritize integration with warp, as it is written and developed by the same people who actively worked and continue working on tokio stack (the author of warp is the same guy who wrote hyper), also warp feels to be much more "tokio"-like inspired I've been working with.

But that's all is more about elimination of boilerplate and copy-paste part inside the frameworks (currently warp does most of the things internally, so it kinda duplicates some logic which is implemented on our side).

@vickenty
Copy link
Author

vickenty commented Sep 8, 2019

Yes, that was exactly my question, whether tungstenite can provide handshaking logic to frameworks, so they don't have to re-implement it.

I took a look at what warp does. They have two-step process: first the websocket related bits are extracted from the request, and later user can accept the request by calling a method with a closure to handle the websocket connection.

So I think the minimal API looks fairly simple:

struct WebSocketReq { /* key, version, requested extensions, and so on */ }
/// Extract upgrade information from HTTP request.
fn prepare(req: http::Request) -> Result<WebSocketReq, Error>;
/// Generate `101` response from the request.
fn accept(ws_req: WebSocketReq) -> http::Response;

I think this can serve both warp and other hyper based frameworks. Framework still has to deal with the HTTP upgrade and scheduling of futures and such, but no longer has to have knowledge how to generate Sec-WebSocket-Accept header.

Do you think this direction is worth pursuing?

@najamelan
Copy link
Contributor

najamelan commented Sep 13, 2019

This issue has come up in tide: http-rs/tide#67 as to how to support websockets there.

I see the situation as follows:
A websocket connection has 2 clearly distinct phases, an http handshake and the actual message passing + close handshake, which is integrated in phase 2.

We have at least 5 implementations of phase 1: actix-web, hyper, tungstenite, ws, websocket, + a number of forks of some of those.

We have at least 5 implementations of the second phase (hyper doesn't implement that but warp does).

The problems this implies:

  • duplicate effort
  • several half-baked implementations (lacking docs, tests, features like extensions and protocols, ...)
  • larger surface for bugs and security vulnerabilities (eg. phase one has to deal with TLS as well as HTTP).
  • interoperability

Those problems are present for both phases. Ideally there would be 1 very good implementation of websockets in rust. It's always hard to get a bunch of people with different visions to work together, but I think it's safe to say the current situation is unfortunate.

What is particular to the first phase is that it also concerns http servers, and not only the other three libs. Maybe it would be good start with this first phase as an attempt to both reduce duplication of effort and improve quality. I think it would be a good idea to split it in a separate crate, co-developed by the authors of all four current implementations. I'm in an uncomfortable position proposing this, since I'm not one of such authors and I don't want to be, but I really think it would be the way forward.

If there were 1 websocket_handshake crate that everybody could agree on, it would be a great day for rust networking!

Just want to mention about warp, that they don't want to expose tungstenite (implementation detail), and thus have provided their own websocket API (bye bye interoperability), which has numerous issues which i have started to describing here: seanmonstar/warp#257 and seanmonstar/warp#258. If there was only one reference websocket crate in rust that everyone agreed was good, maybe they could agree to use that instead of rolling their own.

@daniel-abramov
Copy link
Member

I would also like to change tungstenite-rs and tokio-tungstenite, so that warp can directly rely on tokio-tungstenite and be compatible with it, that makes sense. I think we even discussed it somewhere in warp issues a long time ago, but no-one of us worked found enough time to work on that...

@sdroege
Copy link
Contributor

sdroege commented Nov 24, 2019

Together with #93, I think the only thing missing for easy integration with async frameworks would be to move AsyncRead / AsyncWrite impls (i.e. the ones from tokio/async-tungstenite) into this crate instead of having them separately. That is currently blocked by tokio and the futures crate having different versions of these traits.

Once that is there, handshake::server::create_response() can be used to create a proper http::Response based on the http::Request that the framework will likely provide in one way or another. And then the "upgraded" request (e.g. with an API similar to the one in hyper) one would get a AsyncRead/Write that can be directly wrapped in WebSocketStream::from_raw_socket().

All that is already possible with tokio/async-tungstenite (once they're also updated to use the http types) but you'd have to decide on which set of traits you want to use.

Anything I'm missing here?

@daniel-abramov
Copy link
Member

Together with #93, I think the only thing missing for easy integration with async frameworks would be to move AsyncRead / AsyncWrite impls (i.e. the ones from tokio/async-tungstenite) into this crate instead of having them separately. That is currently blocked by tokio and the futures crate having different versions of these traits.

I really hope that they'll release a stable version soon which will rely on the same traits.

As far as I understood it, this was the only one (?or maybe one among several reasons?) why the PR which brings the support of async to tokio-tungstenite uses AllowStd and a bunch of unsafe code around it. Of course we would like to minimize the amount of unsafe code.

@sdroege
Copy link
Contributor

sdroege commented Nov 25, 2019

I really hope that they'll release a stable version soon which will rely on the same traits.

They intend to stay with their own versions of the traits for the time being, see tokio-rs/tokio#1297

the support of async to tokio-tungstenite uses AllowStd and a bunch of unsafe code around it

The unsafe code is not needed from what I can see, but I'll spend time on removing that once it's actually otherwise correct (see snapview/tokio-tungstenite#68 (comment) ).

The AllowStd stuff is needed for having a Read+Write that is internally backed by an AsyncRead+AsyncWrite and that will always be needed in one way or another (and should really be moved to a separate crate (this is basically like that but also has some problems)).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants