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

move method name into serialized message data, rather than separate field #2481

Closed
erights opened this issue Feb 19, 2021 · 6 comments · Fixed by #5392
Closed

move method name into serialized message data, rather than separate field #2481

erights opened this issue Feb 19, 2021 · 6 comments · Fixed by #5392
Assignees
Labels
Milestone

Comments

@erights
Copy link
Member

erights commented Feb 19, 2021

Blocking #2092

Blocking our use of subscriptions. We're using `notifier's where we should be using descriptors because of this.

@warner explains possible solutions at
#2092 (comment)
and
#2092 (comment)

@warner
Copy link
Member

warner commented Mar 10, 2021

Does anyone mind if I expand/refine this to "swingset+comms should accept Symbol-named method names"? Being able to send Symbols through the kernel/comms in arguments is one thing, but the real issue that affected #2092 was the inability to use them as method names (which are treated specially). @erights ?

@erights
Copy link
Member Author

erights commented Mar 10, 2021

Fine with me. Please do.

Btw, I thought they were already fine over comm and were only a problem through the SwingSet kernel. Wrong?

@warner
Copy link
Member

warner commented Mar 10, 2021

Sending Symbols as arguments over comms is fine, but using a Symbol as a method name runs into the same (actually more) problems as it does in the kernel.

@warner warner changed the title Cannot send Symbol.asyncIterable through the SwingSet kernel move method name into serialized message data, rather than separate field Mar 10, 2021
@warner
Copy link
Member

warner commented Mar 10, 2021

Ok this ticket is now about:

  • change liveslots to create a serialized message as serialize({ method, args }), rather than simply serialize(args)
  • change syscall.send from (target, method, args, result) to (target, message, result)
  • change the kernel-vat protocol to match
    • the vatSyscallObject flavor which was ['send', target, method, args, result] becomes ['send', target, message, result]
    • change the per-vat translator function (vatTranslator.js translateMessage) to match, it unpacks the message
  • change the kernel-side syscall handler to match
  • change the run-queue format to match
  • change the kernel promise table's pipelined-message queue format to match
  • examine deliverToVat and friends, make sure they can handle the new format
  • deliverToTarget cites msg.method in the Error object it creates when the message is sent to a Promise that is resolved to a non-Presence, it will need to deserialize the message and extract the method name (or we need to abandon that diagnostic detail)
  • change the vatTranslator on the way back into a vat
  • change the signature of dispatch.deliver() from (target, method, args, result) to (target, message, result)
  • change liveslots deliver() to handle the new format
  • comms changes:
    • comms/dispatch.js deliver() needs to change, it should probably unserialize the message immediately so it can dispatch on the method name for its internal object handlers (e.g. receive, addRemote, addEgress, addIngress)
    • comms/delivery.js sendToRemote() must parse localDelivery differently, with the method baked into a message along with the args
    • the comms protocol must change: remove the distinct :${method}: serialization, leaving the method to the trailing variable-length ${body}
    • any debug tools we have that think they can extract method from a comms message must be updated
  • any diagnostic messages (e.g. lsdebug) which are helpfully announcing the message name must be changed, probably to stop being so helpful, maybe to unserialize and extract the message name first
    • this unserialization, for the purpose of extracting a single field, should probably be done with a plain JSON.parse, because 1: we know the message field is either a string or a @qclass: symbol, and 2: we certainly don't want to invoke the usual slotToVal machinery

Some of the kernel-side changes are merely documentation and type definitions: the kernel itself only unpacks the kernelDeliveryObject in a few places. The other references don't need to change their behavior. The biggest changes are in comms, liveslots, and the translators. The biggest risks are:

  • silent type confusion along the kernel-side vat-to-queue-to-vat pathways, which could be mitigated by first applying thorough typescript definitions to everything
  • terminology exhaustion, in this ticket I'm using message to mean { method, args }, but of course the word "message" is hideously overloaded, with at least one concept fighting for a name at every level of our protocol stack, sometimes two

@erights
Copy link
Member Author

erights commented Mar 10, 2021

  • terminology exhaustion, in this ticket I'm using message to mean { method, args }, but of course the word "message" is hideously overloaded, with at least one concept fighting for a name at every level of our protocol stack, sometimes two

I suggest request. Both requests and responses are messages, but only requests invoke methods on targets, with a response normally expected.

warner added a commit that referenced this issue Mar 12, 2021
Liveslots special-cases this one symbol by converting it into a (string)
method name of `"Symbol.asyncIterator"` for `syscall.send`, and back again
during `dispatch.deliver`. This has several limitations:

* no other Symbols are accepted yet
* `E(target)[Symbol.asyncIterator](args)` and
`E[target]['Symbol.asyncIterator'](args)` will invoke the same target method,
which is clearly wrong
* a method named `'Symbol.asyncIterator'` (as a string) cannot be invoked
remotely

This should all be fixed someday by #2481.

closes #2619
@Tartuffo Tartuffo added MN-1 and removed MN-1 labels Feb 2, 2022
@Tartuffo Tartuffo added this to the Mainnet 1 milestone Mar 23, 2022
@Tartuffo
Copy link
Contributor

I suggest we discuss whether this is a real block for MN-1. (If it must be done eventually, how bad would the upgrade path be?)

@Tartuffo Tartuffo modified the milestones: Mainnet 1, RUN Protocol RC0 Apr 5, 2022
warner added a commit that referenced this issue Apr 29, 2022
@warner warner removed their assignment May 11, 2022
FUDCo pushed a commit that referenced this issue 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
eventually to allow remote function call, i.e., no method selector at
all, to be supported).

refs #2481
FUDCo pushed a commit that referenced this issue 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
eventually to allow remote function call, i.e., no method selector at
all, to be supported).

refs #2481
FUDCo pushed a commit that referenced this issue 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
Tartuffo pushed a commit that referenced this issue 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 closed this as completed in #5392 May 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants