xs-worker doesn't tolerate BigInts in console.log() #2936
Comments
Shall I open an issue? Or was that more of a rhetorical "need" in the sense of "would ideally"? |
done |
|
On what to change it to, try replacing ... JSON.stringify(stuff) ...with something like const { quote: q } = assert;
... `${q(stuff)} ...It was designed for purposes like this --- strings useful for human diagnostics. Anything semantically closer to |
|
The algorithm it uses to stringify is at https://github.com/endojs/endo/blob/a7222d745ddc831167996ffaf30ccd7e036fe726/packages/ses/src/error/stringify-utils.js#L22 . Like your suggestion above, it calls |
We serialize console.log arguments (to send over the pipe to the kernel process) with JSON.stringify, which doesn't tolerate BigInt. Use a simple replacer to turn them into Numbers first; we can tolerate the loss of fidelity for console.log. closes #2936
We serialize console.log arguments (to send over the pipe to the kernel process) with JSON.stringify, which doesn't tolerate BigInt. Use a simple replacer to turn them into Numbers first; we can tolerate the loss of fidelity for console.log. closes #2936
We serialize console.log arguments (to send over the pipe to the kernel process) with JSON.stringify, which doesn't tolerate BigInt. Use a simple replacer to turn them into Numbers first; we can tolerate the loss of fidelity for console.log. closes #2936
We serialize console.log arguments (to send over the pipe to the kernel process) with JSON.stringify, which doesn't tolerate BigInt. Use a simple replacer to turn them into Numbers first; we can tolerate the loss of fidelity for console.log. closes #2936
We serialize console.log arguments (to send over the pipe to the kernel process) with JSON.stringify, which doesn't tolerate BigInt. Use a simple replacer to turn them into Numbers first; we can tolerate the loss of fidelity for console.log. closes #2936
We serialize console.log arguments (to send over the pipe to the kernel process) with JSON.stringify, which doesn't tolerate BigInt. Use a simple replacer to turn them into Numbers first; we can tolerate the loss of fidelity for console.log. closes #2936
We serialize console.log arguments (to send over the pipe to the kernel process) with JSON.stringify, which doesn't tolerate BigInt. Use a simple replacer to turn them into Numbers first; we can tolerate the loss of fidelity for console.log. closes #2936
We serialize console.log arguments (to send over the pipe to the kernel process) with JSON.stringify, which doesn't tolerate BigInt. Use a simple replacer to turn them into Numbers first; we can tolerate the loss of fidelity for console.log. closes #2936
We serialize console.log arguments (to send over the pipe to the kernel process) with JSON.stringify, which doesn't tolerate BigInt. Use a simple replacer to turn them into Numbers first; we can tolerate the loss of fidelity for console.log. closes #2936
We serialize console.log arguments (to send over the pipe to the kernel process) with JSON.stringify, which doesn't tolerate BigInt. Use a simple replacer to turn them into Numbers first; we can tolerate the loss of fidelity for console.log. closes #2936
We serialize console.log arguments (to send over the pipe to the kernel process) with JSON.stringify, which doesn't tolerate BigInt. Use a simple replacer to turn them into Numbers first; we can tolerate the loss of fidelity for console.log. closes #2936
|
It would be nice to retain whatever structure we can, so I wouldn't want to entirely flatten/stringify the arguments. @erights this is a case where the serializer (the XS worker child process) wants to deliver full-fidelity information to the unserializer (the kernel process): both sides are as privileged as we get. Is there a way to use @dckc I'm going to hand this one to you. The patch I started with is f55e733 (which includes a test). Please see if you can adapt @erights 's approach to it. |
|
I don't understand. The full fidelity approach is marshal. But this is about logging. What role does any kind of unserialization have in logging? |
|
The code is trying to pass the individual |
We serialize console.log arguments (to send over the pipe to the kernel process) with JSON.stringify, which doesn't tolerate BigInt. Use a simple replacer to turn them into Numbers first; we can tolerate the loss of fidelity for console.log. closes #2936
We serialize console.log arguments (to send over the pipe to the kernel process) with JSON.stringify, which doesn't tolerate BigInt. Use a simple replacer to turn them into Numbers first; we can tolerate the loss of fidelity for console.log. closes #2936
|
Oh, I should mention that we must not |
|
You can certainly use |
We serialize console.log arguments (to send over the pipe to the kernel process) with JSON.stringify, which doesn't tolerate BigInt. Use a simple replacer to turn them into Numbers first; we can tolerate the loss of fidelity for console.log. closes #2936
* fix(swingset): supervisor-xs: tolerate console.log(BigInt) We serialize console.log arguments (to send over the pipe to the kernel process) with JSON.stringify, which doesn't tolerate BigInt. Use a simple replacer to turn them into Numbers first; we can tolerate the loss of fidelity for console.log. closes #2936 * chore(xs-worker): use assert.q replacer to handle bigint etc. * test(swingset): console.log doesn't harden args Co-authored-by: Brian Warner <warner@lothar.com>
We serialize console.log arguments (to send over the pipe to the kernel process) with JSON.stringify, which doesn't tolerate BigInt. Use a simple replacer to turn them into Numbers first; we can tolerate the loss of fidelity for console.log. closes #2936
We serialize console.log arguments (to send over the pipe to the kernel process) with JSON.stringify, which doesn't tolerate BigInt. Use a simple replacer to turn them into Numbers first; we can tolerate the loss of fidelity for console.log. closes #2936
We serialize console.log arguments (to send over the pipe to the kernel process) with JSON.stringify, which doesn't tolerate BigInt. Use a simple replacer to turn them into Numbers first; we can tolerate the loss of fidelity for console.log. closes #2936
We serialize console.log arguments (to send over the pipe to the kernel process) with JSON.stringify, which doesn't tolerate BigInt. Use a simple replacer to turn them into Numbers first; we can tolerate the loss of fidelity for console.log. closes #2936
We serialize console.log arguments (to send over the pipe to the kernel process) with JSON.stringify, which doesn't tolerate BigInt. Use a simple replacer to turn them into Numbers first; we can tolerate the loss of fidelity for console.log. closes #2936
We serialize console.log arguments (to send over the pipe to the kernel process) with JSON.stringify, which doesn't tolerate BigInt. Use a simple replacer to turn them into Numbers first; we can tolerate the loss of fidelity for console.log. closes #2936
We serialize console.log arguments (to send over the pipe to the kernel process) with JSON.stringify, which doesn't tolerate BigInt. Use a simple replacer to turn them into Numbers first; we can tolerate the loss of fidelity for console.log. closes #2936
We serialize console.log arguments (to send over the pipe to the kernel process) with JSON.stringify, which doesn't tolerate BigInt. Use a simple replacer to turn them into Numbers first; we can tolerate the loss of fidelity for console.log. closes #2936
Describe the bug
In trying to get the XS worker usable for all vats, one problem I ran into was a vat that did
console.log()of a BigInt. The xsnap supervisor is currently sending the log arguments to the supervisor with the same JSON.stringify it uses for all other messages (namely syscalls). This fails (somewhat opaquely) when vat code logs something that can't be serialized this way.For the local worker, the vat's
console.logis routed to two places:agoric-sdk/packages/SwingSet/src/kernel/kernel.js
Lines 146 to 155 in fa68a8a
The first is
makeConsole(), which returns an instance ofanylogger. The second is thekernelSlot.vatConsole, which wraps the log arguments and ultimately puts them into a serialized JSON object written to the slog file. That serialization special-cases any BigInts:agoric-sdk/packages/SwingSet/src/controller.js
Lines 124 to 136 in fa68a8a
For the XS worker, we need to handle this somehow. It's probably sufficient, for now, to just special-case BigInt (i.e. not use a full marshal, and accept the loss of fidelity for debug logs).
That probably means changing this code:
https://github.com/Agoric/agoric-sdk/blob/master/packages/SwingSet/src/kernel/vatManager/supervisor-subprocess-xsnap.js#L166-L168
to pre-transform
args, like:In the long run, we need to nail down what exactly we're willing to tolerate in console.log calls, what precisely is the set of values that might appear in deliveries and syscalls, and make sure all our manager-worker communication pathways can support that full set. Our deliveries and syscalls are fairly conservative, and were designed to be JSON serializable, so it's really
console.logthat's the outlier.Testing
We should add a
console.log(0n)to our test vat (intest/workers/vat-target.js) to exercise this.The text was updated successfully, but these errors were encountered: