JSRPC: Service bindings and stateless class syntax #1658

kentonv · 2024-02-11T00:03:06Z

This fully supports RPC to service bindings, as opposed to Durable Objects.

Actually, service bindings already worked, by accident, as of #1617.

But this change does the following:

Makes sure env and ctx get passed to RPC methods.
Introduces an alternative class-based syntax for named entrypoints. If you export a class that extends StatelessService, we now understand it to be a stateless entrypoint, not a Durable Object. Meanwhile, you can extend DurableObject to explicitly mark an exported class as being a Durable Object class. Both of these are imported from the new module "cloudflare:entrypoints".
Enables RPC server-side for classes that extend these new base classes, without the need for the js_rpc compat flag. (experimental is still needed for now.)
Implements the new policy that only class methods (prototype properties) are exposed. Instance properties are not available over RPC. This is to defend people who forget to use private properties from accidentally exposing things over RPC that weren't intended.
Adds "constructor" to the reserved name list. I was wrong about this before, it needs to be reserved. Sorry.

src/workerd/api/worker-rpc.h

jasnell · 2024-02-11T00:20:27Z

src/workerd/jsg/setup.h

+    // TODO(cleanup): This should return `JsFunction`, but there is no such type. We only have
+    //   `jsg::Function<...>` (or perhaps more appropriately, `jsg::Constructor<...>`), but we
+    //   don't actually know the function signature so that's not useful here. Should we add a
+    //   `JsFunction` that has no signature?


Yeah, I intentionally didn't add JsFunction because I wanted make sure we thought through the relationship with jsg::Function. We could, for instance have something like jsg::Function<JsValue(kj::ArrayPtr<JsValue> args)> or we could have a generic jsg::JsFunction. Not sure which is the best approach. We don't need to solve that now, tho.

kentonv · 2024-02-11T01:04:11Z

(sorry, bad rebase, fixing)

kentonv · 2024-02-11T01:05:05Z

Fixups:

kentonv · 2024-02-11T01:32:51Z

Windows fixup:

8bfb157

Dunno why only needed on Windows.

kentonv · 2024-02-11T01:41:44Z

Fixup for missing return, not sure why only the internal build caught it.

07bc414

kentonv · 2024-02-11T02:43:11Z

Uhh, another fixup that undoes two previous fixups and tries a different way... 😅

e32defb

geelen

