capnpc-go: more convienent API for calling methods

[zenhack]

The calling convention for clients is a bit funky, to say the least:

remoteObj.Method(ctx, func(pkg.RemoteObj_method_Params p) error {
         p.SetFoo("hello")
         p.SetBar("world")
         return nil
})

Would be nice if we could do something more like:

remoteObj.Method(ctx, "hello", "world")

I think the tradeoffs are similar to the pogs package, so maybe this would be a companion package to that. It would also be nice to be able use it with pogs without having to explicitly call Extract/Insert, but I'd want to find a way to avoid just making all of the args interface{}. Perhaps these wrappers could be generated along with the types from #63.

There's also a similar thing going on with the Server interfaces.

[zombiezen]

How would you deal with default arguments?

[zenhack]

We could just translate them mandatory arguments. They're a pretty rarely used feature in the sandstorm APIs, and not used at all in the schema that ship with capnproto itself. Given that, having to pass a sentinel value every now and then would fit with the go stdlib apis pretty seamlessly.

[zombiezen]

Right, but that means that you can make a backwards-compatible change in your schema and break call sites. That seems not ideal. If you were to go down this route, I would think that you'd want to generate two methods (name bikeshedding aside):

// This is the current Method
func (RemoteObjClient) Method(context.Context, func(RemoteObj_method_Params) error) RemoteObj_method_Results_Promise

// This is the newly generated Method which does not include any default arguments.
func (RemoteObjClient) MethodEasy(context.Context, foo string, bar string) RemoteObj_method_Results_Promise

I'm not convinced that there's an easier way to generate the server interfaces.

[zenhack]

Quoting Ross Light (2017-03-15 18:52:42)

Right, but that means that you can make a backwards-compatible change in your schema and break call sites. That seems not ideal.

It's worth noting we're already in a bad spot re: source compatibility. Every time I update the sandstorm schema, something has grown new methods and it breaks stuff. Also worth nothing that fixing the call-site breakage would be very mechanical (more so than the breakage we already deal with); you just pass defaults for any missing arguments. One could even write a tool to do this ala go fix.

If you were to go down this route, I would think that you'd want to generate two methods (name bikeshedding aside): // This is the current Method func (RemoteObjClient) Method(context.Context, func(RemoteObj_method_Params) err or) RemoteObj_method_Results_Promise // This is the newly generated Method which does not include any default argumen ts. func (RemoteObjClient) MethodEasy(context.Context, foo string, bar string) Remot eObj_method_Results_Promise

Do you mean just don't include the default arguments at all? I have more of a problem that; it means you just *can't* use the shorthand if you need to pass a non-default argument. If their use were more common, I might be convinced we needed a "just works" solution for source upgrades, but you're talking < 10 default 38 args in the entire sandstorm codebase. That's including things like backend.capnp that apps never really see anyway. I actually think just dealing with the breakage is better; it's a rare thing that will be caught at compile time and easy to fix (and could be mechanized by a tool if we wanted to write that).

…

I'm not convinced that there's an easier way to generate the server interfaces.

[zenhack]

@zombiezen, what do you think of using a builder? e.g:

remoteObj.Method().Foo("Hello").Bar("World").Call_(ctx)

I could actually see this scaling up to replace most of the current struct manipulation, but that's obviously hitting the issue of api breakage rather hard.

[zombiezen]

The C++ implementation uses the builder approach. I tried this initially, but the lifetime semantics are a bit fuzzier in Go and would increase generated code size, so I did not use it. I'm still leaning toward the basic/advanced method generation approach, but could be convinced differently.

[zenhack]

I started toying around with a builder-oriented API just to experiment, partial changes here:

zenhack@cf5f363

The builder-style methods aren't very different in size than the existing ones -- just an extra if statement, which is often going to be in lieu of one at a call-site.

The only slightly tricky bit was the error handling, which gets stored in the builder as opposed to returned on each call. But this makes the usage similar to the rpc, since the nature of promise pipelining induces the same kind of do everything then check for errors pattern.

Curious as to your thoughts on that sketch?

[zenhack]

@zombiezen, curious to hear what the problems you encountered were in more detail -- they're not obvious to me at all.

[zombiezen]

My thoughts on that sketch:

• The errors don't illuminate which fields are at fault. You could add more codegen to make them be more useful, but it's tricky to maintain and test. This has come up a few times before, and my answer has always been: use/improve the pogs package.
• The builder style eliminates using interfaces to set similar fields across struct types. This comes up from time to time, so I actually discourage the builder pattern in Go.

I don't think it really addresses this issue per se: I agree it would be really nice to call methods with simple arguments as if they were simple methods. I'm still leaning toward the idea in my earlier comment.

[zenhack]

The point about breaking use of interfaces is a good one; hadn't thought of that.

The other trouble with the earlier idea is it's not clear to me how (or if) it would extend to arguments that weren't "primitive types" -- so far all of our examples have just been strings/text, what if you have a struct argument? Coordinating with #63 is a potential solution. At that point, maybe put the pogs stuff in a separate generated package to avoid having the *Easy suffix, which is a bit awkward.

[zombiezen]

I'd be fine having a struct be passed in as the generated type and then do a copy. It seems a small price to pay.

[zenhack]

I've been thinking about some of this again lately. Question regarding the existing design: what's the point of the callback? As opposed to following the C++ library's design and having something like:

req := remoteObj.MethodRequest()
req.SetFoo("Hello")
req.SetBar("World")
req.Send_(ctx)

It's still a bit more verbose that is ideal, but is at least in line with the rest of the library, and feels a bit more natural to me.

[zombiezen]

I'll expand on my earlier remarks:

