lack of Clone trait on grpc::Client v.0.6

[przygienda]

grpc::CLient used to be :Clone on v.0.4 and the removal of that (why? http client inside is Arc!) breaks lots of APIs that on creation consume a grpc client. This forces now to open an HTTP connection for every client in an API where before it could be generated by with_client so multiple services could be run on same HTTPbis ...

any rationale?

I will fork 0.6.0 and add the Clone derive & try to upstream but some sane remedy would be appreciated ...

[stepancheg]

I'm not sure what's the proper design, my thoughts are these.

It is simply not necessary and complicates implementation a little (Clone for the client requires putting fields of the client into an Arc).

If you need to access client from multiple threads, you can put Client into Arc<Client>, because Client is Sync anyway.

BTW API/design/implementation feedback is always welcome.

[przygienda]

Unfortunately, this being my first thought, it doesn't work. The generated code necessitates a straight grpc::Client move into the API so you can't carry stuff around in Arc since it cannot be moved out ... I have multiple services on the same Client, it's unresolvable now ... Unless you change the generated rust backend code on the with_client calls to take an Arc or something ...

I pushed the change towards you, passes all tests and works fine ...

--- tony

On Fri, Oct 26, 2018 at 6:09 PM Stepan Koltsov notifications@github.com wrote:

I'm not sure what's the proper design, my thoughts are these.

It is simply not necessary and complicates implementation a bit.

If you need to access client from multiple threads, you can put Client into Arc, because Client is Sync anyway.

BTW API/design/implementation feedback is always welcome.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/stepancheg/grpc-rust/issues/132#issuecomment-433578872, or mute the thread https://github.com/notifications/unsubscribe-auth/ABo0C8KNeQpOOKlw0Ii07HSgIwksnVp_ks5uo7JLgaJpZM4X9Keb .

[przygienda]

it would be tad more elegant to internally Cow the host string if Clone is allowed of course but that's just internal cosmetics ...

On Fri, Oct 26, 2018 at 6:23 PM Tony Przygienda tonysietf@gmail.com wrote:

Unfortunately, this being my first thought, it doesn't work. The generated code necessitates a straight grpc::Client move into the API so you can't carry stuff around in Arc since it cannot be moved out ... I have multiple services on the same Client, it's unresolvable now ... Unless you change the generated rust backend code on the with_client calls to take an Arc or something ...

I pushed the change towards you, passes all tests and works fine ...

--- tony

On Fri, Oct 26, 2018 at 6:09 PM Stepan Koltsov notifications@github.com wrote:

I'm not sure what's the proper design, my thoughts are these.

It is simply not necessary and complicates implementation a bit.

If you need to access client from multiple threads, you can put Client into Arc, because Client is Sync anyway.

BTW API/design/implementation feedback is always welcome.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/stepancheg/grpc-rust/issues/132#issuecomment-433578872, or mute the thread https://github.com/notifications/unsubscribe-auth/ABo0C8KNeQpOOKlw0Ii07HSgIwksnVp_ks5uo7JLgaJpZM4X9Keb .

[stepancheg]

The generated code necessitates a straight grpc::Client move into the API

Nope. I've just double-checked in v6.0 branch, generated code is:

pub struct GreeterClient {
    grpc_client: ::std::sync::Arc<::grpc::Client>,
    method_SayHello: ::std::sync::Arc<::grpc::rt::MethodDescriptor<super::helloworld::HelloRequest, super::helloworld::HelloReply>>,
}

impl ::grpc::ClientStub for GreeterClient {
    fn with_client(grpc_client: ::std::sync::Arc<::grpc::Client>) -> Self {
        GreeterClient {
            grpc_client: grpc_client,
            method_SayHello: ::std::sync::Arc::new(::grpc::rt::MethodDescriptor {
                name: "/helloworld.Greeter/SayHello".to_string(),
                streaming: ::grpc::rt::GrpcStreaming::Unary,
                req_marshaller: Box::new(::grpc::protobuf::MarshallerProtobuf),
                resp_marshaller: Box::new(::grpc::protobuf::MarshallerProtobuf),
            }),
        }
    }
}

impl Greeter for GreeterClient {
    ...
}

[przygienda]

ok, look like I possibly did run the build.rs with a different rust grpc than the main package extern ... investigating ...

[przygienda]

ok, yepp, let me close the pullup. That was the reason. Nast'ish stuff. Maybe putting into generated code some indication which version of grpc rust generated the backend would be helpful. Would allow compile time checks ...

[stepancheg]

Maybe putting into generated code some indication which version of grpc rust generated the backend would be helpful

https://github.com/stepancheg/grpc-rust/issues/134

Also, I'm thinking about making generated code with a different version of grpc should be a compile-time error, like this:

grpc:

const VERSION_0_6_0: () = ();

generated_code:

const _VERSION: () = grpc::VERSION_0_6_0;

This is possible to implement, however, this will require client code regeneration in minor versions even if the generated code is compatible.