Skip to content

Commit

Permalink
fix: encapsulate marshal error kludgery
Browse files Browse the repository at this point in the history
  • Loading branch information
@erights
erights committed Apr 5, 2022
1 parent ce0d672 commit db15585
Show file tree
Hide file tree
Showing 3 changed files with 143 additions and 76 deletions.
7 changes: 6 additions & 1 deletion packages/marshal/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,12 @@ export { deeplyFulfilled } from './src/deeplyFulfilled.js';
export { makeTagged } from './src/makeTagged.js';
export { Remotable, Far, ToFarFunction } from './src/make-far.js';

export { QCLASS, makeMarshal } from './src/marshal.js';
export {
QCLASS,
makeMarshal,
makeMarshalErrorRecorder,
marshalDontSaveError,
} from './src/marshal.js';
export { stringify, parse } from './src/marshal-stringify.js';
// Works, but not yet used
// export { decodeToJustin } from './src/marshal-justin.js';
Expand Down
187 changes: 137 additions & 50 deletions packages/marshal/src/marshal.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ import {
passableSymbolForName,
} from './helpers/symbol.js';

/** @typedef {import('./types.js').MakeMarshalOptions} MakeMarshalOptions */
/** @template Slot @typedef {import('./types.js').ConvertSlotToVal<Slot>} ConvertSlotToVal */
/** @template Slot @typedef {import('./types.js').ConvertValToSlot<Slot>} ConvertValToSlot */
/** @template Slot @typedef {import('./types.js').Serialize<Slot>} Serialize */
Expand Down Expand Up @@ -48,36 +47,134 @@ const defaultValToSlotFn = x => x;
const defaultSlotToValFn = (x, _) => x;

