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

Serialize method names in message encoding #5392

Merged
merged 1 commit into from
May 20, 2022
Merged

Serialize method names in message encoding #5392

merged 1 commit into from
May 20, 2022

Conversation

FUDCo
Copy link
Contributor

@FUDCo FUDCo commented May 18, 2022

Changes message encoding to eliminate the method field and move its contents into the args capdata object (now renamed methargs) so that the method name will be serialized, allowing non-string values (notably symbols) to be used as method selectors (and also allows remote function call, i.e., no method selector at all, to be supported).

Fixes #2481

@FUDCo FUDCo added enhancement New feature or request SwingSet package: SwingSet labels May 18, 2022
@FUDCo FUDCo requested a review from warner May 18, 2022 21:44
@FUDCo FUDCo self-assigned this May 18, 2022
Copy link
Member

@warner warner left a comment

Choose a reason for hiding this comment

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

Looks pretty good.. needs a few variable-name changes, and the "args are always present, except eventual-get uses args = undefined" thing we discussed. Feel free to land after those are addressed.

@warner
Copy link
Member

warner commented May 19, 2022

@FUDCo Oh, I forgot to mention: we need to update docs/comms.md too, to update the wire protocol definition to remove $method from the send: lines.

@FUDCo FUDCo added the automerge:rebase Automatically rebase updates, then merge label May 20, 2022
Changes message encoding to eliminate the `method` field and move its
contents into the `args` capdata object (now renamed `methargs`) so
that the method name will be serialized, allowing non-string
values (notably symbols) to be used as method selectors (and also
allow remote function call, i.e., no method selector at all, to be
supported).

refs #2481
@mergify mergify bot merged commit dda33c0 into master May 20, 2022
@mergify mergify bot deleted the 2481-methargs branch May 20, 2022 02:29
Copy link
Member

@erights erights left a comment

Choose a reason for hiding this comment

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

I know it is already merged. Just some questions?

resultPolicy = 'ignore',
slots = [],
) {
function replacer(_, arg) {
Copy link
Member

Choose a reason for hiding this comment

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

Why introduce a replacer that special cases bigints and undefined but nothing else? What is special about these two?

const kref = kernel.getRootObject(vatID);
const kpid = kernel.queueToKref(kref, method, args, resultPolicy);
const methargs = {
body: JSON.stringify([method, args], replacer),
Copy link
Member

Choose a reason for hiding this comment

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

Is this an additional layer of JSON encoding, or does it replace a JSON.stringify removed elsewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously callers of queueToVatRoot had to provide a pre-cooked capdata object (and of course creating a capdata requires a JSON.stringify call to produce the body string). queueToVatRoot is used by tests to inject messages into the kernel from the outside. This particular PR required updating a lot of tests, so one of the things I did was move some of the necessary quasi-serialization work that previously each test had to do by hand into queueToVatRoot in order to make the API less delicate. Ideally we would just use marshal and avoid a lot of the mess here, but unfortunately, we can't. This entry point has to weirdly straddle the world as seen from inside a vat and world as seen from inside the kernel -- it's used to send messages into a vat as if the caller was another vat sending the message, but the caller isn't in a vat; it's a piece of unit test code that in effect exists in kernel space. In particular, the stuff that would otherwise be done by convertValToSlot must instead be done by providing the slots array (containing raw kref strings) directly, because there is no clist for the sender. Tests can get away with this because they construct their worlds to be just so and the worlds they construct tend to be very simple.

The replacer you asked about is migrated from some of the tests which sent messages with arguments containing (in this case) bigints and undefineds. and this freed the callers from having to construct @qclass structures manually. This is another piece of making the API less delicate. Conceivably other @qclass things might need to be added in the future if tests require them, but I don't expect that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:rebase Automatically rebase updates, then merge enhancement New feature or request SwingSet package: SwingSet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

move method name into serialized message data, rather than separate field
3 participants