trillium-client: Add client WebSocket support

[joshtriplett]

Add a websocket feature gating support for client-side WebSocket
support. When set, provide methods on trillium_client::Conn that allow
upgrading to a WebSocketConn.

Usage:

let mut ws = client
    .get(url)
    .with_websocket_upgrade_headers()
    .await?
    .into_websocket()
    .await?;

ws.send_string(format!("Hello from a WebSocket client!")).await?;

Having this support natively in trillium allows reusing a connection
from the connection pool when establishing a WebSocket connection
(though of course the connection cannot go back to the pool). It also
allows using any headers and base already set on the client. And
finally, it allows clients to avoid some of async-tungstenite's
dependencies, such as async-tls and rustls, in favor of
trillium-client's dependencies.

[codecov]

Codecov Report

Attention: 10 lines in your changes are missing coverage. Please review.

Comparison is base (dfbcd15) 46.06% compared to head (c510c79) 48.42%.

Files	Patch %	Lines
client/src/websocket.rs	75.67%	9 Missing ⚠️
websockets/src/websocket_connection.rs	83.33%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #520      +/-   ##
==========================================
+ Coverage   46.06%   48.42%   +2.35%     
==========================================
  Files         179      180       +1     
  Lines        6984     7030      +46     
==========================================
+ Hits         3217     3404     +187     
+ Misses       3767     3626     -141     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[jbr]

Thanks, I like this a lot! My only hesitation: I'm unsure if this would be better as a websockets feature on trillium-client. I put a lot more thought into how to carve up trillium server crates to avoid leaning too heavily on the cargo features or privileging crates I write over external crates, but the client situation is a lot less carefully considered.

I don't have a strong sense of which of the three options is best, but I think this and the proposed features in #449 should have consistent structure

The options:

1. trillium-client has features for additional behavior that pulls in additional dependencies (cookies, websockets, etc). client breaking: add initial version of client handlers #449 uses this approach.
2. the server crates like trillium-websockets/trillium-cookies add a default-disabled client feature and a default-enabled server feature (so people can opt of of any server-only dependencies if they're just using the client; this probably should exist even if there are no server-only dependencies for any crate that is both, for consistency of crate interface). This PR uses this approach.
3. add trillium-client-websockets/trillium-client-cookies crates, and/or rename trillium-client to have a namespace of its own¹. A possible advantage of this over 1. would be that trillium-client versions wouldn't be locked to the trillium version through a mutual dependency

Relevant question: Would the interface be advantaged at all by being intrinsic to trillium_client::Conn?

Footnotes

1. I've considered calling it myrmex because trillium plants are myrmecochorous ↩

[joshtriplett]

@jbr I did initially try the experiment of making this a websocket feature on trillium-client rather than a client feature on websockets. If we could do that, we could avoid the extension trait, and provide the functions directly on trillium_client::Conn conditional on the feature, which seems preferable for usability.

Here's what I ran into when doing that:

• I think we want to use WebSocketConn as the type here. Making that work would require exposing some internals of WebSocketConn, including the constructor and the use of WebsocketPeerIp in the StateSet of the Upgrade.
• This would split implementation details across both trillium-client and trillium-websockets, unless we factored out a common crate which seems like overkill. I ended up duplicating the handshake handling rather than introducing a tiny dependency crate.

If you don't mind some API bumps, I think the ideal interface looks like this:

• trillium-websockets exposes more of WebSocketConn as public API, and provides some some helpers.
• Remove WebsocketPeerIp, and have WebSocketConn store an Option<IpAddr> in a (private) field with a setter, as a workaround for Upgrade not having it. In the future, consider removing this in favor of a field on Upgrade when bumping the major versions of trillium_http and trillium.
• trillium_client::Conn adds the functions directly, under a websocket feature, and trillium-client has an optional dependency on trillium-websockets.

[joshtriplett]

@jbr I've rewritten this to live in trillium-client. On balance, the result seems much more usable.

[joshtriplett]

@jbr I've also tested this in my project that's using trillium, and it seems to work perfectly.

[jbr]

Expressing intent: I intend to merge this. If I have minor interface iterations, would you prefer I merge and make them as subsequent PR or suggest them here?

If the latter:

The hunch is that the multiple calls here present unnecessary opportunities to write code that doesn't work as intended:

let websocket = client
    .get(url)
    .with_websocket_upgrade_headers()
    .await?
    .into_websocket()?;

In this case, the last three lines will almost always be called in sequence, and if any of them is omitted, it won't work as intended.

A simpler interface might be:

let websocket = client.get(url).upgrade_websocket().await?;

where upgrade_websocket applies the appropriate headers with a try_insert, calls self.exec(), and then into_websocket. It might be that this should be a later improvement after there's a client::Error type that can contain the websocket error or trillium_http::Error.

This might even be built in terms of the existing interface, but I'm not sure with_websocket_upgrade_headers and into_websocket need to be public.

What do you think?

[joshtriplett]

@jbr wrote:

Expressing intent: I intend to merge this.

❤️ Thank you!

If I have minor interface iterations, would you prefer I merge and make them as subsequent PR or suggest them here?

Up to you, but I do like being involved in the ones that affect interface design, since they might surface potential assumptions about the ways I'm expecting to use the interface.

The hunch is that the multiple calls here present unnecessary opportunities to write code that doesn't work as intended:

let websocket = client
    .get(url)
    .with_websocket_upgrade_headers()
    .await?
    .into_websocket()?;

In this case, the last three lines will almost always be called in sequence, and if any of them is omitted, it won't work as intended.

A simpler interface might be:

let websocket = client.get(url).upgrade_websocket().await?;

where upgrade_websocket applies the appropriate headers with a try_insert, calls self.exec(), and then into_websocket. It might be that this should be a later improvement after there's a client::Error type that can contain the websocket error or trillium_http::Error.

This might even be built in terms of the existing interface, but I'm not sure with_websocket_upgrade_headers and into_websocket need to be public.

What do you think?

I agree with you, and I seriously considered using this approach. In particular, I thought about having into_websocket() automatically set the headers (if not already set) and send the connection if not yet sent.

The original reason I hadn't done this was because I need the error to be recoverable, retrieving the Conn from it, and that would have been more of a pain to implement in a multi-variant error enum. However, I do agree that it'd be more usable, so I'll go ahead and implement this.

[joshtriplett]

@jbr Done. into_websocket() and into_websocket_with_config() now send the request if not already sent. I also added a test case for recovering the Conn from an error after a failed upgrade, and using it to read an error message from the body.

I removed the public function for setting the upgrade headers, because I couldn't think of a use case for needing to run code between a try_insert of those headers and sending the request. Users may need to add additional headers to a WebSocket upgrade request, but they can always add them before calling into_websocket().

[joshtriplett]

The CI failure seems unrelated. Submitted a PR that should fix it: #522

[joshtriplett]

Rebased atop #522 so that CI passes now.

[joshtriplett]

@jbr Would it be possible to get a release with this change, please?