/**
* @template Slot
* @param {ConvertValToSlot<Slot>} [convertValToSlot]
* @param {ConvertSlotToVal<Slot>} [convertSlotToVal]
* @param {MakeMarshalOptions} [options]
* @callback MarshalErrorRecorder
* @param {Error} err
* @returns {string|undefined}
*/
export const makeMarshal = (
convertValToSlot = defaultValToSlotFn,
convertSlotToVal = defaultSlotToValFn,
{
errorTagging = 'on',
marshalName = 'anon-marshal',
// TODO Temporary hack.
// See https://github.com/Agoric/agoric-sdk/issues/2780
errorIdNum = 10000,
// We prefer that the caller instead log to somewhere hidden
// to be revealed when correlating with the received error.
marshalSaveError = err =>
console.log('Temporary logging of sent error', err),
} = {},
) => {

/**
* @typedef {Object} MakeMarshalErrorRecorderOptions
* @property {string=} marshalName Used to identify sent errors.
* @property {number=} errorIdNum Ascending numbers staring from here
* identify the sending of errors relative to this marshal instance.
* @property {((fmt: string, err: Error) => void)=} log
* Errors are logged to `log` *after* `assert.note` associates
* that error with its errorId. Thus, if `marshalSaveError` in turn logs
* to the normal SES console, which is the default, then the console will
* show that note showing the associated errorId.
*/

/**
* @param {MakeMarshalErrorRecorderOptions=} options
* @returns {MarshalErrorRecorder}
*/
export const makeMarshalErrorRecorder = ({
marshalName = 'anon-marshal',
// TODO Temporary hack.
// See https://github.com/Agoric/agoric-sdk/issues/2780
errorIdNum = 10000,
// We prefer that the caller instead log to somewhere hidden
// to be revealed when correlating with the received error.
log = (fmt, err) => console.log(fmt, err),
} = {}) => {
assert.typeof(marshalName, 'string');
assert(
errorTagging === 'on' || errorTagging === 'off',
X`The errorTagging option can only be "on" or "off" ${errorTagging}`,
);
const nextErrorId = () => {
errorIdNum += 1;
return `error:${marshalName}#${errorIdNum}`;
};

const marshalErrorRecorder = err => {
// We deliberately do not share the stack, but it would
// be useful to log the stack locally so someone who has
// privileged access to the throwing Vat can correlate
// the problem with the remote Vat that gets this
// summary. If we do that, we could allocate some random
// identifier and include it in the message, to help
// with the correlation.
const errorId = nextErrorId();
assert.note(err, X`Sent as ${errorId}`);
log('Temporary logging of sent error', err);
return errorId;
};
return harden(marshalErrorRecorder);
};
harden(makeMarshalErrorRecorder);

/** @type {MarshalErrorRecorder} */
export const marshalDontSaveError = _err => undefined;
harden(marshalDontSaveError);

/**
* @typedef {Object} MakeMarshalOptionsAdapter
* @property {MarshalErrorRecorder=} marshalErrorRecorder
* @property {'on'|'off'=} errorTagging controls whether serialized errors
* also carry tagging information, made from `marshalName` and numbers
* generated (currently by counting) starting at `errorIdNum`. The
* `errorTagging` option defaults to `'on'`. Serialized
* errors are also logged to `marshalSaveError` only if tagging is `'on'`.
* @property {(err: Error) => void=} marshalSaveError If `errorTagging` is
* `'on'`, then errors serialized by this marshal instance are also
* logged by calling `marshalSaveError` *after* `assert.note` associates
* that error with its errorId. Thus, if `marshalSaveError` in turn logs
* to the normal SES console, which is the default, then the console will
* show that note showing the associated errorId.
*/

/**
* @typedef {MakeMarshalErrorRecorderOptions & MakeMarshalOptionsAdapter} MakeMarshalOptions
*/

/**
* This is an adaptor between the old calling convention and the new, for
* compat purposes.
*
* @param {MakeMarshalOptions=} options
* @returns {MarshalErrorRecorder}
*/
const makeMarshalRecorderFromMakeMarshalOptions = (options = {}) => {
const {
marshalErrorRecorder,
errorTagging,
marshalSaveError,
marshalName,
errorIdNum,
log,
} = options;
let logger = log;
if (marshalErrorRecorder) {
return marshalErrorRecorder;
}
if (errorTagging === 'off') {
return marshalDontSaveError;
}
if (marshalSaveError && !log) {
logger = (_fmt, err) => marshalSaveError(err);
}
return makeMarshalErrorRecorder({ marshalName, errorIdNum, log: logger });
};

/**
* @template Slot
* @typedef {Object} Marshal
* @property {Serialize<Slot>} serialize
* @property {Unserialize<Slot>} unserialize
*/

/**
* @template Slot
* @param {ConvertValToSlot<Slot>=} convertValToSlot
* @param {ConvertSlotToVal<Slot>=} convertSlotToVal
* @param {MakeMarshalOptions=} options
* @returns {Marshal<Slot>}
*/
export const makeMarshal = (
convertValToSlot = defaultValToSlotFn,
convertSlotToVal = defaultSlotToValFn,
options = {},
) => {
const marshalErrorRecorder = makeMarshalRecorderFromMakeMarshalOptions(
options,
);
/**
* @type {Serialize<Slot>}
*/
Expand All @@ -91,7 +188,7 @@ export const makeMarshal = (
* @param {InterfaceSpec=} iface
* @returns {Encoding}
*/
function serializeSlot(val, iface = undefined) {
const serializeSlot = (val, iface = undefined) => {
let slotIndex;
if (slotMap.has(val)) {
// TODO assert that it's the same iface as before
Expand All @@ -118,7 +215,7 @@ export const makeMarshal = (
iface,
index: slotIndex,
});
}
};

/**
* Even if an Error is not actually passable, we'd rather send
Expand All @@ -130,32 +227,21 @@ export const makeMarshal = (
* @returns {Encoding}
*/
const encodeError = err => {
// Must encode `cause`, `errors`.
// nested non-passable errors must be ok from here.
if (errorTagging === 'on') {
// We deliberately do not share the stack, but it would
// be useful to log the stack locally so someone who has
// privileged access to the throwing Vat can correlate
// the problem with the remote Vat that gets this
// summary. If we do that, we could allocate some random
// identifier and include it in the message, to help
// with the correlation.
const errorId = nextErrorId();
assert.note(err, X`Sent as ${errorId}`);
marshalSaveError(err);
return harden({
[QCLASS]: 'error',
errorId,
message: `${err.message}`,
name: `${err.name}`,
});
} else {
const errorId = marshalErrorRecorder(err);
if (errorId === undefined) {
return harden({
[QCLASS]: 'error',
message: `${err.message}`,
name: `${err.name}`,
});
}
assert.typeof(errorId, 'string');
return harden({
[QCLASS]: 'error',
errorId,
message: `${err.message}`,
name: `${err.name}`,
});
};

/**
Expand Down Expand Up @@ -296,7 +382,7 @@ export const makeMarshal = (
/** @type {Map<number, Passable>} */
const valMap = new Map();

function unserializeSlot(index, iface) {
const unserializeSlot = (index, iface) => {
if (valMap.has(index)) {
return valMap.get(index);
}
Expand All @@ -306,7 +392,7 @@ export const makeMarshal = (
const val = convertSlotToVal(slot, iface);
valMap.set(index, val);
return val;
}
};

/**
* We stay close to the algorithm at
Expand Down Expand Up @@ -343,7 +429,7 @@ export const makeMarshal = (
*
* @param {Encoding} rawTree must be hardened
*/
function fullRevive(rawTree) {
const fullRevive = rawTree => {
if (!isObject(rawTree)) {
// primitives pass through
return rawTree;
Expand Down Expand Up @@ -488,7 +574,7 @@ export const makeMarshal = (
}
return result;
}
}
};
return fullRevive;
};

Expand Down Expand Up @@ -518,3 +604,4 @@ export const makeMarshal = (
unserialize,
});
};
harden(makeMarshal);
25 changes: 0 additions & 25 deletions packages/marshal/src/types.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -181,31 +181,6 @@ export {};
* @returns {Passable}
*/

/**
* @template Slot
* @typedef {Object} Marshal
* @property {Serialize<Slot>} serialize
* @property {Unserialize<Slot>} unserialize
*/

/**
* @typedef {Object} MakeMarshalOptions
* @property {'on'|'off'=} errorTagging controls whether serialized errors
* also carry tagging information, made from `marshalName` and numbers
* generated (currently by counting) starting at `errorIdNum`. The
* `errorTagging` option defaults to `'on'`. Serialized
* errors are also logged to `marshalSaveError` only if tagging is `'on'`.
* @property {string=} marshalName Used to identify sent errors.
* @property {number=} errorIdNum Ascending numbers staring from here
* identify the sending of errors relative to this marshal instance.
* @property {(err: Error) => void=} marshalSaveError If `errorTagging` is
* `'on'`, then errors serialized by this marshal instance are also
* logged by calling `marshalSaveError` *after* `assert.note` associated
* that error with its errorId. Thus, if `marshalSaveError` in turn logs
* to the normal console, which is the default, then the console will
* show that note showing the associated errorId.
*/

// /////////////////////////////////////////////////////////////////////////////

/**
Expand Down

0 comments on commit db15585

Please sign in to comment.