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

LoadBalancer related traits are not object safe #275

Closed
jamesmunns opened this issue Jun 11, 2024 · 1 comment
Closed

LoadBalancer related traits are not object safe #275

jamesmunns opened this issue Jun 11, 2024 · 1 comment
Assignees
Labels
enhancement New feature or request

Comments

@jamesmunns
Copy link
Contributor

What is the problem your feature solves, or the need it fulfills?

The traits necessary for LoadBalancer, such as SelectionAlgorithm and BackendSelection are not object safe. This makes it not possible to use dyn Trait for implementations for this.

In river, we'd like to have runtime-selected Load Balancing, Health Checks, and Service Discovery options. This makes it difficult to determine how to properly create an instance of LoadBalancer with types chosen at runtime

Describe the solution you'd like

Ideally, if it were possible to rework these traits to be object safe, it would be possible to create something like:

LoadBalancer<dyn BackendSelection<Iter = WeightedIterator<dyn SelectionAlgorithm>>>

Describe alternatives you've considered

I'm not certain this is a great way of doing things! It's possible I could work around the lack of object safety using my own constructions, but I wanted to bring this up in case this was an oversight, or if you had other ideas for how this could be implemented.

I'm going to dig a little deeper to see if there are changes I could propose to get the traits object safe, or if there are fundamental mismatches due to how the trait is actually used in the existing examples. Open to any pointers!

Additional context

This is to support the river project, CC memorysafety/river#39

@drcaramelsyrup drcaramelsyrup added the enhancement New feature or request label Jun 13, 2024
@jamesmunns
Copy link
Contributor Author

Closing this, but noting what I found:

  • Making these traits object safe is likely unreasonably hard to do, BUT
  • It's probably not necessary to make these object safe.

The trick that I missed is that it is probably better to just have your Service type be generic over the same generics as LoadBalancer, e.g.:

impl<BS> ProxyHttp for RiverProxyService<BS>
where
    BS: BackendSelection + Send + Sync + 'static,
    BS::Iter: BackendIter,

This is because pingora::Server::add_services() takes Vec<Box<dyn Service>> - essentially we are already fully erasing the type at the service level! This makes it unnecessary to do type erasure at the Service level itself (or for it's components).

Feel free to point people this way if they think that they want object safety for these traits in the future :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants