feat(server): support upgrading to other protocols #1287

spinda · 2017-08-12T04:20:30Z

The Http type learns a new method, bind_upgradable_connection, which returns a Future. This method behaves like bind_connection, except the Service supplied must return an UpgradableResponse containing either an HTTP Response or some information describing a protocol switch. If an Upgrade response is returned, the server task is shut down (without closing the underlying io) and this information is forwarded to the Future, along with the underlying io and any remaining buffered data that has already been read from it.

If the server task shuts down normally, the Future completes with a None output value. In the case of an error handling the connection, the error is also forwarded to the Future.

This allows for implementing servers with hyper that can upgrade their connections to different protocols, such as WebSockets, in response to client requests. Such protocols can be implemented outside of hyper itself. The API is designed to allow for potentially multiple upgrade paths out of an HTTP exchange, with the upgrade information attached to the UpgradableResponse::Upgrade signal determining which is used.

A companion crate implementing WebSocket upgrades can be found here, with sample code here.

Importantly, the code path for normal, non-upgradable connections is entirely untouched by this addition.

The commit messages match the guidelines in https://github.com/hyperium/hyper/blob/master/CONTRIBUTING.md#git-commit-guidelines

Allow unwrapping the connection to its inner I/O object and read buffer, if the write buffer is empty.

The Http type learns a new method, bind_upgradable_connection, which returns a Future. This method behaves like bind_connection, except the Service supplied must return an UpgradableResponse containing either an HTTP Response or some information describing a protocol switch. If an Upgrade response is returned, the server task is shut down (without closing the underlying io) and this information is forwarded to the Future, along with the underlying io and any remaining buffered data that has already been read from it. If the server task shuts down normally, the Future completes with a None output value. In the case of an error handling the connection, the error is also forwarded to the Future. This allows for implementing servers with hyper that can upgrade their connections to different protocols, such as WebSockets, in response to client requests. Such protocols can be implemented outside of hyper itself. The API is designed to allow for potentially multiple upgrade paths out of an HTTP exchange, with the upgrade information attached to the UpgradableResponse::Upgrade signal determining which is used. Importantly, the code path for normal, non-upgradable connections is entirely untouched by this addition.

This makes it easier to send a "101 Switching Protocols" before upgrading from HTTP to another protocol.

seanmonstar · 2017-08-18T01:33:35Z

This looks really impressive! I apologize I haven't had time to review it, I've been trying to wrap up changes to the http crate before RustConf. I'll take a look soon, I promise!

spinda · 2017-08-18T01:42:47Z

No problem, and thanks!

spinda · 2017-08-31T06:20:56Z

Hi again, have you had a chance to look at this since RustConf?

seanmonstar · 2017-09-02T18:26:59Z

@spinda I'm so sorry, I don't have particularly good reasons other than forgetting. I'd like to look at this this week. I think upgrading protocols is necessary, and so hyper should definitely support it.

cc @carllerche

Instead of printing Server { core: "..." } we print Server { core: ... } This clarifies the situation, that a field is unprintable and hence has been omitted, instead of looking like the `core` field contains a string.

seanmonstar

Alright, I got a chance to look through this. This is solid work, thanks!

I like that it's generic over the upgrade protocol: it doesn't matter what is actually being upgraded to, the user just receives the socket back and can do whatever they want.

I started thinking about if there were alternative ways to do this, and this is something I came up with:

Instead of needing to use a special method on Http, and a special enum return type, I wondered if this could be done while still using a normal Service, and still returning a Response. What if a method or constructor of Response were just used, with a sort of callback like interface.

It could use a futures::sync::oneshot channel internally (but perhaps newtyped to hide that detail), and a user can send the completion future anywhere they want. For example:

impl Service for HttpUpgrades {
    fn call(&self, req: Request) -> Self::Future {
        // omitting inspection of Request, details irrelevant
        let (resp, on_upgraded) = Response::upgrade();
        self.ws_queue.send((details, on_upgraded));
        future::ok(resp)
    }
}

The on_upgraded argument would be a impl Future<Item=(T, Bytes)> of some kind.

Perhaps a constructor doesn't make sense, and it should instead be a mutator of the Response, that's fine.

What do you think of an API like this?

seanmonstar · 2017-09-06T22:03:34Z

src/server/mod.rs

+
+type UpgradableResponseHead<P> = Result<__ProtoResponse, (P, Option<__ProtoResponse>)>;
+
+impl<T, B, P> ServerProto<(T, UpgradableSender<T, B, P>)> for UpgradableHttp<P, B>


That all of these boilerplate impls are needed is a bummer. Sorry to have to write that XD

seanmonstar · 2017-09-06T22:04:39Z

tests/upgradable_server.rs

