Refactor JSRPC to prepare for capability-based RPC#1617
Conversation
87ff769 to
cd89398
Compare
11caf28 to
a256154
Compare
MellowYarker
left a comment
There was a problem hiding this comment.
This is a great refactor! I want to spend a bit more time looking at the membrane related commits since it's still fairly new to me but generally this PR makes sense.
a32e7a6 to
ccd1b47
Compare
ccd1b47 to
6c173d1
Compare
| // The return value of the function is a promise that resolves once the remote returns the result | ||
| // of the RPC call. | ||
| return RpcFunction([this, methodName=kj::str(name)] | ||
| kj::Maybe<JsRpcCapability::RpcFunction> JsRpcCapability::getRpcMethod( |
There was a problem hiding this comment.
May be missing something, but is this method actually needed? AFAICT we only call Fetcher::getRpcMethod(), and never actually construct a JsRpcCapability, we just call the static JsRpcCapability::sendJsRpc(). It seems like all we actually need is the static method, which could just be a function in worker-rpc.c++ instead, no?
Judging by some comments in worker-rpc.h I figure it may be relevant in the future when we're able to pass capabilities around, but I'm wondering if the intention is to use this class then, or just put more stuff on Fetcher? If we're going to use Fetcher then this class isn't needed anymore.
There was a problem hiding this comment.
Sorry, started replying to this earlier then got distracted.
Currently, there's no way to construct a JsRpcCapability, but that will change soon. This is the type we will use when people pass an object with methods as one of the parameters or results of some other RPC call. The receiving end will get a JsRpcCapability that makes RPCs back to the sender.
059e0d8 to
cb57cc2
Compare
|
Fixup: 059e0d8 |
This will enable easier reuse soon.
If, after the initial JS RPC call is complete, some capabilities have been exchanged between the client and server and are still held, we want the session to remain open until those go away. That means that `GetJsRpcTargetCustomEventImpl::run()` should not complete in the meantime. To accomplish this, we use some capnp membranes.
I decided the old name was confusing given that the method does not return until the session is done. The new name also makes the name of the CustomEvent implementation a lot nicer, as is a better operation name for Jaeger tracing purposes. Note that the rename does not affect the Cap'n Proto communications on the wire, since the ordinal numbers remain the same.
The `RevokerMembrane` in `JsRpcSessionCustomEventImpl::sendRpc()` will already take care of propagating errors.
The `WorkerRpc` type can now be used to represent capabilities received across RPC. It couldn't do that while also extending `Fetcher`. `Fetcher` itself now has its own wildcard property that invokes its own version of `getRpcMethod()`. `Fetcher`'s version starts a new session, whereas `WorkerRpc::getRpcMethod()` now operates within an existing session. An alternative design would have been to keep `WorkerRpc` as it is, and introduce a new separate class to represent non-top-level capabilities. This is something we debated before. I felt that it made more sense to push this down into `Fetcher` because: - The existence of `WorkerRpc` was already preventing `Fetcher` from adding new methods, since they would override any RPC method with the same name. So having RPC pulled into a separate subclass didn't really avoid affecting `Fetcher`. I like putting these issues all in one place. - I think it will be cumbersome to mark bindings on the client side as supporting RPC or not, so that we can decide whether to make them a `WorkerRpc` vs. `Fetcher`. I'd rather they just all have the same type and the endpoint throws an error from `jsRpcSession` if it doesn't do RPC. Note that I think we should rename `Fetcher` to something that makes clear it does more than just fetch. I don't have a great idea for a name yet, though.
This better-reflects its new meaning.
cb57cc2 to
878f15b
Compare
This is a series of small refactorings that prepares for the future where RPC params or return values can contain new RPC objects. For this to work, we need to disentangle the sending of one RPC call from the creation of RPC sessions, since one session may encompass many calls in both directions.