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.

Sign up for GitHub

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

Jump to bottom

worker fixes #1797

Merged
merged 5 commits into from
Sep 21, 2020
Merged

worker fixes #1797

merged 5 commits into from
Sep 21, 2020

Conversation

warner
Copy link
Member

@warner warner commented Sep 17, 2020

Address a bunch of small issues with our different vatWorker types, to align their feature sets more closely. They aren't quite identical yet, but this is enough to allow more tests to pass, and should unblock the addition of CI for the xs-vat-worker type (#1299)

@warner warner added the SwingSet package: SwingSet label Sep 17, 2020
@warner warner self-assigned this Sep 17, 2020
@warner warner requested a review from FUDCo September 17, 2020 19:06
@warner
Copy link
Member Author

warner commented Sep 17, 2020

@FUDCo could you also run yarn test test/workers/test-worker.js on your Mac? This PR re-enables two tests which were disabled in older PRs of yours, I think because they caused failures on your local machine. (BTW the commit comments on the disabling patches only mentioned refactoring other things, we should probably put such things in their own commits in the future). Thanks!

@FUDCo
Copy link
Contributor

FUDCo commented Sep 18, 2020

The requested test fails on my Mac with the output:

yarn run v1.22.5
$ ava test/workers/test-worker.js

SwingSet global metering is disabled (no replaceGlobalMeter)
SwingSet global metering is disabled (no replaceGlobalMeter)
node-worker does not support enableInternalMetering
node-worker does not support enableInternalMetering
[SyntaxError: Unexpected end of JSON input
  at JSON.parse (<anonymous>)
  at DestroyableTransform.<anonymous> (/Users/chip/Scratch/agoric-sdk/packages/SwingSet/src/kernel/vatManager/subprocessSupervisor.js:94:33)
  at DestroyableTransform.emit (events.js:314:20)
  at DestroyableTransform.EventEmitter.emit (domain.js:486:12)
  at addChunk (/Users/chip/Scratch/agoric-sdk/node_modules/through2/node_modules/readable-stream/lib/_stream_readable.js:291:12)
  at readableAddChunk (/Users/chip/Scratch/agoric-sdk/node_modules/through2/node_modules/readable-stream/lib/_stream_readable.js:276:55)
  at DestroyableTransform.Readable.push (/Users/chip/Scratch/agoric-sdk/node_modules/through2/node_modules/readable-stream/lib/_stream_readable.js:245:10)
  at DestroyableTransform.Transform.push (/Users/chip/Scratch/agoric-sdk/node_modules/through2/node_modules/readable-stream/lib/_stream_transform.js:148:32)
  at DestroyableTransform.readNetstring [as _transform] (/Users/chip/Scratch/agoric-sdk/node_modules/netstring-stream/lib/netstring.js:57:18)
  at DestroyableTransform.Transform._read (/Users/chip/Scratch/agoric-sdk/node_modules/through2/node_modules/readable-stream/lib/_stream_transform.js:184:10)]
vat does buildRootObject
callback 11 12
  node-subprocess vat manager

  Error: Promise returned by test never resolved
    at Test.finishDueToInactivity (/Users/chip/Scratch/agoric-sdk/node_modules/ava/lib/test.js:696:7)
    at Runner.beforeExitHandler (/Users/chip/Scratch/agoric-sdk/node_modules/ava/lib/runner.js:236:13)
    at process.emit (events.js:314:20)
    at process.EventEmitter.emit (domain.js:486:12)

  ─

  1 test failed
  1 test skipped
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

FUDCo
FUDCo approved these changes Sep 18, 2020
View reviewed changes
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.

Looks good modulo the mysteriously non-working test.

packages/SwingSet/src/controller.js
const supercode = require.resolve(
'./kernel/vatManager/subprocessSupervisor.js',
);
return startSubprocessWorker(process.execPath, ['-r', 'esm', supercode]);
Copy link
Contributor

Choose a reason for hiding this comment

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

The use of process.execPath here seems like it could introduce some interesting (and I'm not sure it it's the good kind of interesting or the bad kind of interesting) possibilities if the executable is something that wraps node or otherwise is not actually node itself. In particular, I can imagine a use case in which the launching executable is something greater than node per se but where the workers really should be run with the actual node executable. I'm not sure this is a problem we need to solve now or indeed if it's even a problem at all, but you might give some thought to putting in a way to parameterize what gets run here instead of assuming "run whatever kind of thing I am".

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, I figured this was the simplest way to run under the same version of Node as the parent process, but needing to add the -r esm is a hassle. Once we get rid of -r esm and move to type: module everywhere, maybe this could change.

@warner
Copy link
Member Author

warner commented Sep 19, 2020

Ugh, it looks like the netstring-stream library we're using doesn't buffer input correctly, and assumes that every frame appears in a single read. That breaks when the parent process writes a frame that's larger than the OS buffer, whose size is selected by the OS kernel. I picked netstring-stream because it was the least ancient of everything I found on npm (latest release is three years old), but I should have looked more carefully at it.

I'll search around for a better library, or I'll figure out the Stream interface and write my own.

@warner warner mentioned this pull request Sep 19, 2020
Closed
warner added a commit that referenced this pull request Sep 19, 2020
@warner
fix: add netstring encode/decode/stream library
664b218 
For use by the kernel-worker protocol.

refs #1797
closes #1807
@warner warner mentioned this pull request Sep 19, 2020
Merged
warner added a commit that referenced this pull request Sep 19, 2020
@warner
fix: add netstring encode/decode/stream library
fd1da9e 
For use by the kernel-worker protocol.

refs #1797
closes #1807
@warner
fix: clean up worker subprocess spawning
afeced8 
fixes #1777
@warner
fix: handle syscallResult and deliveryResult consistently among workers
9e6e31a 
fixes #1775
@warner
fix: xs-vat-worker: add makeMarshal to vatPowers
1edd620 
to match other workers

refs #1776
@warner
fix: pass testLog to all vatWorkers
29bc81a 
This adds a new message to the vat-to-kernel protocol, `['testLog',
...args]`, to deliver the strings from the vat to the kernel. They get added
to the same `c.dump().log` array that local workers can write to. This is
solely for the benefit of unit tests.

refs #1776 (closing the `testLog` part, but not the other vatPowers)
@warner
test: exercise testLog in test-worker.js
49605c6
@warner
Copy link
Member Author

warner commented Sep 21, 2020

ok I've got a branch with a new kernel-worker protocol (with the correct netstring stream decoder), I'll land this subset of changes first, and then make a new PR with the rest

@warner warner merged commit 44ab432 into master Sep 21, 2020
@warner warner deleted the 1775-worker-fixes branch September 21, 2020 21:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FUDCo FUDCo FUDCo approved these changes

Assignees

@warner warner

Labels
SwingSet package: SwingSet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@warner @FUDCo