Skip to content
This repository has been archived by the owner on Sep 13, 2018. It is now read-only.

Generalize TcpServer::serve's service a bit #130

Merged
merged 1 commit into from
Jan 26, 2017

Conversation

alexcrichton
Copy link
Contributor

The services accepted by TcpServer::serve wire up the service's
request/response types the underlying request/response types of the BindServer
implementation. These types tend to be what we want for the simple case (non
streaming), but for the streaming case they have the
tokio_proto::streaming::Message type wired in.

The Message type typically isn't what libraries want as the request/response
types for their services, so this PR generalizes to use From and Into to
ensure that custom types can still be used so long as they can be converted.

I think this sould be backwards compatible as well (not causing inference
regressions), hopefully at least.

The services accepted by `TcpServer::serve` wire up the service's
request/response types the underlying request/response types of the `BindServer`
implementation. These types tend to be what we want for the simple case (non
streaming), but for the streaming case they have the
`tokio_proto::streaming::Message` type wired in.

The `Message` type typically isn't what libraries want as the request/response
types for their services, so this PR generalizes to use `From` and `Into` to
ensure that custom types can still be used so long as they can be converted.

I think this sould be backwards compatible as well (not causing inference
regressions), hopefully at least.
@alexcrichton
Copy link
Contributor Author

The generics got a little unwieldy here, but I wasn't sure why all the 'static bounds popped up...

@aajtodd
Copy link
Contributor

aajtodd commented Jan 19, 2017

Might be good to have an example of this (even if it's just a compile test).

@alexcrichton
Copy link
Contributor Author

Yeah the intention was that libraries would implement From<Message> and Into<Message> for their request/response types and could then use this builder, but unfortunately the motivation here (Hyper) still can't use this because it needs access to the remote address of the underlying TCP stream. As a result I'm not sure that we'd want to pursue this just yet, but curious on others' thoughts still.

@aturon
Copy link
Contributor

aturon commented Jan 23, 2017

LGTM.

@carllerche
Copy link
Member

@alexcrichton suggested holding off on this for now.

@alexcrichton
Copy link
Contributor Author

So over our lunch discussion today I'm actually inclined to go ahead and merge, @aturon @carllerche sound ok?

@aturon
Copy link
Contributor

aturon commented Jan 26, 2017

Yep, I think this is a step in the right direction.

@carllerche
Copy link
Member

Seems fine w/ me

@alexcrichton
Copy link
Contributor Author

Alright let's do this!

@alexcrichton alexcrichton merged commit 9e5aebb into tokio-rs:master Jan 26, 2017
@alexcrichton alexcrichton deleted the generalize branch January 26, 2017 22:26
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants