JSRPC: Support "serializing" functions by turning them into stubs #1756

kentonv · 2024-03-02T23:39:27Z

It's simple: If you send someone a function, they get an RpcStub. The stub is callable. Calling it makes an RPC back to the isolate where the function was created, calling the original object there.

To reiterate, because this keeps confusing people: The function's code is NOT serialized. We are NOT magically moving code around. Closures strictly execute in the isolate where they were created.

I also went ahead and said that functions are allowed to have properties, but these work like properties on an RpcTarget. That is, they are not marshalled proactively when sending the function, but you can access them by accessing properties of the RpcStub representing the function.

There are also some other commits thrown in this PR because I did them at the same time:

First two commits: Prevent sending instances of application-defined classes over RPC, and improve error messages around inability to serialize.
Last two commits: Implement visitForGc() and visitForMemoryInfo().

There are a lot of commits here but they are each pretty light...

kentonv · 2024-03-02T23:43:58Z

src/node/internal/internal_assert.ts

-  const isThenable = typeof maybeThennable.then === "function" &&
-    typeof maybeThennable.catch === "function";
-
-  return isThenable && typeof maybeThennable !== "function";


I couldn't tell if there was a specific reason for the "not a function" check here. If so, I can remove this commit and keep my ugly monkey-patch instead.

Looking at it again refreshed my memory, this bit is taken directly from the polyfill that deno is using (https://github.com/denoland/deno/blob/main/ext/node/polyfills/assert.ts#L864) ... looking at it I'm not entirely sure what the intent originally was but I think it's fine to fix this up.

src/workerd/jsg/ser.c++

jasnell · 2024-03-03T00:49:48Z

One additional thought ... It would be good to include some additional tests for the structuredClone method to ensure it still maintains the expected behavior for things like functions as we're enabling these new bits. Just to make sure we don't accidentally introduce regressions... It might be that the existing structuredClone tests are already adequate but let's double check.

src/workerd/jsg/ser.c++

src/workerd/jsg/ser.h

irvinebroque · 2024-03-03T16:34:59Z

src/workerd/jsg/ser.h

+    //   fully thinking through what an attacker could do by sending them an instance of that class
+    //   when it isn't expected. Probably, we just shouldn't support this over RPC at all. For DO
+    //   storage, it could be OK since the application would only be deserializing objects it wrote
+    //   itself, but it may not be worth it to support for only that use case.


this comment helped me a lot, thank you

- 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

The default behavior of V8 serialization (and structured clone, I guess) is to serialize a class instance as if it were a plain object, completely ignoring the prototype. This is probably not useful in real use cases since it is lossy. So, we'd prefer that this just fails, both because it's not useful and because we might want to introduce better behavior in the future.

V8's default implementation of `WriteHostObject()` is pretty bad. It does NOT call `ThrowDataCloneError()`; instead it throws a regular old `Error`. The error message is also very weird, it puts the characters `#<>` around the type name. This commit changes to throwing a DataCloneError with a nicer error message.

This required a V8 patch to allow functions to be treated as host objects. This commit constructs the RpcStubs but they are not actually callable yet -- there is more work to be done in subsequent commits.

We need to make RpcStub itself callable, so having this as a private method of JsRpcProperty no longer works.

We can just use an old fashion if statement here.

…ables. We're about to make JsRpcPromise be callable, which makes it a function. But `isValidThenable()` in `internal_asserts.ts` thinks that thenables cannot be callable, which means `assert.rejects` doesn't our promises. This commit monkey-patches it to solve the problem, but maybe `isValidThenable()` itself should change...

…le thenables. This lets us get rid of the monkey-patch in js-rpc-test.js, but I'm not completely sure if it's correct. It does seem that V8 itself accepts callable thenables, but the check here looks very explicit so I'm not sure if it has a reason. `isValidThenable()` was also requiring the presence of a `catch` method, but this is not actually a requirement of thenables, so I removed that check as well.

This allows a stub to point to a bare function, and allows an RPC to directly return a bare function.

Sometimes people make a "callable object" by creating a function and then giving it properties. We should support this. I had originally wanted to disallow this if the function's prototype had been tampered with, like: ``` let func = () => {}; func.__proto__ = SomeClass.prototype; ``` This technique allows you to create a "callable class instance". In theory we ought to treat it like a user-defined class, i.e., don't support it, unless it extends JsRpcTarget. Unfortunately, it's sort of hard to distinguish "plain functions". There are multiple prototypes to check for: Function, AsyncFunction, and maybe others. I decided to give up and say that we treat all callable objects as functions.

…ctions. Since these types are callable, `IsFunction()` returns true, but if we want to actually handle "serializing" these, we need it to work very differently. So, make sure we don't count them, and add some tests. In the future we might decide to actually support these.

This should be the same as a class instance of application-defined type.

- 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 March 2, 2024 23:39

kentonv requested review from a team as code owners March 2, 2024 23:39

kentonv requested a review from harrishancock March 2, 2024 23:39

kentonv commented Mar 2, 2024

View reviewed changes