Usability of the WebSocket filter

[najamelan]

I'm implementing a lib that provides AsyncRead/AsyncWrite on top of websockets, to enable codec based communication between wasm code running in the browser and a server.

The wasm part is published, but the server part turns out to be more of a challenge. Since applications with a web frontend almost always have to serve static files, it makes sense to write this library as to be compatible with other crates like hyper and warp. This makes it possible for people to upgrade http connections benefiting from the convenience, not having to handle TLS separately and so on.

So it would seem the most appropriate input for my library would be to take in an object that implements Stream<Result<tungstenite::Message, tungstenite::Error>> and Sink<tungstenite::Message, Error=tungstenite::Error> and return an object that implements AsyncRead/AsyncWrite. So I had a look at the warp ws module to see how to go about providing compatibility with warp and I'm running into some blockers:

1. warp does not return the tungstenite types like tokio-tungstenite. This means I'm obliged to create my own type which implements From<warp::ws::Message> and From<warp::Error>. And creating similar conversion from tungstenite types to provide compatibility with raw tungstenite or tokio-tungstenite connections. In itself this is not convenient, but it's not the end of the world, it can be done. Is it important for warp to not expose dependency types in the public API? If not, could we at least allow access to the inner type, or provide Into<tungstenite::Message> for warp::ws::Message so that we can access the inner type?

2. warp::ws::Message does not provide the CloseFrame from tungstenite, making it impossible for the client code to detect that the websocket client disconnects cleanly and read the close code and reason.

3. warp::ws::Message returns the data contained in the message as a &[u8]. With tungstenite::Message which is an enum, I can destructure in order to get the inner Vec<u8> or String without making an extra copy of the data. With warp::ws::Message this doesn't seem possible if I need to store the data in a new type. I'm not even sure that I could mem::swap a &[u8] to obtain a Vec<u8>. If that's possible, than this is not an issue. If the copy is necessary I won't use warp until this changes, and I will look for a more low-level solution like hyper->tokio-tungstenite. I suppose I could just make my type an enum over types from both crates, which means I can access the data by reference where it is actually needed, but it will result in quite some code bloat on my part, and maybe it would be nice to add an api which takes self by value and returns the inner data by value. tungstenite::Message provides this copyless conversion with Into<Vec<u8>>.

I wonder how these issues can best be resolved. I shared some other ideas of how we could reduce redundency of websocket code in the ecosystem here.

[najamelan]

ps: is_pong() -> bool is also missing from warp::ws::Message

[najamelan]

Looking a bit more into the Stream implementation for ws::WebSocket, it does actually return Close messages from tungstenite, but the ws::Message type does not allow the user to inspect it. That also means that the following unreachable isn't really unreachable:

/// Return the bytes of this message.
pub fn as_bytes(&self) -> &[u8] {
    match self.inner {
        protocol::Message::Text(ref s) => s.as_bytes(),
        protocol::Message::Binary(ref v) => v,
        _ => unreachable!(),
    }
}

[najamelan]

Im running into some more trouble looking at the error handling. warp::Error has a kind enum, but it's not public, nor is the inner error. So it seems the only ways to get any information about warp::Error are the deprecated cause method which btw for tungstenite will always return None, in other words no information, and the Display or Debug information.

The latter one would mean parsing textual error messages back into some meaningful enum to be able to act on that. I don't think I want to go there.

So not only does it encapsulate all the useful information from the tungstenite::Error, I couldn't even match on Kind to distinguish it from another type of warp message.

Btw, it's perfectly fine to use the replacement method of cause, called source because the standard library provided default for source will just call cause under the hood.

[seanmonstar]

Thanks for writing this all up, we've been slowly chipping away at it. I think the majority is complete, with these notes:

1. warp doesn't expose that tungstenite is used internally, which allows for changing to a different implementation, or upgrading versions.
2. There's now Message::close() and Message::close_with(code, reason) to create close messages. There still isn't a way to inspect received messages beyond Message::is_close().
3. Message::as_bytes() now works for all frame types, and Message::into_bytes() will consume itself and return a Vec, so a copy is no longer needed.
4. Message::is_pong() is now available.
5. Yea, I'd probably like to add some warp::ws::Error that is returned by warp::ws::WebSocket instead of just warp::Error, but that's a breaking change, so would need to be part of 0.3. We could start by adding that type with getters to inspect for some error cases, putting that inside the warp::Error, and then you could look for it via Error::source.

[seanmonstar]

With that in mind, I'm going to close this. If there's desire for continuing a specific point, we can open a focused issue for it.