JSRPC: Support resource management for RPC stubs #1784

kentonv · 2024-03-08T06:42:32Z

RpcStubs and RpcPromises now have Symbol.dispose.
Stubs passed in call parameters are automatically disposed upon return.
If the result of a call is an object, a disposer will be added to it, which disposes all stubs therein. (If it's not an object, it can't possibly contain any stubs...)
Stubs also have a dup() method which creates a separately-disposed copy of the stub, in case you want to keep a stub past when it would otherwise be automatically disposed.
Of course, when the I/O context is destroyed, everything is implicitly disposed.

Note: The first few commits use a method named "dispose" but the last commit changes it to Symbol.dispose. Maybe I should have done that first, oh well.

jasnell · 2024-03-08T14:44:13Z

src/workerd/jsg/setup.c++

+              v8::NewStringType::kInternalized).ToLocalChecked()));
+      symbolAsyncDispose.Reset(ptr, v8::Symbol::New(ptr,
+          v8::String::NewFromUtf8(ptr, "asyncDispose",
+              v8::NewStringType::kInternalized).ToLocalChecked()));


v8::Symbol::For(...) is probably better here. And we should probably namespace the description in the polyfill, essentially like Symbol.for('cloudflare.dispose').

Thinking about it, I guess there's a non-zero chance this might break someone if they happen to already be polyfilling Symbol.dispose in their worker. It's probably unlikely enough to not worry about, but it's something we'll want to keep an eye out for.

v8::Symbol::For(...) is probably better here.

Hmm, why would we want that? Seems like that could only lead to foot-shooting: someone might depend on the fact that Symbol.dispose == Symbol.for("cloudflare.dispose"), and then this assumption will break as soon as V8 starts providing this symbol itself.

Thinking about it, I guess there's a non-zero chance this might break someone if they happen to already be polyfilling Symbol.dispose in their worker. It's probably unlikely enough to not worry about, but it's something we'll want to keep an eye out for.

How could it break someone?

If they are polyfilling carefully (only assigning the symbol if it doesn't already exist) they should be fine.

If they are just overwriting the symbol indiscriminately, they won't be able to use RPC correctly, but existing code should continue to work fine.

Sorry, I had meant ForApi(...). My brain got stuck on Symbol.for when I was writing the comment. If we used ForApi(...) we wouldn't need to cache the symbol ref anywhere and could just use ForApi(...) when we wanted to grab it again.... So if I took another pass at my comment, it would have been...

v8::Symbol::ForApi(...) is probably better....`

and

v8::Symbol::ForApi(js.strIntern("cloudflare.dispose"));

But as it is currently is fine also.

How could it break someone

As I said, it's probably unlikely enough not to worry about, but theoretically someone could have a branch the executes only if Symbol.dispose is not present, or they are already depending on Symbol.dispose being identity equal to their own polyfilled Symbol.for('whatever') variant.

I don't think anyone will actually be broken by this, just acknowledging that the risk is non-zero (even if super close to zero)

Ah, I suppose ForApi() wouldn't have the problem of allowing applications to become dependent on it -- but still not clear if it provides any real advantages. The cached symbol ref is faster to access anyway (no need to look up an interned string, etc.) and more type-safe.

jasnell · 2024-03-08T14:48:04Z

src/workerd/jsg/rtti.h

+    using Traits = FunctionTraits<Method>;
+    BuildRtti<Configuration, typename Traits::ReturnType>::build(method.initReturnType(), rtti);
+    using Args = typename Traits::ArgsTuple;
+    TupleRttiBuilder<Configuration, Args>::build(method.initArgs(std::tuple_size_v<Args>), rtti);


@mrbbot ... just a heads up here, on the type generation side of things, eventually, for any type that implements the dispose or async dispose, we'll likely want to mark those explicitly with Typescript's implements Disposable and implements AsyncDisposable (https://www.typescriptlang.org/docs/handbook/release-notes/typescript-5-2.html#using-declarations-and-explicit-resource-management).

src/workerd/jsg/rtti.capnp

src/workerd/jsg/resource.c++

src/workerd/api/worker-rpc.h

src/workerd/api/global-scope.h

jasnell · 2024-03-08T15:28:00Z

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

@@ -147,12 +186,107 @@ private:
  kj::ForkedPromise<void> promise;
 };

+// Given a value, check if it has a dispose method and, if so, invoke it.
+void tryCallDisposeMethod(jsg::Lock& js, jsg::JsValue value) {
+  js.withinHandleScope([&]() {


non-blocking: Hopefully v8 will give us an api to call this directly on a v8::Object

jasnell · 2024-03-08T15:29:16Z

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

+      auto dispose = obj.get(js, js.symbolDispose());
+      if (dispose.isFunction()) {
+        jsg::check(v8::Local<v8::Value>(dispose).As<v8::Function>()->Call(
+            js.v8Context(), value, 0, nullptr));


The Symbol.dispose method is not supposed to return a value, but if is does, I wonder if we should emit a warning here... especially true if user code gets it wrong and has Symbol.dispose return a promise.

Well, it's actually fine if it returns a promise. The promise will still run.

Note that all JSRPC disposal is async, that is, it always runs after the caller has already moved on, and any exceptions thrown by the disposer do not propagate to the caller. Given that, there's not actually anything inherently wrong with a disposer that performs async tasks.

Oh, I know. My concern is largely catching coding bugs.. like exceptions that potentially end up being swallowed and just going nowhere.

async [Symbol.dispose]() { throw new Error('boom'); }

Certainly not a big deal, just being overly cautious.

I believe these exceptions will be reported in the inspector as uncaught exceptions FWIW.

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

* Add dup() and dispose() methods to JsRpcStub. * Implicitly dispose parameters when call returns. * Add dispose() method to returned objects which disposes all stubs therein.

The application can implement `dispose()` to find out when the last stub pointing at their object is dropped.

If a returned object has a dispose method, we don't want to serialize it. We do, however, want it to be called when the call's pipeline is dropped, meaning the object is no longer needed to satisfy pipeline calls.

…sing promises. This commit ended up with two separate (but related) things mixed up in it: 1. When dispose() is invoked an a JsRpcPromise, we want this to be equivalent to awaiting the result and calling `dispose()` on that. 2. If an RPC returns an object which has a `dispose()` method, we really want to call that that only when the client calls `dispose()` on the client-side return value. Prior to this commit, the server-side disposer would be called when the JsRpcPromise on the client was GC'd, which could be either before or after `dispose()` was called on the client-side result.

We don't want people to rely on GC to close their stubs for them, because this means a change in GC behavior could break them. We cannot control what V8 does with GC. To that end, we say that any stub which is leaked will live until the IoContext is destroyed. We'll also log a warning if we detect this. In order to test this, I also added the `ctx.abort()` method we've talked about adding -- guarded as experimental.

This polyfills `Symbol.dispose` and `Symbol.asyncDispose` per the explicit resource management spec: https://github.com/tc39/proposal-explicit-resource-management I decided to entirely replace the special name "dispose" with Symbol.dispose, rather than write complicated logic to make it possible to use either one. However, I could be convinced that this is too ugly and that we should go back to allowing "dispose" as an alias.

kentonv requested review from jasnell and MellowYarker March 8, 2024 06:42

kentonv requested review from a team as code owners March 8, 2024 06:42

kentonv requested a review from Frederik-Baetens March 8, 2024 06:42

kentonv force-pushed the kenton/jsrpc-disposal branch from a3c4f27 to cc75641 Compare March 8, 2024 06:44