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

SwingSet: Consolidate kernel run queue syscalls #4593

Merged
merged 3 commits into from
Feb 18, 2022

Conversation

mhofman
Copy link
Member

@mhofman mhofman commented Feb 18, 2022

Description

This change is preliminary refactor before #4542 and #3465, and just moves some code around. The goal is to consolidate the implementation of syscalls that impact the run queue into a single place. After this I believe all operations that result in a message to be added to the run queue transit through one of the methods of kernelQueue.js, except for the create-vat message.

Security Considerations

None

Documentation Considerations

N/A

Testing Considerations

Since this only moves code, no tests are added or removed, and all tests pass.

@mhofman mhofman requested review from warner and FUDCo February 18, 2022 00:16
* @param {string} kref Target of the message
* @param {string} method The message verb
* @param {*} args The message arguments
* @param {ResolutionPolicy=} policy How the kernel should handle an eventual
Copy link
Member Author

Choose a reason for hiding this comment

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

@warner, this method had a duplicate JSDoc depending on where you grabbed it from. I carried the stricter type of ResolutionPolicy here, but that causes a type error on line 105 below (if (policy !== 'none')), since 'none' is not a valid ResolutionPolicy.

Copy link
Member

Choose a reason for hiding this comment

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

Hm, I think that comparison should have been sendOnly. But grepping for queueToKref in test/, I see a whole lot of 'none' getting used, and no sendOnly.

Could you try replacing sendOnly with none in the type definition (and docs), and see if the tests all pass? If so, 'none' it is.

Copy link
Contributor

@FUDCo FUDCo left a comment

Choose a reason for hiding this comment

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

いいです

@warner warner added the SwingSet package: SwingSet label Feb 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 good, modulo that one type problem.

* @param {string} kref Target of the message
* @param {string} method The message verb
* @param {*} args The message arguments
* @param {ResolutionPolicy=} policy How the kernel should handle an eventual
Copy link
Member

Choose a reason for hiding this comment

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

Hm, I think that comparison should have been sendOnly. But grepping for queueToKref in test/, I see a whole lot of 'none' getting used, and no sendOnly.

Could you try replacing sendOnly with none in the type definition (and docs), and see if the tests all pass? If so, 'none' it is.

@mhofman
Copy link
Member Author

mhofman commented Feb 18, 2022

Could you try replacing sendOnly with none in the type definition (and docs), and see if the tests all pass? If so, 'none' it is.

That was it!

@mhofman mhofman added the automerge:rebase Automatically rebase updates, then merge label Feb 18, 2022
@mhofman mhofman force-pushed the mhofman/4542-syscall-refactor branch from 81bfb53 to a03e508 Compare February 18, 2022 17:37
@mergify mergify bot merged commit eae0201 into master Feb 18, 2022
@mergify mergify bot deleted the mhofman/4542-syscall-refactor branch February 18, 2022 18:08
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 SwingSet package: SwingSet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants