Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds JSG_NAMED_INTERCEPT #1288

Merged
merged 1 commit into from
Oct 26, 2023
Merged

Adds JSG_NAMED_INTERCEPT #1288

merged 1 commit into from
Oct 26, 2023

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Oct 9, 2023

This mechanism uses v8's underlying named property handler mechanism to allow a JSG_RESOURCE_TYPE to expose dynamic named properties (much like a Proxy). Currently this only supports read-only properties. The intent of this is to support the dynamic interface of a JS RPC proxy.

This is a first pass at the JSG api for this. I'm sure there is lots of room for improvement but I wanted to get something out that would allow the folks on the JS RPC stuff to make progress.

@MellowYarker
Copy link
Contributor

I think registerNamedIntercept needs to be added to jsg/rtti.h's MemberCount and MembersBuilder. When I take the exact ProxyImpl defined in jsg-test.c++ (excluding testing stuff) and try to create a new class outside of jsg-test.c++ the build complains about registerNamedIntercept not being found.

@jasnell jasnell force-pushed the jsnell/jsg-named-intercept branch 3 times, most recently from ac94f6b to 63e79db Compare October 18, 2023 23:39
@jasnell

This comment was marked as resolved.

@jasnell jasnell force-pushed the jsnell/jsg-named-intercept branch 3 times, most recently from ba7c284 to e8bf535 Compare October 19, 2023 15:40
Copy link
Collaborator

@dom96 dom96 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 really cool! Left some questions/suggestions, but good to go in general.

src/workerd/jsg/jsg.h Show resolved Hide resolved
src/workerd/jsg/jsg-test.c++ Show resolved Hide resolved
src/workerd/jsg/jsg-test.c++ Show resolved Hide resolved
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.

Thanks for this James, very much appreciated!

src/workerd/jsg/rtti.h Show resolved Hide resolved
src/workerd/jsg/resource.h Outdated Show resolved Hide resolved
src/workerd/jsg/resource.h Show resolved Hide resolved
src/workerd/jsg/jsg.h Show resolved Hide resolved
src/workerd/jsg/resource.h Outdated Show resolved Hide resolved
@MellowYarker
Copy link
Contributor

I was working on #1311 (comment) and noticed that if I JSG_FAIL_REQUIRE() from getNamed, then even if I wrap my JS in a try...catch I still get an uncaught exception.

Ex.

try {
    obj.alarm(); // This is not allowed, we should throw via JSG_FAIL_REQUIRE().
} catch(e) {
    // We don't enter here, we just terminate outright.
}

Should be able to catch a JSG exception thrown during name interception, right? Any ideas what might be wrong?

This mechanism uses v8's underlying named property handler mechanism
to allow a JSG_RESOURCE_TYPE to expose dynamic named properties
(much like a Proxy). Currently this only supports read-only properties.
The intent of this is to support the dynamic interface of a JS RPC
proxy.
@jasnell jasnell requested review from a team as code owners October 26, 2023 16:00
@jasnell
Copy link
Member Author

jasnell commented Oct 26, 2023

Should be able to catch a JSG exception thrown during name interception, right? Any ideas what might be wrong?

Yeah, I've updated with a fix for that. Please give it a try again :-)

@MellowYarker
Copy link
Contributor

Should be able to catch a JSG exception thrown during name interception, right? Any ideas what might be wrong?

Yeah, I've updated with a fix for that. Please give it a try again :-)

Works now, thanks!

@jasnell
Copy link
Member Author

jasnell commented Oct 26, 2023

Ok, I'll get an internal CI run going now and if that looks good I'll get this merged. I'll likely hold off actually pulling this into the internal repo immediately tho.

@jasnell jasnell merged commit 36a3688 into main Oct 26, 2023
11 checks passed
int getBar() { return 123; }

// NamedIntercept implementation
kj::Maybe<jsg::JsValue> getNamed(jsg::Lock& js, kj::StringPtr name) override {
Copy link
Member

Choose a reason for hiding this comment

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

@jasnell Could we allow getNamed() to return an arbitrary type, which then gets JSG-wrapped?

(Most implementations would return jsg::Optional<Something> so that they can return none for properties that don't exist...)

// would return kj::none).
//
// struct ProxyImpl: public jsg::Object,
// public jsg::NamedIntercept {
Copy link
Member

Choose a reason for hiding this comment

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

@jasnell It surprises me that this is based on inheritance, and the use of multiple inheritance makes me nervous since if you don't declare jsg::Object as the first superclass, stuff will break.

Is this NamedIntercept base class actually necessary? It looks like the implementation is templated such that the calls are always made on the derived class anyway...

Copy link
Contributor

Choose a reason for hiding this comment

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

I actually ran into this problem the other day, declared the jsg::Object (in my case, Fetcher) after NamedIntercept and got segfaults for a while.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is this NamedIntercept base class actually necessary?

Probably not. I put this together fairly quickly so that @MellowYarker could make progress, fully expecting that it could be refined. I think we ought to be able to drop the base class and I want to see if I can make the three methods that need to be implemented here a bit more ergonomic.

@MellowYarker
Copy link
Contributor

Just noticed that if I define a private method on my DO and try to call it from the client side, I get jsg.SyntaxError: Private field '#getPrivate' must be declared in an enclosing class and getNamed() isn't invoked.

Is there any way we can have private names also be intercepted? We want to reject calls to private methods over RPC, but I think it would be better if the script got a clear exception stating we don't allow calling private methods over RPC rather than the message above.

@fhanau fhanau deleted the jsnell/jsg-named-intercept branch December 4, 2023 22:27
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.

4 participants