segmentio
Support context
Even though the standard library's net/rpc doesn't support contexts, we should because they're getting more popular and others have asked for it. The big question is-- Should we have a Context suffixed method for every RPC endpoint, or should we make a breaking change and have context.Context as the first argument across the board?
I don't feel strongly. It's easy to add context.TODO() if you don't care about contexts in your caller, but I also don't want to trouble people too much by making a breaking change... though it may be a good opportunity for me (and others) to learn gofix.
(Added assignees for feedback)
Since you're dealing with generated code I don't think the breaking change is much of an issue.
gofix is the way to go 👍 translate client.Method(...) into client.Method(context.TODO(), ...) is probably not too hard.
Also I +💯 the initiative, we need contexts everywhere!
I would suggest having 2 methods, one without the context client.Method(...) and client.MethodWithContext(ctx, ...). If not, it's going to break all the applications using the clients. I don't think it's a good idea to force update your code because we are adding an extra feature in a library.
support context != context or nothing ;)
I actually prefer having one generated Go function per RPC method gofix method to keep things simple, but does it feel wrong to make the New method in the generated clients require context.Context in its interface when this is not the case in the standard library?
If we're even OK with enforcing client support for contexts, what should the interface of the client we ask the user for look like?
type Client interface {
Call(method string, args interface{}, reply interface{}) error
}
Right now, it's like so but not sure if the Call method should take context, or if it should be CallContext? What RPC client with context support are we using now? I'd like to follow that...
client template - https://github.com/segmentio/glue/blob/master/templates/client.gohtml
If we have a decently big installed client base, we should be careful about breaking compatibility. Given that we're generating code, we can generate a new client that has an interface which supports contexts.
How does the standard lib handle reporting back infrastructure-layer information (e.g. metrics or on-the-wire message sizes) without using a context ?
Wild idea but since you have a code generator, you could make it generate either version. Add a command line argument to enable one version or the other.
I doubt that there would be code out there that need both version of the methods in the same project.
How does the standard lib handle reporting back infrastructure-layer information (e.g. metrics or on-the-wire message sizes) without using a context ?
@gesterhuizen The standard library comes with a very bare RPC server and doesn't handle that.
@achille-roussel Most of our services commit a client package with the generated code so that wouldn't be inline with our workflow since whether or not to use context is the caller's choice.
But the caller could regenerate its own client if it didn't want context support right? Passing context.TODO() is also trivial if you have code that's not context-ready, for this reason, I wouldn't worry much about breaking changes or offering only the context-based API.
Just FYI, since the standard net/rpc package is frozen, a fork of that package at github.com/keegancsmith/rpc adds context.Context support to rpc.Server and rpc.Client types with cancellations.
It extends Call method with a context parameter, so Client interface would become
type Client interface {
Call(ctx context.Context, method string, args interface{}, reply interface{}) error
}
and client wrapper functions also take additional context.Context parameter as in,
func (c *Math) Sum(ctx context.Context, args math.SumArg) (*math.SumReply, error) {
reply := new(math.SumReply)
err := c.RPC.Call(ctx, "Math.Sum", args, reply)
return reply, err
}
