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

Suggestion: Document Steer + Reconnect combo #625

Closed
yokomizor opened this issue Jan 8, 2022 · 1 comment
Closed

Suggestion: Document Steer + Reconnect combo #625

yokomizor opened this issue Jan 8, 2022 · 1 comment

Comments

@yokomizor
Copy link

Hi there,

I have a doc suggestion.

Background

Steer provides functionality to aid managing routing requests between Services.

Current Steer::poll_ready implementation polls all inner services beforehand:

fn poll_ready(&mut self, cx: &mut Context<'_>) -> Poll<Result<(), Self::Error>> {
loop {
// must wait for *all* services to be ready.
// this will cause head-of-line blocking unless the underlying services are always ready.
if self.not_ready.is_empty() {
return Poll::Ready(Ok(()));
} else {
if let Poll::Pending = self.services[self.not_ready[0]].poll_ready(cx)? {
return Poll::Pending;
}
self.not_ready.pop_front();
}
}
}

There has been some discussion about trade-offs and alternative strategies already:

And for the interested reader: the reason we need to poll all services is that we do not know at the time of the poll_ready which service the request subsequently sent to call will be for. We have two options. Either, we unconditionally accept the next element, and only then poll_ready on the relevant service, but then we are forced to buffer potentially an unlimited number of elements in case they are all for an unready service. Or, we get head-of-line blocking because every request requires all services to be ready. I suppose in theory there is a third option of trying to "guess" which service the next request will be for, but that still deteriorates to the "buffer indefinitely" in the worst case.

Originally posted by @jonhoo in #426 (comment)

With that said, I would like to add an extra "gotcha" that this implementation has. If one inner services fails to poll_ready, the Steer must be dropped, and all inner services must be recreated, which could be expensive and complex. Steer is this service that becomes more valuable the more services it routes. Unfortunately, more services also means higher chances for poll_ready to fail, and more expensive and complex drop/recover becomes, kind of cancelling out its motivation.

My suggestion

I think one way around this could be Steer<Reconnect<MakeService<BoxService>>>. The idea being that MakeService::poll_ready "most likely" has a smaller chance to fail. I think documenting this idea and the reasoning can be helpful for users considering Steer.

Today's doc suggests Steer<BoxService>, which works, but could be expensive depending on how many inner services you have and how fragile these are.

cheers

@yokomizor
Copy link
Author

Ok, after implementing my suggestion, I realized I was gluing components in a way that works, but also in way that was not supposed to be. I mean, I had to fake a Target for Reconnect, and things got pretty ugly. Anyway, closing this suggestion.

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

No branches or pull requests

1 participant