Making Soketto easier to use with `http` types (and `hyper`) #48

jsdw · 2021-09-14T16:01:20Z

This is an attempt to integrate some code behind an http feature flag which (hopefully) makes it much simpler to turn http::Requests (such as those hyper hands out) into Soketto WebSocket connections.

Have a look at the hyper example to see what making use of this would look like :)

Todo:

code
document feature flag
tests?

…to with http types

jsdw · 2021-09-14T16:01:54Z

.github/workflows/ci.yml

@@ -40,7 +40,7 @@ jobs:
        uses: actions-rs/cargo@v1.0.3
        with:
          command: check
-          args: --all-targets
+          args: --all-targets --all-features


Now that we have a feature, make sure CI checks the code behind it

jsdw · 2021-09-14T16:02:19Z

Cargo.toml

+
+[[example]]
+name = "hyper_server"
+required-features = ["http"]


This just makes the error messages nicer when trying to run the example.

jsdw · 2021-09-14T16:49:05Z

I'd definitely appreciate some feedback on the API design here (and whether we want it at all)! Part of me likes the idea of making it easier to use Soketto with the common http types and hyper because they are quite dominant in the ecosystem, but I don't like adding too much complexity/maintenance overhead either..

jsdw · 2021-09-14T16:52:43Z

src/handshake/http.rs

+/// This is handed back on a successful call to [`upgrade_request`].
+pub struct Upgraded {
+	/// This should be handed back to the client to complete the upgrade.
+	pub response: http::Response<()>,


We are actually only returning a set of headers, so I could conceivably return just those, and let the caller construct a Response themselves. Headers would be a more precise type, but also add a little more overhead to using (I expect people should just return this as-is pretty much)

dvdplm

I think this is useful; it's likely a common usage scenario and I think it's good to have a nice example for it. Left a few questions and suggestions.

src/handshake/http.rs

examples/hyper_server.rs

src/handshake/http.rs

examples/hyper_server.rs

src/handshake.rs

dvdplm · 2021-09-16T08:59:00Z

src/handshake/server.rs

@@ -126,6 +132,16 @@ impl<'a, T: AsyncRead + AsyncWrite + Unpin> Server<'a, T> {
 		self.socket
 	}

+	/// Configure the server based on the [`ServerConfiguration`] payload provided. This


I think we should be explicit about that it is the extensions that can be configured this way, and perhaps even mention that the deflate extension is the only one available?

My initial aim here was to have a fairly opaque object that may just be used for extensions or may be handy for other bits (because I wasn't sure whether we might want it to handle more), but I've been reading more about extensions, and have realised that:

I think being more specific here does make sense; it's all about Extension configuration and I don't think anything else will come into play!

I missed a piece of the puzzle! I realise (and it makes a lot of sense) that the server needs to respond with infomration on which extensions are agreed on, based in its configuration and the requested extensions, and that's a little awkward given the way things are currently split, so I'll need to think about that a little!

src/handshake/server.rs

jsdw · 2021-09-16T14:15:52Z

I actually ended up significantly reworking the code, because it turned out that my handling of extensions wasn't good enough!

Now, there is a handshake::http::Server which somewhat resembles the current handshake::Server, but is made to be given http::Requests and hand back appropriate http::Responses, rather than doing this dance internally.

The API is different where necessary so that it's possible to actually do the relevant dance (because the thing returned from hyper::upgrade::on doesn't resolve until a response is returned to the client, but I need it to be resolved if the Server requires a socket from the word go).

…imise diff

…ings

dvdplm

LGTM. :)

examples/hyper_server.rs

dvdplm · 2021-09-17T15:53:19Z

examples/hyper_server.rs

+	// Note: awaiting this won't succeed until the handshake response has been returned to the client, so this must be
+	// spawned on a separate task so as not to block that response being handed back.


This is good, without this comment I'd have missed how this works.

maciejhirsz

Looks clean, just some notes on the accept key encoding :)

maciejhirsz · 2021-09-21T08:50:37Z

src/handshake.rs

+// writes the response that's expected to be handed back in the response header `Sec-WebSocket-Accept`.
+//
+// See https://datatracker.ietf.org/doc/html/rfc6455#section-1.3 for more information on this.
+fn generate_accept_key<'k>(key_base64: &[u8; 24], output_buf: &'k mut [u8; 32]) -> &'k [u8] {


FYI: there is a WebSocketKey type alias defined here ([u8; 24]): the hash is always 16 bytes, and base64 encoding of it is always 24 bytes. You could skip the output_buf and just return WebSocketKey by value here.

Thanks for the pointer! I am now returning a 28 (not 32) byte array rather than taking in a buffer. Unless I'm mistaken, base64 encoding a 160bit hash will always use exactly 28 bytes!

