Skip to content
New issue

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

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

Already on GitHub? # to your account

exit and vatStore are not provided consistently across all vat workers #2643

Closed
warner opened this issue Mar 15, 2021 · 3 comments · Fixed by #4378
Closed

exit and vatStore are not provided consistently across all vat workers #2643

warner opened this issue Mar 15, 2021 · 3 comments · Fixed by #4378
Assignees
Labels
bug Something isn't working SwingSet package: SwingSet

Comments

@warner
Copy link
Member

warner commented Mar 15, 2021

Describe the bug

While working on adding syscall.dropImports tests (#2635), I noticed that we weren't consistently providing all vat worker types with access to the exit and vatStoreGet/Set/Delete syscalls. The xsnap worker lacked exit, while the subprocess-node and subprocess-nodeworker types lacked both exit and the vatStore trio.

The root problem is the unfortunate duplication of the const syscall = in all four worker types (src/kernel/vatManager/{syscall,supervisor-nodeworker,supervisor-subprocess-node,supervisor-subprocess-xsnap}.js). One of these days we should find a tidy way to refactor those.

I'll update the syscall definitions, and add more calls to test/workers/vat-target.js to exercise all four cases.

@warner warner added bug Something isn't working SwingSet package: SwingSet labels Mar 15, 2021
@warner warner self-assigned this Mar 15, 2021
@warner
Copy link
Member Author

warner commented Mar 15, 2021

Oh, interesting, to test this reliably, I think I'll need to implement #512 by making vatPowers.vatStore.get (making a key-prefix subspace of syscall.vatStore{Get,Set,Delete} available to userspace. I could try to use the virtual object manager and makeKind to exercise the syscalls, but I think those are only going to write to the DB when the cache is full, and until #2644 is done, that would take a distracting number of writes.

@warner
Copy link
Member Author

warner commented Mar 15, 2021

Eh, I'll fix this now and leave this open until the tests are added, to maintain momentum.

warner added a commit that referenced this issue Mar 15, 2021
Some unfortunate code duplication meant the nodeworker/subprocess-node
workers were lacking vatstoreGet, vatstoreSet, and vatstoreDelete. And
everything but the local worker was lacking syscall.exit.

refs #2643 , but doesn't close it because I want to (eventually) write proper
tests for this
warner added a commit that referenced this issue Mar 15, 2021
Some unfortunate code duplication meant the nodeworker/subprocess-node
workers were lacking vatstoreGet, vatstoreSet, and vatstoreDelete. And
everything but the local worker was lacking syscall.exit.

refs #2643 , but doesn't close it because I want to (eventually) write proper
tests for this
@warner
Copy link
Member Author

warner commented Jan 25, 2022

This got fixed as a side-effect of #2671, which refactored dispatch() and created a common place to build a syscall object for all worker types. I added a quick test today: it exercises vatStore in the workers that can block (local and subprocess-xsnap), and checks that vatPowers.exitVat and .exitVatWithFailure are functions (although it does not exercise them).

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bug Something isn't working SwingSet package: SwingSet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant