don't panic the kernel upon a device error #4414
Conversation
|
Apparently I'm still fighting with the typechecker, but I think the rest of the PR should be reviewable. |
f776a01 to
95e3bbd
Compare
| /** @type { DeviceInvocationResult } */ | ||
| const deviceResults = dispatch.invoke(target, method, args); | ||
| // common error: raw devices returning capdata instead of ['ok', capdata] | ||
| assert.equal(deviceResults.length, 2, deviceResults); |
There was a problem hiding this comment.
I don't understand this assert. What is the meaning of the 3rd argument?
There was a problem hiding this comment.
That's the message emitted if the assertion fails (or at least I think that's what it's for), so the developer at the console can figure out why it threw without going back and inserting a console.log. optDetails seems to be the official name. The default for optDetails reports the two values being compared, in this case "2" and whatever .length is. In the failure mode I ran into, where my raw device returned { body: .., slots: ..} instead of ['ok', { body, slots }], the assertion printed undefined !== 2, which didn't help me very much.
There was a problem hiding this comment.
I also thought the 3rd arg was the message string, which is why I am puzzled because deviceResults is not a string.
| insistValidVatstoreKey(key); | ||
| kdebug(`syscall[${deviceID}].vatstoreDelete(${key})`); | ||
| return harden(['vatstoreDelete', deviceID, key]); | ||
| } |
There was a problem hiding this comment.
Seems like there's quite a bit of code duplication with vatTranslator.js. Should this be factored into some common code to ensure that changes to the syscall interface don't diverge under maintenance?
There was a problem hiding this comment.
Yeah, maybe. For now, devices don't get the same syscalls as vats: they don't do promises, so syscall.send isn't quite right, they can't syscall.exit, they don't participate in GC so no syscall.dropImports.
But in the long run, yeah, we want devices to be more like vats, and at that point it'll make sense to drop deviceTranslator.js and use vatTranslator in its place.
There was a problem hiding this comment.
I was thinking more along the lines of a syscall translator that's parameterized according to whether it thinks it's doing the job for a vat or a device, which would allow differences between the two to be accommodated but would allow the 95% that's the same to be a single piece of code to maintain.
Previously, any error thrown by a device invocation would set the kernel to the "panic" state, mark the calling vat as scheduled for termination, then return an error indication to the calling vat, which makes the `syscall.callNow()` throw an exception. From the calling vat's perspective, their `D()` invocation throw a "you killed my kernel, prepare to die" error, the delivery would complete, then the entire kernel would shut down. `controller.run()` rejects its return promise, causing the host application to halt without committing state. This was fail-stop safe against surprises in the device code, but is too difficult to use in practice. Devices could not use assert() to validate caller arguments without giving the calling vat the power to shut down the entire kernel. With this change, errors during device invocation cause the calling vat to get an error, as before, but the error is neither vat-fatal nor kernel-fatal. It is still vat-fatal to pass promises in arguments to devices, where the mistake is caught by the vat's outbound translator, rather than the device's inbound translator. We'll probably relax this restriction later. The device inbound translator used to throw an error if it got a foreign device node, or a promise that somehow got past the guard above. In both cases, the translator error caused a kernel panic. This commit changes the inbound translator to handle both foreign device nodes and promises, which ought to prevent vats from triggering kernel panics via this route. Deviceslots will still reject both, but since that error occurs in the device *invocation*, the calling vat should get a (catchable) error, and the kernel will not panic. GC handling is still really sketchy around devices: do not expect objects passed to a device to ever be dropped. closes #4326
These were really out of date. They could still use examples of how to manage state correctly, because it's pretty tricky.
95e3bbd to
cef1f88
Compare
fix(swingset): don't kernel panic upon device error
Previously, any error thrown by a device invocation would set the kernel to
the "panic" state, mark the calling vat as scheduled for termination, then
return an error indication to the calling vat, which makes the
syscall.callNow()throw an exception. From the calling vat's perspective,their
D()invocation throw a "you killed my kernel, prepare to die" error,the delivery would complete, then the entire kernel would shut down.
controller.run()rejects its return promise, causing the host applicationto halt without committing state.
This was fail-stop safe against surprises in the device code, but is too
difficult to use in practice. Devices could not use assert() to validate
caller arguments without giving the calling vat the power to shut down the
entire kernel.
With this change, errors during device invocation cause the calling vat to
get an error, as before, but the error is neither vat-fatal nor kernel-fatal.
It is still vat-fatal to pass promises in arguments to devices, where the
mistake is caught by the vat's outbound translator, rather than the device's
inbound translator. We'll probably relax this restriction later.
The device inbound translator used to throw an error if it got a foreign
device node, or a promise that somehow got past the guard above. In both
cases, the translator error caused a kernel panic. This commit changes the
inbound translator to handle both foreign device nodes and promises, which
ought to prevent vats from triggering kernel panics via this route.
Deviceslots will still reject both, but since that error occurs in the
device invocation, the calling vat should get a (catchable) error, and the
kernel will not panic.
GC handling is still really sketchy around devices: do not expect objects
passed to a device to ever be dropped.
closes #4326