maciejhirsz · 2021-09-21T08:51:14Z

src/handshake/http.rs

+		}
+
+		// Pull out the Sec-WebSocket-Key and generate the appropriate response to it.
+		let key: &[u8; 24] = match key.as_bytes().try_into() {


Could also use WebSocketKey alias here.

Add a feature and update hyper_example to make it easier to use Soket…

3f0fbda

…to with http types

jsdw commented Sep 14, 2021

View reviewed changes

dvdplm requested a review from maciejhirsz September 14, 2021 16:23

jsdw commented Sep 14, 2021

View reviewed changes

dvdplm reviewed Sep 16, 2021

View reviewed changes

jsdw added 2 commits September 16, 2021 14:09

rethink 1 (but it turns out this won't work..)

6c98072

Make a new Server interface instead so that we have an API we can use

8695672

jsdw added 3 commits September 16, 2021 15:16

remove the default_feature

17987b7

dedupe code in handshake and undo changes in handshake::server to min…

a0593b2

…imise diff

Add link to RFC explaining Sec-Websocket-Key

41d7678

jsdw changed the title ~~Making Soketto easier to use with http::Request types (and hyper)~~ Making Soketto easier to use with http types (and hyper) Sep 17, 2021

add basic logger to example so we can get log output for debugging th…

54ea9ce

…ings

dvdplm approved these changes Sep 17, 2021

View reviewed changes

jsdw added 2 commits September 17, 2021 17:18

Fix up a couple of comments

5e80e21

use log since we added a logger, and comment tweaks

e606700

maciejhirsz approved these changes Sep 21, 2021

View reviewed changes

jsdw added 4 commits September 21, 2021 14:04

use WebSocketKey alias

c851479

return 28 byte base64 encoding instead of passing buffer in

719a782

Tweak the docs a little

57dc84c

fix a typo

555ac9e

niklasad1 approved these changes Sep 23, 2021

View reviewed changes

jsdw merged commit 921d031 into develop Sep 23, 2021

jsdw deleted the jsdw-hyper-soketto branch September 23, 2021 11:13

This was referenced Sep 29, 2021

Release v0.7.0 (to develop, not master.. whoops) #50

Merged

Release v0.7.0 #51

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Making Soketto easier to use with `http` types (and `hyper`) #48

Making Soketto easier to use with `http` types (and `hyper`) #48

jsdw commented Sep 14, 2021 •

edited

Loading

jsdw Sep 14, 2021

jsdw Sep 14, 2021 •

edited

Loading

jsdw commented Sep 14, 2021

jsdw Sep 14, 2021

dvdplm left a comment

dvdplm Sep 16, 2021

jsdw Sep 16, 2021 •

edited

Loading

jsdw commented Sep 16, 2021 •

edited

Loading

dvdplm left a comment

dvdplm Sep 17, 2021

maciejhirsz left a comment

maciejhirsz Sep 21, 2021

jsdw Sep 21, 2021 •

edited

Loading

maciejhirsz Sep 21, 2021

		// Note: awaiting this won't succeed until the handshake response has been returned to the client, so this must be
		// spawned on a separate task so as not to block that response being handed back.

Making Soketto easier to use with http types (and hyper) #48

Making Soketto easier to use with http types (and hyper) #48

Conversation

jsdw commented Sep 14, 2021 • edited Loading

jsdw Sep 14, 2021

Choose a reason for hiding this comment

jsdw Sep 14, 2021 • edited Loading

Choose a reason for hiding this comment

jsdw commented Sep 14, 2021

jsdw Sep 14, 2021

Choose a reason for hiding this comment

dvdplm left a comment

Choose a reason for hiding this comment

dvdplm Sep 16, 2021

Choose a reason for hiding this comment

jsdw Sep 16, 2021 • edited Loading

Choose a reason for hiding this comment

jsdw commented Sep 16, 2021 • edited Loading

dvdplm left a comment

Choose a reason for hiding this comment

dvdplm Sep 17, 2021

Choose a reason for hiding this comment

maciejhirsz left a comment

Choose a reason for hiding this comment

maciejhirsz Sep 21, 2021

Choose a reason for hiding this comment

jsdw Sep 21, 2021 • edited Loading

Choose a reason for hiding this comment

maciejhirsz Sep 21, 2021

Choose a reason for hiding this comment

Making Soketto easier to use with `http` types (and `hyper`) #48

Making Soketto easier to use with `http` types (and `hyper`) #48

jsdw commented Sep 14, 2021 •

edited

Loading

jsdw Sep 14, 2021 •

edited

Loading

jsdw Sep 16, 2021 •

edited

Loading

jsdw commented Sep 16, 2021 •

edited

Loading

jsdw Sep 21, 2021 •

edited

Loading