I only have bikesheddy things to suggest, like not liking cloudflare:entrypoints (I think it should be cloudflare:workers) and thinking Service is fine rather than StatelessService (since it's being imported, not a global), but I don't feel all that strongly.

geelen · 2024-02-12T07:03:08Z

src/workerd/api/tests/js-rpc-test.js

+  #counter = 0;
+
+  constructor(state, env) {
+    super(state, env);


Is the superclass making properties with these names? I feel like it should for convenience, and because there's no risk of exposing them over RPC accidentally.

Either way, for this class, you can omit the constructor entirely

I think we still don't want to:

We are going to allow exposing properties as long as they are on the class, not instance properties. So we'd have to make special exceptions for these which is weird.

I don't like them being automatically public to local code, even if they aren't exposed over RPC.

I am not sure the names state and env (and ctx for stateless) are the best names. We've always used them in docs but nothing in the API actually mandates these specific names today. I'd rather not have to think about this.

geelen · 2024-02-12T07:10:59Z

src/workerd/api/tests/js-rpc-flag.js

+import assert from 'node:assert';
+
+// This class does not extend `DurableObject`, but because we have the `js_rpc` compat flag on,
+// it'll still accept RPC.


You deleted the "js-rpc-disabled" test, should something like that still exist?

As of this commit, the main js-rpc-test no longer has the flag set, and tests all the behavior around that, making js-rpc-disabled redundant. I did however introduce js-rpc-flag which is the opposite test: it tests behavior when the flag is enabled.

geelen · 2024-02-12T07:19:53Z

src/cloudflare/internal/entrypoints.d.ts

+}
+
+export class StatelessService {
+  public constructor(ctx: unknown, env: unknown);


The order of these arguments makes me deeply uncomfortable, but I see why you've done it.

Btw I don't think this is doing what you expect, TS is weird around asserting constructor arguments. However we might want to specify that both of these attach properties to their instance, if that's what they're going to do (otherwise why are they passed to super?)

Yes, the ordering is awkward but it seems ore important to be consistent with DurableObject than to be consistent with the freestanding functions.

The superclass constructor requires these arguments (despite doing nothing with them) for a couple reasons:

If the subclass doesn't declare a constructor, the superclass constructor will receive these params by defalut, so it seems like the right thing to do.

I want to reserve the right to do something with them in the future even if we don't today.

geelen · 2024-02-13T04:57:08Z

src/cloudflare/internal/workers.d.ts

I actually kinda think the internal file could still be entrypoints, and the public cloudflare:workers file could export a bunch of stuff from a few cloudflare-internal files?

Maybe but it doesn't matter, the file is internal and ideally I'd like to remove it entirely when built-in modules (defined in C++) start supporting named exports properly.

src/workerd/api/worker-rpc.c++

Tests failing. Will review again once things are green

kentonv · 2024-02-13T18:45:51Z

^ Fixed test failure

kentonv · 2024-02-16T20:54:34Z

Added commits covering recent discussions, this should now be up to date with the current consensus.

jasnell · 2024-02-16T21:32:28Z

src/cloudflare/internal/workers.d.ts

+  public constructor(ctx: unknown, env: unknown);
+
+  public ctx: unknown;
+  public env: unknown;


Are these readonly?

jasnell · 2024-02-16T21:36:01Z

src/workerd/api/worker-rpc.c++

+
+  jsg::JsObject self(args.This());
+  self.set(js, "ctx", jsg::JsValue(args[0]));
+  self.set(js, "env", jsg::JsValue(args[1]));


I'm wondering if these shouldn't be defined as readonly instance properties on the instance. I also wonder if these should be marked non-enumerable. The former being more important.

Eh, I don't feel the need for these to be anything other than plain old properties as you'd get by assigning them in JS. They exist for the benefit of the subclass, so if the subclass decides to overwrite them, whatever.

jasnell

LGTM with one question relative to whether the fields should be readonly.

Er, umm... well actually, service bindings already worked before this commit. However, they did not pass the `env` and `ctx` parameters. This commit makes it so we inject those two parameters between the first and second params. Obviously, this insertion is pretty ugly. However: * We need `env` and `ctx` to be passed _somewhere_. * We would like to achieve backwards-compatibility so that existing handlers for things like queues and scheduled workers can just be RPC in the future. * All existing handlers use exactly three arguments, where the second and third are `env` and `ctx`. * We cannot say "env and ctx are appended after whatever the client sendss" because this would very likely allow a client to trick the server into using objects provided by the client instead of `env` and `ctx`, by sending additional arguments when the server didn't expect them. * In practice, we'll encourage people (through documentation) to use exactly one argument. We'll probably leave it undocumented that additional arguments go after `ctx`. * We will also introduce an alternative syntax using classes that allows multiple-argument methods, with `env` and `ctx` being passed to the class constructor instead.

This contains the classes `DurableObject` and `StatelessService`. Top-level class exports should inherit one of these two classes to indicate what kind of export it is.

The Own is faked currently, but this supports the upcoming introduction of stateless handler classes where a new instance is constructed for each event. Miraculously, most callers don't need any changes, because they put the result on the stack, and then they pass it into a function that expects `Maybe<ExportedHander&>`, which is an allowed implicit conversion.

This isn't needed since `v8::Local<v8::Map>` can already implicitly convert to `v8::Local<v8::Object>`, which is already allowed. In fact, the presence of this constructor actually _blocked_ other implicit conversions, such as from `v8::Local<v8::Function>`, since they would try to implicitly convert to `v8::Local<v8::Map>` instead of to `v8::Local<v8::Object>`.

E.g. comparing a `JsObject` to another `JsObject` would give some warning about "C++ standard says this is ambiguous even though it obviously isn't" or whatever. This seems to make the warning go away.

This returns the class constructors for `StatelessService` and `DurableObject`, which we'll need in order to be able to check whether a particular exported class extends one of them.

Works like: ``` export class MyService extends StatelessService { constructor(ctx, env) { this.ctx = ctx; this.env = env; } async fetch(req) { return new Response("Hello world!"); } } ``` For every request, a new instance of the class will be constructed. Since `env` and `ctx` are passed to the constructor, they do not need to be passed to individual methods. This means in particular that JSRPC methods can have multiple arguments without awkwardly inserting `env` and `ctx` between them. The constructor argument order is `ctx, env` in order to be consistent with Durable Object classes, which use that ordering.

I admit, I was wrong about this. I thought `constructor` was just another name inherited from `Object` and not actually the same thing as the class constructor. However, it turns out that every class declaration does indeed implicitly define a method called `constructor`. We do not want to expose this over RPC!

This requires creating a bypass on the client side that lets us specify these methods without inadvertently getting the respective method on the stub.

We will automatically enable JS RPC on the server side if the entrypoint class extends `DurableObject` or `StatelessService`. We'll also allow it for bare object entrypoints, because this seems harmless. Historical Durable Object classes which did not extend `DurableObject` will not accept RPC, for fear of exposing methods that weren't intended to be exposed. The `js_rpc` compat flag can still be used to enable RPC on all Durable Object classes, in order to avoid breaking anyone who is already playing with this functionality, but the flag may eventually go away.

Specifically, if the function's `.length` attribute is more than 3, we assume the last two declared args are the `env, ctx`. Note: The next commit will actually prohibit more than one arg, but I wanted to have this implementation in place in the history first, in case we decide to use it later.

…ed RPC. We enforce both that the server-side function is declared with (no more than) three args, and that the client sends exactly one. We could revert this commit later if we like the idea of inferring the position of `env, ctx` from arity.

This mainly extends the tests, but I made the error messages more specific to benefit the test. This proves that `env` and `ctx` are instance properties and not prototype properties, since prototype properties would be accessible over RPC and it's really important that `env` is not.

- Consolidates Service binding documentation to be within the Runtime APIs section of the docs. Currently docs for configuring a Service binding, and docs for how to write code around Service bindings are in separate sections of the docs, which makes getting started hard, requires jumping back and forth between pages. Relevant content from [the configuration section](https://github.com/cloudflare/cloudflare-docs/blob/production/content/workers/configuration/bindings/about-service-bindings.md) has been moved here, and will be removed. - Explains what Service bindings are and what to use them for. - Provides separate code examples RPC and HTTP modes of working with Service bindings. refs cloudflare/workerd#1658, cloudflare/workerd#1692, cloudflare/workerd#1729, cloudflare/workerd#1756, cloudflare/workerd#1757

* [do not merge] Revamp Service Bindings docs for RPC - Consolidates Service binding documentation to be within the Runtime APIs section of the docs. Currently docs for configuring a Service binding, and docs for how to write code around Service bindings are in separate sections of the docs, which makes getting started hard, requires jumping back and forth between pages. Relevant content from [the configuration section](https://github.com/cloudflare/cloudflare-docs/blob/production/content/workers/configuration/bindings/about-service-bindings.md) has been moved here, and will be removed. - Explains what Service bindings are and what to use them for. - Provides separate code examples RPC and HTTP modes of working with Service bindings. refs cloudflare/workerd#1658, cloudflare/workerd#1692, cloudflare/workerd#1729, cloudflare/workerd#1756, cloudflare/workerd#1757 * Remove Service bindings config page, update links, redirects * Apply suggestions from code review * Further consolidate bindings content within Runtime APIs, link from config section * Redirect from config bindings to Runtime APIs bindings * Update links to point to Runtime APIs bindings section * Fix redirects * Fix linter warnings * Bold bullet points for Service Bindings explainer * Add missing bindings to /runtime-apis/bindings * Add env vars and secrets links to /runtime-apis/bindings/ section * Update content/workers/runtime-apis/bindings/ai.md * Update content/workers/runtime-apis/bindings/service-bindings.md * Apply suggestions from code review Co-authored-by: Matt Silverlock <[email protected]> * Break docs into RPC and HTTP sections * Moving over more docs * Fix titles * Fixes * More docs * More, need to break apart into pages * more * fixup * Apply suggestions from code review Co-authored-by: Michael Hart <[email protected]> Co-authored-by: Kenton Varda <[email protected]> * Remove unnecessary changes * Create RPC and Context sections * Rename to /visibility * Edits * Fix naming * Edits * Add note about Queues to context docs * Clarify language in RPC example * Clarify service binding performance * Link to fetch handler in describing HTTP service bindings * Move remaining content over from tour doc * Add limits section, note about Smart Placement * Edits * WorkerB => MyWorker * Edits plus partial * Update content/workers/runtime-apis/bindings/service-bindings/rpc.md * Edits * Making sure internal doc covered, minus Durable Objects docs * Remove extraneous section * Call out RPC lifecycle docs in Service Bindings section * Update content/workers/runtime-apis/rpc/lifecycle.md * Edits to JSRPC API docs (#13801) * Clarify structured clonability. - Let's not talk about class instances being an exception to structured clone early on. Instead, we can have an aside later down the page. Most people probably wouldn't even expect structured clone to treat classes this way anyway, so telling the about it just to tell them that we don't do that is distracting. - Adjust the wording in anticipation of the fact that we're likely to add many more types that can be serialized, and this list will likely not keep up. The important thing here is to explain the types that have special behavior (they aren't just data structures treated in the obivous way). - Briefly describe these special semantics in the list, to get people excited to read more. * Minor wording clarification. It was confusing whether "object" here meant a plain object or a class instance. * Clarify garbage collection discussion. The language here was not very precise, and would have confused people who have a deep understanding of garbage collectors. * Better link for V8 explicit resource management. The previous link pointed to a mailing list thread of messages generated by the bug tracker. Let's link directly to the bug tracker. * Fix typo. * Clarify language about disposers. The language here said that stubs "can be disposed" at the end of a using block, which implies that they might not be, or that this is some sort of hint to the GC. It's more accurate to say that they *will* be disposed, that is, their disposer *will* be called, completely independent of GC. The advice about when to use `using` was unclear. I've changed the advice to simply say that the return value of any call should be stored into `using`, which is an easy rule to follow. * Remove "Sessions" section from lifecycle. This section was placed under "automatic disposal" but doesn't seem to belong here. I don't think it's really necessary to define a "session" unless we are then going to say something about sessions, but the word doesn't appear anywhere else on the page. Sessions are closely related to execution contexts, but execution contexts were already described earlier. * Clarify section on automatic disposal. * Correct docs on promise pipelining. The previous language incorrectly suggested that promise pipelining would kick in even if the caller awaited each promise. In fact, it is necessary to remove the `await` in order to get the benefits. * Fix reserved methods doc. The doc incorrectly stated that `fetch` and `connect` were special on `RpcTarget` in addition to `WorkerEntrypoint` and `DurableObject`. This is not correct. The doc didn't cover the numerous other reserved method names. * elide -> omit Co-authored-by: Brendan Irvine-Broque <[email protected]> --------- Co-authored-by: Brendan Irvine-Broque <[email protected]> * Apply suggestions from code review Co-authored-by: Greg Brimble <[email protected]> Co-authored-by: Kenton Varda <[email protected]> * Apply suggestions from code review Co-authored-by: Greg Brimble <[email protected]> * More RPC doc tweaks: Move stuff around! (#13808) * Move section on proxying stubs to compatible-types. This isn't really lifecycle-related. But it is another kind of thing that can be sent over RPC. * Move "promise pipelining" to compatible-types under "class instances". Promise pipelining isn't really about lifecycle. I think it fits under "class instances" because it is motivated by class instances. * Merge compatible-types into RPC root doc. The compatible-types list ends up highlighting the key exciting features of the RPC system. This should be at the root. * Tweak RPC root page. I'm removing "How it Works" with the function example because: 1. The example itself doesn't really explain "how it works". 2. We now present this same example down the page under "Functions". * Add changelog entry * Update content/workers/runtime-apis/rpc/lifecycle.md Co-authored-by: Greg Brimble <[email protected]> * More more JSRPC doc tweaks (#13840) * Add documentation for `rpc` compat flag. * Update links to about-service-bindings. * Update content/workers/_partials/_platform-compatibility-dates/rpc.md * Update content/workers/runtime-apis/rpc/_index.md Co-authored-by: James M Snell <[email protected]> * Update content/workers/_partials/_platform-compatibility-dates/rpc.md Co-authored-by: James M Snell <[email protected]> * Named entrypoints (#13861) * Named entrypoint configuration in `wrangler.toml` * Named entrypoints example * Apply suggestions from code review * Apply suggestions from code review --------- Co-authored-by: Brendan Irvine-Broque <[email protected]> * Apply suggestions from code review * Clarify RPC unsupported errors (#13863) * * Add Durable Objects RPC docs (#13765) * Update DO counter example with RPC * Clarify RPC pricing * Rename "Configuration" to "Best Practices" section * Fix some redirects (#13865) * Order the RPC docs sections in nav (#13866) * Fix links * Fix more redirects * Fix DO redirect in Versions & Deployments * fix merge conflict --------- Co-authored-by: Matt Silverlock <[email protected]> Co-authored-by: Michael Hart <[email protected]> Co-authored-by: Kenton Varda <[email protected]> Co-authored-by: Greg Brimble <[email protected]> Co-authored-by: James M Snell <[email protected]> Co-authored-by: Vy Ton <[email protected]> Co-authored-by: Rita Kozlov <[email protected]> Co-authored-by: Rita Kozlov <[email protected]>

kentonv requested review from geelen, jasnell and MellowYarker February 11, 2024 00:03

kentonv requested review from a team as code owners February 11, 2024 00:03

kentonv requested a review from ObsidianMinor February 11, 2024 00:03

jasnell reviewed Feb 11, 2024

View reviewed changes

src/workerd/api/worker-rpc.h Outdated Show resolved Hide resolved

jasnell reviewed Feb 11, 2024

View reviewed changes

kentonv force-pushed the kenton/jsrpc-service-binding branch from 0811199 to 6cd287c Compare February 11, 2024 01:02

kentonv requested a review from a team as a code owner February 11, 2024 01:02

kentonv force-pushed the kenton/jsrpc-service-binding branch from 6cd287c to 0811199 Compare February 11, 2024 01:03

kentonv removed request for a team and ObsidianMinor February 11, 2024 01:03

kentonv force-pushed the kenton/jsrpc-service-binding branch from 0811199 to 5f258c3 Compare February 11, 2024 01:04

kentonv force-pushed the kenton/jsrpc-service-binding branch from 8bfb157 to 50c8bbd Compare February 11, 2024 01:32

kentonv force-pushed the kenton/jsrpc-service-binding branch from 07bc414 to 2861e49 Compare February 11, 2024 01:41

kentonv force-pushed the kenton/jsrpc-service-binding branch from e32defb to 77635b9 Compare February 11, 2024 02:42

geelen approved these changes Feb 12, 2024

View reviewed changes

jasnell previously approved these changes Feb 12, 2024

View reviewed changes

kentonv force-pushed the kenton/jsrpc-service-binding branch from 1e5a34a to e9e4b43 Compare February 13, 2024 00:01

geelen reviewed Feb 13, 2024

View reviewed changes

MellowYarker reviewed Feb 13, 2024

View reviewed changes

src/workerd/api/worker-rpc.c++ Outdated Show resolved Hide resolved

kentonv force-pushed the kenton/jsrpc-service-binding branch from e9e4b43 to ce8b952 Compare February 13, 2024 18:25

kentonv force-pushed the kenton/jsrpc-service-binding branch from df72102 to 0274fb6 Compare February 16, 2024 20:53

jasnell reviewed Feb 16, 2024

View reviewed changes

jasnell approved these changes Feb 16, 2024

View reviewed changes

kentonv added 18 commits February 16, 2024 16:05

JSRPC: Define "cloudflare:entrypoints" module.

757fe9c

This contains the classes `DurableObject` and `StatelessService`. Top-level class exports should inherit one of these two classes to indicate what kind of export it is.

Fix compiler warning when comparing Js* types.

5c8fb60

E.g. comparing a `JsObject` to another `JsObject` would give some warning about "C++ standard says this is ambiguous even though it obviously isn't" or whatever. This seems to make the warning go away.

Add JsObject::getPrototype() method.

fa2c691

JSRPC: Add Worker::Api::getEntrypointClasses().

2b86ec0

This returns the class constructors for `StatelessService` and `DurableObject`, which we'll need in order to be able to check whether a particular exported class extends one of them.

JSRPC: Allow the default entrypoint to be a class.

49315cd

JSRPC: Add tests for disallowed method names.

1142b24

This requires creating a bypass on the client side that lets us specify these methods without inadvertently getting the respective method on the stub.

JSRPC: Rename: StatelessService -> WorkerEntrypoint

6881b42

JSRPC: Rename: cloudflare:entrypoints -> cloudflare:workers

3ebd72b

JSRPC: Automatically populate this.env and this.ctx.

67ccd35

kentonv force-pushed the kenton/jsrpc-service-binding branch from 0274fb6 to 63a44b9 Compare February 16, 2024 22:05

MellowYarker approved these changes Feb 16, 2024

View reviewed changes

kentonv merged commit 4d32cdc into main Feb 16, 2024
11 checks passed

irvinebroque mentioned this pull request Mar 3, 2024

Revamp Service Bindings docs for RPC cloudflare/cloudflare-docs#13252

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JSRPC: Service bindings and stateless class syntax #1658

JSRPC: Service bindings and stateless class syntax #1658

kentonv commented Feb 11, 2024

jasnell Feb 11, 2024

kentonv commented Feb 11, 2024

kentonv commented Feb 11, 2024

kentonv commented Feb 11, 2024

kentonv commented Feb 11, 2024

kentonv commented Feb 11, 2024

geelen left a comment

geelen Feb 12, 2024

kentonv Feb 12, 2024

geelen Feb 12, 2024

kentonv Feb 12, 2024

geelen Feb 12, 2024

kentonv Feb 12, 2024

geelen Feb 13, 2024

kentonv Feb 13, 2024

kentonv commented Feb 13, 2024

kentonv commented Feb 16, 2024

jasnell Feb 16, 2024

jasnell Feb 16, 2024

kentonv Feb 16, 2024

jasnell left a comment

JSRPC: Service bindings and stateless class syntax #1658

JSRPC: Service bindings and stateless class syntax #1658

Conversation

kentonv commented Feb 11, 2024

Choose a reason for hiding this comment

kentonv commented Feb 11, 2024

kentonv commented Feb 11, 2024

kentonv commented Feb 11, 2024

kentonv commented Feb 11, 2024

kentonv commented Feb 11, 2024

geelen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kentonv commented Feb 13, 2024

kentonv commented Feb 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell left a comment

Choose a reason for hiding this comment