Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
[RFC] rpc: optional context.Context in methods #8038
Conversation
|
NOTE: this will not be landing until after we branch for 2.3. I'm not concerned about breakage, but it's an unnecessary risk and can wait. |
rogpeppe
approved these changes
Nov 8, 2017
LGTM in general, modulo a few comments and suggestions. A lot of the changes look to me like they could be made in a separate PR - if this one only had the rpc package changes, things would be clearer, I think.
| const friendlyErrMsg = "error blocking on leadership release" | ||
| var result params.ErrorResult | ||
| + // TODO(axw) make it possible to plumb a context.Context |
rogpeppe
Nov 8, 2017
Owner
I'm not sure that making it possible to cancel arbitrary requests is a good idea. It would complicate the RPC protocol considerably (9p had cancellation, and it was by far the hardest part of the protocol to get right).
When we have the need for cancellation, it's possible to do what the watchers do - return a separate handle that can be blocked on and cancelled explicitly.
For the time being, perhaps just run the facade call in a separate goroutine and abandon it if we get cancelled - the amount of resources consumed is likely to be pretty small (even smaller on the client side if we resurrected net/rpc's Go method).
axw
Nov 9, 2017
Member
That sounds fine, though I don't see why it would be that hard? Send a message to say "cancel request N", which will just cancel the context for that request. The client doesn't necessarily need to wait for it to be cancelled, just for the request to be acknowledged.
Haven't thought about it too deeply. I'll reword the TODO to be less prescriptive.
| @@ -176,6 +182,7 @@ func (conn *Conn) Start() { | ||
| conn.mutex.Lock() |
rogpeppe
Nov 8, 2017
Owner
Wouldn't it be more appropriate for this method to take a Context as an argument rather than creating one from scratch? (or perhaps pass the context into NewConn)
| + // request returns. | ||
| + // | ||
| + // TODO(axw) provide a means for clients to cancel a request. | ||
| + ctx, cancel := context.WithCancel(conn.context) |
rogpeppe
Nov 8, 2017
Owner
I wonder if this might not be better inside methodCaller.Call, which would cancel the context a little sooner.
axw
Nov 9, 2017
Member
We're dealing with a MethodCaller interface here, so it would have to be as well as, not instead of. I'll add cancellation in there too.
wallyworld
approved these changes
Nov 10, 2017
I like the principal behind this. It looks sound but hard to know of subtle breakages. Would be good to address Rog's comments and land into fb and we can perform some more extensive testing.
axw
added some commits
Nov 1, 2017
axw
changed the base branch from
develop
to
state-controller-refactor
Nov 10, 2017
|
Rog's comments have been addressed. Retargeted to feature branch, so we can keep progressing this and land after branching for 2.3. |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://ci.jujucharms.com/job/github-merge-juju |
jujubot
merged commit 9c517c9
into
juju:state-controller-refactor
Nov 10, 2017
axw commentedNov 8, 2017
Description of change
This PR adds the capability to include a context.Context parameter in methods exposed via a juju/rpc connection. When the connection is closed or the request completes normally, the context will be cancelled.
The singular/leadership API calls are updated to use the context to cancel lease/leadership claims, rather than relying on the lease managers in state being killed. State.KillWorkers is removed. This change paves the way for moving the API server to the dependency engine, reducing the number of active MongoDB connections we maintain, and simplifying how we thread the state pool through to the introspection worker.
This change will also enable us to introduce OpenTracing calls to the API server, and later the API client.
QA steps
(wait)
Documentation changes
None.
Bug reference
None.