[Breaking] Add support for arbitrary transports. #27

tikue · 2016-02-23T08:13:36Z

Quite a bit of machinery added:

Listener trait
Dialer trait
Stream trait
Transport trait

I made a lot of types default to Tcp* for:

Convenience
Backwards compatibility (though it's still a breaking change in some respects)

~~Before merging this, we should impl the traits for unix_socket to make sure the trait is compatible.~~
I've now impled Transport for Unix sockets:

Server.spawn(UnixTransport(path))
Client::new(UnixDialer(path))

This is a breaking change but only for code that used ToSocketAddrs args other than &str. Code using &str to specify address will still work.
To update code:
Server.spawn(socket_addr) => Server.spawn(TcpTransport(socket_addr))

Bikeshed

As I played around with unix sockets, I grew frustrated with the second-class nature of them WRT tarpc. Specifically, having to use spawn_with_config just because the transport changed, even though I was using default Config settings, was annoying. In light of that, I propose making spawn take a Transport, and just go ahead and impl<A: ToSocketAddrs> Transport for A { ... } such that it creates a tcp transport. Even though other transports use sockets, I think it's reasonable to have a sane default here for the sake of ergonomics.

If you think that's reasonable, I'll add that to this PR, as well.

Update: the above was implemented as well, but due to coherence, I can only do impl<'a> Transport for &'a str. Specialization might allow Transport impls for all ToSocketAddrs types, but maybe not.

Fixes #16

Quite a bit of machinery added: * Listener trait * Dialer trait * Stream trait * Transport trait

tikue · 2016-02-23T16:24:25Z

tarpc/src/macros.rs

@@ -569,6 +581,20 @@ mod functional_test {
        assert_eq!(3, client2.add(1, 2).get().unwrap());
    }

+    #[test]
+    fn async_try_clone_unix() {
+        let handle = Server.spawn_with_config(UnixTransport("/tmp/test"),


This won't work if you run it twice because the file already exists. We can just delete it at the end I guess?/Choosing something like /tmp/tarpc-async_try_clone_UNIX?

Use this: https://crates.io/crates/tempdir? I think that generates random directories, so you can do Tempdir::new().unwrap().path().join("file");

I don't think tempdir exposes the path? I'll double check.
On Feb 24, 2016 7:40 PM, "shaladdle" notifications@github.com wrote:

In tarpc/src/macros.rs
#27 (comment):

@@ -569,6 +581,20 @@ mod functional_test {
assert_eq!(3, client2.add(1, 2).get().unwrap());
}

#[test]

fn async_try_clone_unix() {

let handle = Server.spawn_with_config(UnixTransport("/tmp/test"),

Use this: https://crates.io/crates/tempdir? I think that generates random
directories, so you can do Tempdir::new().unwrap().path().join("file");

—
Reply to this email directly or view it on GitHub
https://github.com/google/tarpc/pull/27/files#r54045195.

Hm, I must have been looking at a different crate last time. Tempdir should work.

tikue · 2016-02-23T17:52:31Z

tarpc/src/transport/unix.rs

+        self.accept().map(|(stream, _)| stream)
+    }
+    fn dialer(&self) -> io::Result<UnixDialer<PathBuf>> {
+        self.local_addr().map(|addr| UnixDialer(addr.as_pathname().unwrap().to_owned()))


I shouldn't be unwrapping here, and we shouldn't merge as-is. I just haven't thought about the proper error handling yet. Thoughts?

Maybe like this:

self.local_addr().map(|addr| { match addr.as_pathname() { Some(path) => Ok(UnixDialer(path.to_owned())), None => Err(io::Error::new(io::ErrorKind::Other, "tried to create unix socket with invalid path")), } })

Basically copied from TcpStream verbatim.

Also, make spawn() take a Dialer, but impl Dialer for str, defaulting to TCP transport.

shaladdle · 2016-02-28T22:25:53Z

README.md

@@ -47,7 +47,7 @@ impl HelloService for HelloServer {

 fn main() {
    let server_handle = HelloServer.spawn("0.0.0.0:0").unwrap();
-    let client = hello_service::Client::new(server_handle.local_addr()).unwrap();
+    let client = hello_service::Client::new(server_handle.dialer()).unwrap();


Maybe we should have the example in the readme just use a specific IP address? Like localhost:10000 or something? Then people don't have to learn about dialers if they don't care about them.

Alternatively, we could have the server handle have a method that returns a client.

server_handle.new_client().unwrap();

Thoughts?

My thoughts were more that the readme example should be the most common simple case. I think it's more common to have a client/server in different processes, rather than in the same one. So, I was thinking that using an address would be more familiar to some people.

A separate issue is that we don't really have a full suite of examples.. Just the benchmark. That should be addressed separately though.

Returning client seems ok too. The one advantage of having a Dialer is that you can send bogus stuff over a stream, which is useful for verifying that we don't misbehave if someone sends junk to the server.

I'm fine making the example simpler. The handle should have two methods,
one returning Dialer and one creating a new client.
On Mar 2, 2016 8:47 PM, "shaladdle" notifications@github.com wrote:

In README.md
#27 (comment):

@@ -47,7 +47,7 @@ impl HelloService for HelloServer {

fn main() {
let server_handle = HelloServer.spawn("0.0.0.0:0").unwrap();

let client = hello_service::Client::new(server_handle.local_addr()).unwrap();

let client = hello_service::Client::new(server_handle.dialer()).unwrap();

My thoughts were more that the readme example should be the most common
simple case. I think it's more common to have a client/server in different
processes, rather than in the same one. So, I was thinking that using an
address would be more familiar to some people.

A separate issue is that we don't really have a full suite of examples..
Just the benchmark. That should be addressed separately though.

Returning client seems ok too. The one advantage of having a Dialer is
that you can send bogus stuff over a stream, which is useful for verifying
that we don't misbehave if someone sends junk to the server.

—
Reply to this email directly or view it on GitHub
https://github.com/google/tarpc/pull/27/files#r54835799.

Sounds fine

shaladdle · 2016-03-03T05:29:21Z

tarpc/src/transport/tcp.rs

+
+/// A transport for TCP.
+pub struct TcpTransport<A: ToSocketAddrs>(pub A);
+impl<A: ToSocketAddrs> super::Transport for TcpTransport<A> {


Add a line in between the struct and the impl?

I'll try but it might be from cargo fmt.
On Mar 2, 2016 9:29 PM, "shaladdle" notifications@github.com wrote:

In tarpc/src/transport/tcp.rs
#27 (comment):

@@ -0,0 +1,62 @@
+use std::io;
+use std::net::{SocketAddr, TcpListener, TcpStream, ToSocketAddrs};
+use std::time::Duration;
+
+/// A transport for TCP.
+pub struct TcpTransport<A: ToSocketAddrs>(pub A);
+impl<A: ToSocketAddrs> super::Transport for TcpTransport {

Add a line in between the struct and the impl?

—
Reply to this email directly or view it on GitHub
https://github.com/google/tarpc/pull/27/files#r54837589.

I don't think so. Line 19 on unix.rs has a line between them.

tikue · 2016-03-17T04:51:44Z

All comments addressed. Ready for re-review, I think.

coveralls · 2016-04-25T01:36:17Z

Coverage increased (+0.8%) to 84.044% when pulling 4d636d2 on tikue:listener into 3693c95 on google:master.

tikue added 3 commits February 22, 2016 23:50

Add support for arbitrary transports.

fcdb0d9

Quite a bit of machinery added: * Listener trait * Dialer trait * Stream trait * Transport trait

Make Tcp* default types

a0e6147

Merge branch 'master' of github.com:google/tarpc into listener

2d1a77e

tikue changed the title ~~Add support for arbitrary transports.~~ [Breaking] Add support for arbitrary transports. Feb 23, 2016

tikue added 2 commits February 23, 2016 01:13

Reorgnize modules

28c6c33

Add a test

396aec3

tikue reviewed Feb 23, 2016
View reviewed changes

Move generic bounds to where clause

d5b2f23

tikue reviewed Feb 23, 2016
View reviewed changes

tikue added 8 commits February 24, 2016 20:26

Create temp file using tempdir in test

774411c

Better documentation for Stream.

8307c70

Basically copied from TcpStream verbatim.

Remove Addr associated type of Dialer.

03dc512

Also, make spawn() take a Dialer, but impl Dialer for str, defaulting to TCP transport.

Why were we wrapping the service in an arc?

c398e23

Fix examples

9827f75

rustfmt

6273ebe

Reformat some generic bounds

a1f529f

Don't unwrap in Listener::dialer

714541a

tikue assigned shaladdle Feb 25, 2016

tikue added 5 commits February 25, 2016 22:28

Update README to list arbitrary transports as a feature.

8e5a44b

Update deps versions.

72a9f8f

Simplify lib.rs example

5ac4b71

Merge branch 'master' into listener

9dcd38c

Merge branch 'master' into listener

e995acd

shaladdle reviewed Feb 28, 2016
View reviewed changes

shaladdle reviewed Mar 3, 2016
View reviewed changes

tikue added 7 commits March 16, 2016 20:36

Add a documentation note on addresses

5d27d34

Add a line in between struct and impl

aec1574

Cargo fmt

b0495eb

Generify doc comment

6a6832c

Add a line between impl and struct

f33cb3b

Simplify readme example

bbfb432

Add spaces between items in impls.

709f4ab

Merge master into listener.

4d636d2

shaladdle merged commit 6ce3a3d into google:master Apr 25, 2016

tikue deleted the listener branch September 16, 2016 23:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Breaking] Add support for arbitrary transports. #27

[Breaking] Add support for arbitrary transports. #27

tikue commented Feb 23, 2016

tikue Feb 23, 2016

shaladdle Feb 25, 2016

tikue Feb 25, 2016

tikue Feb 25, 2016

tikue Feb 25, 2016

tikue Feb 23, 2016

shaladdle Feb 25, 2016

tikue Feb 25, 2016

shaladdle Feb 28, 2016

tikue Feb 28, 2016

shaladdle Mar 3, 2016

tikue Mar 3, 2016

shaladdle Mar 3, 2016

shaladdle Mar 3, 2016

tikue Mar 3, 2016

shaladdle Mar 3, 2016

tikue commented Mar 17, 2016

coveralls commented Apr 25, 2016

[Breaking] Add support for arbitrary transports. #27

[Breaking] Add support for arbitrary transports. #27

Conversation

tikue commented Feb 23, 2016

Bikeshed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tikue commented Mar 17, 2016

coveralls commented Apr 25, 2016