+}
+
+#[test]
+fn test_upgrade() {


seanmonstar · 2017-09-06T22:05:55Z

src/server/mod.rs

+    Response(Response<B>),
+    /// A protocol upgrade signal with accompanying information and optional
+    /// "switching protocols" HTTP response head.
+    Upgrade(P, Option<Response<()>>),


So, what exactly is the P? From what I can tell, is it just some arbitrary thing that might be created when inspected a Request, and useful once upgraded?

spinda · 2017-09-08T22:31:16Z

Thanks for taking a look! Not sure how much time I'm going to have over the next few days (ending an internship and moving back home). I'll address these comments as soon as I can.

seanmonstar · 2017-09-08T22:59:50Z

I'd be especially interested in discussing the design before spending time writing code :D

seanmonstar · 2017-09-16T04:07:09Z

I put more details of the proposal I suggested in #1323.

blueridanus · 2017-11-17T10:38:00Z

How's progress on this? Can I help?

nayato · 2017-11-21T00:00:55Z

is this now blocked on moving away from tokio-proto?

seanmonstar · 2017-11-28T23:11:36Z

The internal change to not use tokio-proto is already merged, just not on by default while bugs are still found. It should probably be easier to make upgrades work, however, because of this work.

I'd say the blocker for this feature is settling on an API to expose to a user.

seanmonstar · 2018-02-21T23:00:46Z

I'm closing this due to inactivity. I recommend discussing the feature more in #1323.

spinda force-pushed the upgradable branch from fca6c34 to 06497ad Compare August 12, 2017 19:48

spinda mentioned this pull request Aug 17, 2017

[WIP] Chrome DevTools Protocol implementation servo/servo#18133

Closed

12 tasks

spinda added 4 commits August 17, 2017 17:23

feat(http): add Conn::try_into_inner

e5eb3ea

Allow unwrapping the connection to its inner I/O object and read buffer, if the write buffer is empty.

feat(server): allow sending a head with an upgrade

d2bad09

This makes it easier to send a "101 Switching Protocols" before upgrading from HTTP to another protocol.

test(server): add tests for upgradable connections

fa9ca95

spinda force-pushed the upgradable branch from 06497ad to fa9ca95 Compare August 18, 2017 00:25

spinda mentioned this pull request Aug 31, 2017

Update to support Hyper 0.11? websockets-rs/rust-websocket#132

Open

fix(server): print ... instead of "..." in Debug

d54407b

Instead of printing Server { core: "..." } we print Server { core: ... } This clarifies the situation, that a field is unprintable and hence has been omitted, instead of looking like the `core` field contains a string.

seanmonstar reviewed Sep 6, 2017

View reviewed changes

Eijebong mentioned this pull request Jan 21, 2018

Update hyper to 0.12 servo/servo#19825

Closed

seanmonstar closed this Feb 21, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(server): support upgrading to other protocols #1287

feat(server): support upgrading to other protocols #1287

spinda commented Aug 12, 2017

seanmonstar commented Aug 18, 2017

spinda commented Aug 18, 2017

spinda commented Aug 31, 2017

seanmonstar commented Sep 2, 2017

seanmonstar left a comment

seanmonstar Sep 6, 2017

seanmonstar Sep 6, 2017

seanmonstar Sep 6, 2017

spinda commented Sep 8, 2017

seanmonstar commented Sep 8, 2017

seanmonstar commented Sep 16, 2017

blueridanus commented Nov 17, 2017

nayato commented Nov 21, 2017 •

edited

Loading

seanmonstar commented Nov 28, 2017

seanmonstar commented Feb 21, 2018


		type UpgradableResponseHead<P> = Result<__ProtoResponse, (P, Option<__ProtoResponse>)>;

		impl<T, B, P> ServerProto<(T, UpgradableSender<T, B, P>)> for UpgradableHttp<P, B>

feat(server): support upgrading to other protocols #1287

feat(server): support upgrading to other protocols #1287

Conversation

spinda commented Aug 12, 2017

seanmonstar commented Aug 18, 2017

spinda commented Aug 18, 2017

spinda commented Aug 31, 2017

seanmonstar commented Sep 2, 2017

seanmonstar left a comment

Choose a reason for hiding this comment

seanmonstar Sep 6, 2017

Choose a reason for hiding this comment

seanmonstar Sep 6, 2017

Choose a reason for hiding this comment

seanmonstar Sep 6, 2017

Choose a reason for hiding this comment

spinda commented Sep 8, 2017

seanmonstar commented Sep 8, 2017

seanmonstar commented Sep 16, 2017

blueridanus commented Nov 17, 2017

nayato commented Nov 21, 2017 • edited Loading

seanmonstar commented Nov 28, 2017

seanmonstar commented Feb 21, 2018

nayato commented Nov 21, 2017 •

edited

Loading