The C++ implementation uses the builder approach. I tried this initially, but the lifetime semantics are a bit fuzzier in Go and would increase generated code size, so I did not use it.

Recall that C++ implicitly calls a destructor when a request goes out of scope. The C++ implementation uses this to reuse arena memory between individual RPCs, something that's desirable for the Go implementation as well. While one could add an explicit Close(), the application has to remember to call this. Finalizers are not guaranteed to be called, so they are not a solution. Using a callback IMO makes the lifetime explicit and less error-prone.

(The other place where lifetimes need to be explicit, capabilities, are more complex and in the v3 branch you'll notice I developed an API around cloning handles but requiring each handle be Close()d.)

[zenhack]

Quoting Ross Light (2019-12-20 11:18:24)

The C++ implementation uses this to reuse arena memory between individual RPCs, something that's desirable for the Go implementation as well.

Aha, this is the bit that I was missing. Thanks for the clarification.

…

-Ian

[zenhack]

Coming back to this, re the idea of doing:

req := remoteObj.MethodRequest()
req.SetFoo("Hello")
req.SetBar("World")
req.Send_(ctx)

Rather than have a separate .Close()/.Release() on the request, we could just have Send() take ownership of it, so after the call to Send() req should not be used. This makes it impossible to forget to free the request.

@lthibault and I had a conversation about this the other day while debugging things (which resulted in #243), and both eventually came down really hard on the side of making this change, for a few reasons:

• The API is much more natural imo.
• While debugging, we observed that several of the ClientHook implementations hold locks while invoking PlaceArgs, which is a bug and actually theoretically can deadlock some app code I've written, though it wasn't rearing its head that day.
  • We could rework this to avoid holding locks during that callback, but this API change would obviate the whole invariant for that section of the code.
  • Especially given this is an invariant that implementations of a publicly exported interface have to care about, I'd really like to simplify this (though granted user supplied implementations of ClientHook are unlikely, but even currently we have enough different impls that it's a pain to track down).

The current design just seems very convoluted, and if we have Send() also take care of freeing the request, I don't see a huge downside. @zombiezen, do you see any other problems?

[zombiezen]

I think the general idea can be made to work. Only problem I see is that you still need to expose an explicit release operation for the case where the caller encounters an error building the message that stops it from sending the request. (Notice that you've omitted the error handling code for both SetFoo and SetBar in your example.) That was ultimately what pushed me over to the callback-based approach to avoid giving the user a footgun, but if the implementation cost is prohibitive, then maybe that's the tradeoff that needs to be made.

[zenhack]

Good point about the error handling -- though putting it in a callback doesn't force you to check errors from setters either; it's kindof a separable issue, but it's really easy to forget about this for setters, because they don't return a value you need, and the standard Arenas never fail under normal circumstances, so if you forget you're unlikely to notice without very extensive testing.

It would be nice if we could make this safer somehow.

Splitballing: what if arenas "remembered" allocation errors? Then, Send() could check the error on the message and refuse to send (but could still free the message) which would make this a bit more robust. (I think allocation errors are the only thing that can actually cause setters to fail; let me know if I'm wrong here).

[zenhack]

(It would probably still be a good idea to have an explicit release operator)

[zombiezen]

Good point about the error handling -- though putting it in a callback doesn't force you to check errors from setters either; it's kindof a separable issue, but it's really easy to forget about this for setters, because they don't return a value you need, and the standard Arenas never fail under normal circumstances, so if you forget you're unlikely to notice without very extensive testing.

Agreed, although I believe there are static analysis tools that find unchecked errors in Go.

Splitballing: what if arenas "remembered" allocation errors? Then, Send() could check the error on the message and refuse to send (but could still free the message) which would make this a bit more robust.

Potentially. I was giving the error case as the most common reason to avoid a send, but it is possible that a caller may want to drop the message under other circumstances as well. And an advanced caller may even deal with allocation errors and still want the message to send, who knows.

(I think allocation errors are the only thing that can actually cause setters to fail; let me know if I'm wrong here).

Struct setters are a little more complicated, because they can involve copying data so it requires doing a read. Setters also do some general checks before performing an allocation, so the set routines would need to cooperate with the arena in some way.

(It would probably still be a good idea to have an explicit release operator)

👍

[lthibault]

I find it helpful to start with something concrete, and Ian's suggestion reminds me of this errWriter pattern, so I thought I'd contribute a rough sketch of what this might look like.

call := g.Greet()     // begin method call
defer call.Finish()  // send the call iff no error and not aborted, and release the message

call.SetPerson(p)
call.SetTitle("Dr.")

// Optional:  check and log any errors
if err := call.Err(); err != nil {
    log.Println(err)
}

// Actually, nevermind... I don't like that guy...
call.Abort()  // as an implementation detail, this could set ErrAbort

I think this ticks most (all?) of the boxes mentioned in this discussion. 🤔

And an advanced caller may even deal with allocation errors and still want the message to send, who knows.

Might this be addressed with a SetErr(nil) method?

forgetting to do x...

The obvious issue is that users might forget to call Finish(), but we could guard against this using a finalizer that either:

1. Calls Finish() (which should probably be idempotent, anyway)
2. Logs this at a WARN level
3. Panics (probably a bit too extreme)

[zenhack]

What if we gave the request object a Release() method, which:

1. Releases the underlying message, if the request has not already been sent (if it has, Send() will have released it)
2. Also replaces the ReleaseFunc that method calls currently return.

This way, for common use cases you can just do defer req.Release(), and that covers you whether or not you actually send the message. combining it with the existing ReleaseFunc seems like it would make things more ergonomic.

[zenhack]

I opened a separate issue for the errWriter inspired API; I think these are orthogonal concerns.