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

comms: handle known-resolved promise in remote-to-kernel message correctly #2640

Closed
warner opened this issue Mar 15, 2021 · 1 comment
Closed
Assignees
Labels
bug Something isn't working SwingSet package: SwingSet

Comments

@warner
Copy link
Member

warner commented Mar 15, 2021

I suspect that we currently suffer from a bug, due to the changes we landed in #2516, but writing a test is the way to make sure. The scenario I think we don't handle correctly starts with a comms vat, that has two remotes (remoteA and remoteB), and the local kernel. Then:

  • remoteA sends a message to remoteB, which references a new (exported) promise
    • comms adds a promise table entry for it, with A as the decider, and B as a subscriber
    • note that the local kernel is not a subscriber (yet)
  • remoteA resolves the promise
  • comms receives a messge from remoteB, for the local kernel, which references the promise
    • remoteB must have emitted this message before it received the resolution, else it would make a new (ephemeral) promise ID
    • the messages crossed on the comms-remoteB wire
  • comms translates the message into kernel-facing identifiers
    • the promise is known to be resolved
    • comms creates an ephemeral promise ID to send to the kernel
    • comms does syscall.send into the local kernel, referencing the new vpid
    • BUG comms is responsible for promptly doing syscall.resolve for that vpid, but I suspect it does not

Previously, this prompt syscall.resolve was handled by a Promise.resolve().then(_ => syscall.resolve(...)) in provideKernelForLocal, as the promise ID was translated into a vpid. The translation was trivial, because we used the same IDs for both "local space" and "kernel-facing space", but it provided a convenient time to check the resolution status, and arrange to tell the kernel about it. We used Promise.resolve() to ensure the syscall.resolve didn't happen until after the syscall.send, because the kernel is certainly not prepared to receive a resolution before the export.

This "notify after translate" clause overlapped with changes (maybe from #2358?) that also did a syscall.resolve for ancillary promises after translating a remote resolve message. The overlap caused duplicate resolution syscalls for the remote resolve case. Removing the one in provideKernelForLocal reduced the number of resolves in the remote-resolve case from 2 to 1, but I think it would also reduce the number of resolves in the remote-send case from 1 to 0, and that will be a bug.

The task here is to write a test case that exercises the scenario above (probably added to test-comms.js, and following its pattern of "real comms vat, fake everything else", rather than attempting to build a test-message-patterns.js approach). Then see if it fails, and fix it. I think the fix will come out of the work in #2363, where we decide what follow-on resolve messages need to be sent after translation, rather than during.

I'm going to throw this at @FUDCo because he's already working in this space, but feel free to grab me for help writing the test case.

@warner warner added bug Something isn't working SwingSet package: SwingSet labels Mar 15, 2021
@FUDCo
Copy link
Contributor

FUDCo commented Apr 10, 2021

Closed by #2752

@FUDCo FUDCo closed this as completed Apr 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working SwingSet package: SwingSet
Projects
None yet
Development

No branches or pull requests

2 participants