Add handler for unknown method #405
Conversation
|
This is as barebones as it gets. Lets start picking at it. Interface? yes/no/good enough anyone? :) |
|
Could use a test, but I'm fine with this. Before we tag 3.0 I think I want to do the interface reworking we talked about though. |
|
Ah yeah, +1 to the test. Also, @zenhack, can you open an issue for the interface refactor? I think you're in the best position to accurately spec it out. |
lthibault
changed the title
|
I've flagged some minor things, but this basically LGTM. I think we're good to merge after these fixes. |
server/server_test.go
Outdated
| return &sm | ||
| } | ||
| blankBoot := capnp.NewClient(srv) | ||
| lis, err := net.Listen("tcp", ":0") |
There was a problem hiding this comment.
You can simplify this and entirely skip setting up a connection with something on the other side and just use blankBoot in place of blankClient
There was a problem hiding this comment.
@zenhack wouldn't that invoke 'Send' instead of 'Recv'? The hook was intended for acting as a proxy forwarder and it doesn't exist in Send.
I do wonder about the use-case for sending on blankBoot directly. I've seen this in other testcases and tried it out only to find it doesn't invoke Recv. When is this workflow applicable? It looks to be intended for the client side?
In the use-case I have, the client side uses the actual client for the intended method in order to build the proper message. It is send to the gateway/proxy which looks up the service that implements that method and forwards it. This allows intercepting the message and do some middleware stuff like authentication and logging, and of course forwarding to the actual capability provider. It works like a charm!
There was a problem hiding this comment.
O.O can't believe I missed this on my first look: Yeah, it would, and you really don't want the two to have different behavior -- the only difference that's supposed to be there is who allocates the arguments. Probably we should move the logic for method lookup into Server.start so it's not duplicated in both Send and Recv.
There was a problem hiding this comment.
Indeed it looks like there is some room for cleanup here as well. Combine methods.find, although that might not be as easy as it sounds.
{rant} Looking at the Send() function the return statement is rather confusing. I tried reading it a few times and am still not quite sure what is happening in what order. I've been bitten in the past by statements that do more than one thing. In this case the return statement of Send() is akin to a rattlesnake den. The casual reader will benefit from having it split into a statement for each of the things it is doing. It looks like it needs the result of methods.find before srv.start is called but this is not quite clear to me.
Would it be okay if I add the hook to Send() in the same way it is added to Recv and leave the cleanup to more capable folks? I'd rather leave the test case as is to make sure the hook is tested with Recv, since the primary use-case is forwarding the request on receive.
|
Quoting Henk (2023-01-03 00:10:07)
The casual reader will benefit from having it split into a statement
for each of the things it is doing.
Yeah, I agree; it would be better to split it up into multiple
statements.
It looks like it needs the result of methods.find before srv.start is
called but this is not quite clear to me.
Yeah, I think that's fixable without too much major surgery, but
Would it be okay if I add the hook to Send() in the same way it is
added to Recv and leave the cleanup to more capable folks?
I'd be ok with that; I won't force you to shave that yak.
|
… the client, as pointed out by @zenhack.
Thank you. |
|
I'd still like to remove the connection from that test, but other than that LGTM |
This is a proposed solution to be able to customize the handling of unknown methods. The default behavior is unchanged but a hook allows customized behavior.
The hook can be used to dynamically create a method and have it invoked as if it existed all along.
I was able to use this in a proxy service that dynamically obtains capabilities/methods from other services and forwards requests to those services.