-
Notifications
You must be signed in to change notification settings - Fork 2k
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
context/cancel mechanism for RPC #7119
Comments
👍 I tried this once upon a time, and I vaguely remember a complicating factor being the in-memory/local RPC implementations we use (possibly just in tests)? It could be that that just made it unsuitable for my use case and therefore not worth the effort while working on something else. Feel free to sneak it in opportunistically if an opportunity presents itself. |
@schmichael Its cool that you have some experiences in implementing cancel for rpc. What did you use for this, which library? and how you solve the problem that yamux doesn;t have Context support? |
@tantra35 I tried for Nomad but did not complete it successfully, so it never shipped. Yamux lacking context can be generally solved with a goroutine+Close call:
This pattern works for net.Conns and Yamux sessions, but is not guaranteed to work for all io.Closers. For example some os.File implementations are not guaranteed to unblock readers and writers when File.Close is called. The trickier part is that we need to check Yamux's CloseChan anywhere we might block on the server to avoid wasting resources servicing a request that has been cancelled. This is something we should probably already be doing in some places today as its not context-specific. I forget how easily statestore/memdb blocking queries are cancelled. Hopefully there's a straightforward approach there. |
@schmichael i think that goroutine approach is not fully equivalent of cancel, due with cancel, stream not mandatory may be closed, and why yamux library can't be expand for context support? Its relatively easy add method And what you think about https://github.com/keegancsmith/rpc as a replacement for |
In the work being done for CSI (in particular #6903) we've found that there are RPC calls that can block for very long periods of time, which in busy clusters with lots of volumes could lead to resource pressure on goroutines. It might be helpful to have a way to pass a cancellation context to RPC calls so they can be timed out.
We're not considering this blocking for CSI and it's probably not exclusive to CSI, so deserves a little more consideration than something we'd stick into the CSI Controller RPCs.
The text was updated successfully, but these errors were encountered: