Skip to content

Refactor JSRPC to prepare for capability-based RPC#1617

Merged
kentonv merged 9 commits intomainfrom
kenton/jsrpc-refactor
Feb 9, 2024
Merged

Refactor JSRPC to prepare for capability-based RPC#1617
kentonv merged 9 commits intomainfrom
kenton/jsrpc-refactor

Conversation

@kentonv
Copy link
Member

@kentonv kentonv commented Feb 4, 2024

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.

@kentonv kentonv requested a review from MellowYarker February 4, 2024 23:06
@kentonv kentonv requested review from a team as code owners February 4, 2024 23:06
@kentonv kentonv requested review from mikea and vickykont February 4, 2024 23:06
@kentonv kentonv force-pushed the kenton/jsrpc-refactor branch from 87ff769 to cd89398 Compare February 4, 2024 23:12
@kentonv kentonv force-pushed the kenton/jsrpc-refactor branch 2 times, most recently from 11caf28 to a256154 Compare February 6, 2024 23:41
Copy link
Contributor

@MellowYarker MellowYarker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@kentonv kentonv force-pushed the kenton/jsrpc-refactor branch from a32e7a6 to ccd1b47 Compare February 7, 2024 19:21
@kentonv
Copy link
Member Author

kentonv commented Feb 7, 2024

@kentonv kentonv force-pushed the kenton/jsrpc-refactor branch from ccd1b47 to 6c173d1 Compare February 7, 2024 19:24
// 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(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@kentonv kentonv force-pushed the kenton/jsrpc-refactor branch from 059e0d8 to cb57cc2 Compare February 9, 2024 19:47
@kentonv
Copy link
Member Author

kentonv commented Feb 9, 2024

Fixup: 059e0d8

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.
@kentonv kentonv force-pushed the kenton/jsrpc-refactor branch from cb57cc2 to 878f15b Compare February 9, 2024 20:09
@kentonv kentonv merged commit b5d5dee into main Feb 9, 2024
@kentonv kentonv deleted the kenton/jsrpc-refactor branch February 9, 2024 22:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants