Skip to content
This repository has been archived by the owner on Apr 16, 2020. It is now read-only.

Support no-Clone generated services created by custom MakeService impls #152

Open
mzabaluev opened this issue Apr 6, 2019 · 5 comments
Open

Comments

@mzabaluev
Copy link
Contributor

As currently emitted by the server generator, the service trait requires Clone and the server object implements the Service<()> impl to satisfy MakeService by cloning itself. While simple, this precludes implementations where the service factory provides instances with call-specific data, such as synchronization objects or connection-level attributes. While it may be possible to use the extension map of the HTTP Request (once that is made available on the gRPC request), this is an unnecessarily dynamic approach with performance drawbacks.

It would be nice to have a build flag that removes the Clone requirement and the self-replicating MakeService impl, leaving the application to provide the service factory as appropriate.

This could help with #67 and may facilitate #2.

@mzabaluev
Copy link
Contributor Author

mzabaluev commented Apr 6, 2019

A side note: it feels weird that one type can implement different parameterizations of Service doing very different things, the purpose of which is not completely decided by the parameter type. IMHO it would be conceptually cleaner if the service input was an associated type, so the type creating service instances out of seed data would have to be provided separately from the service type itself. But perhaps there is another case for types serving closely related polymorphic requests.

@seanmonstar
Copy link
Contributor

The generated Service<()> is really to help get it working quickly with tower-h2, but it's not the only way to get started. You make YourMaker and implement a way to make the Service for each connection.

To your point about parameterization: the request type used to be an associated type, but there's some reasons it was changed to a generic:

  1. As an associated type, there could only be 1 impl for the service, even though a service could theoretically work for many kinds of requests.
  2. In Rust, it's more common for "input" types to be generics, and "output" types to be associated types.

If you think about Service being an extension of FnMut, it might seem less weird that different arguments can return different types.

@mzabaluev
Copy link
Contributor Author

You make YourMaker and implement a way to make the Service for each connection.

The Clone prerequisite of the generated Service<()> can still be a drag in case there have to be non-cloneable instance data.

@mzabaluev
Copy link
Contributor Author

mzabaluev commented Apr 9, 2019

If you think about Service being an extension of FnMut, it might seem less weird that different arguments can return different types.

It's not very common to see explicit implementations of FnMut; in fact, the desugared trait signature is unstable. Much less to have one type act as a closure in very different semantic roles.

I understand the case for polymorphism on conceptually similar inputs, as also demonstrated by the generated server struct implementing Service for requests on different protocol layers. But the co-located Service<()> factory impl exhibits a semantic creep caused by the idea that the only tool you should have is a Service. I would rather change MakeService to not have the blanket impl, but have a generic function that accepts a Service<Target> to produce an adapter type implementing MakeService.

@mzabaluev mzabaluev changed the title Support custom MakeService impl Support no-Clone generated services created by custom MakeService impls Apr 9, 2019
@seanmonstar
Copy link
Contributor

I do think it'd be a good idea to generate a Make$NameServer that is the "make service", instead of implementing it on $NameServer directly.

In order to completely remove Clone, deeper changes would be required, since in order to return a Future, we need to clone the service, as we don't yet have a deserialized request type to be able to call the specific method.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants