Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement Clone for ChainedInterceptor #542

Closed
c-thiel opened this issue Mar 26, 2024 · 1 comment
Closed

Implement Clone for ChainedInterceptor #542

c-thiel opened this issue Mar 26, 2024 · 1 comment

Comments

@c-thiel
Copy link

c-thiel commented Mar 26, 2024

First of all thanks for this great create!

The Problem

Tonic Service Client are designed to be cloned in Multi-Threading/Multiplexing scenarios. There is a section "Multiplexing-Requests" on this in the tonic docs: https://docs.rs/tonic/latest/tonic/transport/struct.Channel.html#multiplexing-requests

As a result a tonic client implements Clone. When using a client build with this crate, a XXXServiceClient<InterceptedService<TonicChannel, ChainedInterceptor>> is returned. Everything except ChainedInterceptor implements Clone.

I need to use the client in a Multiplexing scenario and require the client to implement a lightweight clone to avoid opening a large number of channels.

The Solution

... is not as trivial as I would have though because ServiceAccountInterceptor requires mutability for the token. I changed it to use Interior Mutability instead.

I also had to introduce a new Interceptor trait which I don't like.
The only reason for the new interceptor trait is the ChainedInterceptor chain method which runs calls via the original Interceptor trait on &mut self. This breaks my interior mutability approach though.

I would argue that even if you decide to stick to the old implementation of the ChainedInterceptor , the changed ServiceAccountInterceptor and AccessTokenInterceptor are better off using interior mutability and can still be used in clone-able clients even though we couldn't use the ClientBuilder to obtain them.

PR is coming in a second, let me know if you have any better ideas!

@buehler
Copy link
Collaborator

buehler commented Aug 29, 2024

Done via #566

@buehler buehler closed this as completed Aug 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

2 participants