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

tower-buffer probably doesn't need current_message slot #158

Open
carllerche opened this issue Feb 11, 2019 · 5 comments
Open

tower-buffer probably doesn't need current_message slot #158

carllerche opened this issue Feb 11, 2019 · 5 comments
Labels
A-buffer Area: The tower "buffer" middleware C-request Category: A non-feature request, e.g.: metadata updates, optimization suggestion, etc. I-slow Problems and improvements related to performance. S-blocked Status: marked as blocked ❌ on something else such as a PR or other implementation work.

Comments

@carllerche
Copy link
Member

Instead of having a current_message slot, perhaps the worker should "join" on the service and the request queue being ready.

Thoughts @jonhoo

@jonhoo
Copy link
Collaborator

jonhoo commented Feb 12, 2019

I'm not entirely sure what that would look like?

@carllerche
Copy link
Member Author

Ah, I meant to add more info, but forgot...

For context, I'm working on putting a PR together that removes DirectService.

But, roughly, this function would start with:

try_ready!(self.rx.poll_ready());
try_ready!(self.service.poll_ready());

Now I realize this would require additional APIs on mpsc::Receiver, but it should be possible!

@jonhoo
Copy link
Collaborator

jonhoo commented Feb 12, 2019

Ah, I see what you mean. Yes, I think that could work, but only if you assume that a Service cannot become NotReady after it has reported Ready (I suppose the current code would also break if that were the case). I'm not sure why you need the self.rx.poll_ready though?

@carllerche
Copy link
Member Author

You are correct. We do not need rx.poll_ready.

I believe it is a fair guarantee that once Service::poll_ready returns Ready, it must always return Ready until Service::call is called.

@jonhoo jonhoo added A-buffer Area: The tower "buffer" middleware C-request Category: A non-feature request, e.g.: metadata updates, optimization suggestion, etc. I-slow Problems and improvements related to performance. labels Mar 31, 2020
@jonhoo
Copy link
Collaborator

jonhoo commented Mar 31, 2020

I was about to implement this, when I realized that this will also trigger the issue pointed out in #408/#412. If we poll_ready on the Service before we know we have a request to send it, then we may be "holding up" a slot in the service (such as in a Buffer) forever with no way of giving it back. So I think we should leave this as-is for the time being until a disarm mechanism is added.

@jonhoo jonhoo added the S-blocked Status: marked as blocked ❌ on something else such as a PR or other implementation work. label Mar 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-buffer Area: The tower "buffer" middleware C-request Category: A non-feature request, e.g.: metadata updates, optimization suggestion, etc. I-slow Problems and improvements related to performance. S-blocked Status: marked as blocked ❌ on something else such as a PR or other implementation work.
Projects
None yet
Development

No branches or pull requests

2 participants