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

refactor(daemon): Synchronize host makeUnconfined() #2124

Merged
merged 1 commit into from
Mar 9, 2024

Conversation

rekmarks
Copy link
Contributor

@rekmarks rekmarks commented Mar 8, 2024

Progresses: #2086

Synchronizes the host's makeUnconfined() per #2086. Refactoring daemon.js in support of this goal fixed one bug while revealing another.

In particular, #2074 is progressed by enabling indirect cancellation of caplets via their workers. The issue is not resolved since indirect cancellation of caplets via their caplet dependencies still does not work as intended. A new, failing regression test has been added for this specific case.

The revealed bug is #2021, which we believed to be fixed by #2092. Rather than fixing the bug, that PR concealed it by always creating a new incarnation of eval formula workers, even if they already existed. The regression test for #2021 has been marked as failing, and we will have to find a different solution for it.

@rekmarks rekmarks requested review from kumavis and kriskowal and removed request for kumavis March 8, 2024 18:28
@rekmarks rekmarks force-pushed the rekmarks-synchronize-makeUnconfined branch from 65f7bfa to 63a7d1c Compare March 8, 2024 18:33
Copy link
Member

@kumavis kumavis left a comment

Choose a reason for hiding this comment

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

looks a little complicated, but idk how to make it simpler yet

@rekmarks rekmarks force-pushed the rekmarks-delete-makeWorker branch from 6516b25 to 9fe0d6c Compare March 8, 2024 19:51
Base automatically changed from rekmarks-delete-makeWorker to master March 8, 2024 19:57
@rekmarks
Copy link
Contributor Author

rekmarks commented Mar 8, 2024

@kumavis I hope to simplify things as the synchronization effort continues, but the simplest way of doing things may not reveal itself until late in that process.

@rekmarks rekmarks force-pushed the rekmarks-synchronize-makeUnconfined branch from 63a7d1c to 5d356f7 Compare March 8, 2024 22:44
@rekmarks rekmarks marked this pull request as ready for review March 8, 2024 23:08
@rekmarks rekmarks force-pushed the rekmarks-synchronize-makeUnconfined branch from 5d356f7 to aab0a1a Compare March 8, 2024 23:13
Copy link
Member

@kriskowal kriskowal left a comment

Choose a reason for hiding this comment

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

With a nit.

packages/daemon/src/host.js Outdated Show resolved Hide resolved
packages/daemon/src/host.js Outdated Show resolved Hide resolved
// Regression test for: https://github.com/endojs/endo/issues/2021
test('eval-mediated worker name', async t => {
// Regression test for https://github.com/endojs/endo/issues/2021
test.failing('eval-mediated worker name', async t => {
Copy link
Member

Choose a reason for hiding this comment

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

So, we have whacked a mole?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am consulting my exterminator as to our next steps.

Synchronizes the host's `makeUnconfined()` per #2086. Refactoring
`daemon.js` in support of this goal fixed one bug while revealing
another.

In particular, #2074 is progressed by enabling indirect
cancellation of caplets via their workers. The issue is not resolved
since indirect cancellation of caplets via their caplet dependencies
still does not work as intended. A new, failing regression test has been
added for this specific case.

The revealed bug is #2021, which we believed to be fixed by #2092.
Rather than fixing the bug, that PR concealed it by always creating a
new incarnation of `eval` formula workers, even if they already existed.
The regression test for #2021 has been marked as failing, and we will
have to find a different solution for it.
@rekmarks rekmarks force-pushed the rekmarks-synchronize-makeUnconfined branch from aab0a1a to a239f10 Compare March 9, 2024 00:05
@rekmarks rekmarks enabled auto-merge March 9, 2024 00:07
@rekmarks rekmarks merged commit 96e54b9 into master Mar 9, 2024
14 checks passed
@rekmarks rekmarks deleted the rekmarks-synchronize-makeUnconfined branch March 9, 2024 00:10
* Helper for callers of `incarnateNumberedGuest`.
* @param {string} hostFormulaIdentifier - The formula identifier of the host.
*/
const incarnateGuestDependencies = async hostFormulaIdentifier =>
Copy link
Member

Choose a reason for hiding this comment

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

Should be provideGuestDependencies. Doesn’t create formulas. Renaming formulate* functions should make that distinction clearer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants