Thread stack traces to console, add entangled assert #1884

erights · 2020-10-19T19:56:54Z

This change upgrades Agoric SDK packages to use the new SES 0.11.0, which includes the newly entangled Error, assert, and console objects, which collectively hide error details including the stack from callers, while safely revealing them to the console.

To facilitate this, the implementation of the @agoric/assert package shifts into SES, while the package remains useful as a vehicle for TypeScript definitions. All compartments that use @agoric/assert must now be endowed with the global assert.

Much of this change is mechanical, except for the assert.js module itself which may benefit from closer review.

erights · 2020-10-19T20:01:03Z

@kriskowal , As a result of the 2nd commit I am getting the following error everywhere. Looks to me like some packager is turning some esm into cjs with require, which is then failing to require other esm. Could you some help. Thanks.


  Uncaught exception in test/unitTests/test-registrar.js

  Error [ERR_REQUIRE_ESM]: Must use import to load ES Module: /Users/markmiller/src/ongithub/agoric/agoric-sdk/node_modules/ses/src/error/assert.js
  require() of ES modules is not supported.
  require() of /Users/markmiller/src/ongithub/agoric/agoric-sdk/node_modules/ses/src/error/assert.js from /Users/markmiller/src/ongithub/agoric/agoric-sdk/packages/assert/src/assert.js is an ES module file as it is a .js file whose nearest parent package.json contains "type": "module" which defines all .js files in that package scope as ES modules.
  Instead rename /Users/markmiller/src/ongithub/agoric/agoric-sdk/node_modules/ses/src/error/assert.js to end in .cjs, change the requiring code to use import(), or remove "type": "module" from /Users/markmiller/src/ongithub/agoric/agoric-sdk/node_modules/ses/package.json.

kriskowal · 2020-10-19T20:31:48Z

packages/assert/src/assert.js

-/*
-// TODO Somehow, use the `assert` exported by the SES-shim
-import { assert } from 'ses';
+import { assert } from 'ses/src/error/assert';


This will not work and we do not expect to make it work without making ses/assert part of the public API. The reason is the opposite of what you describe.

RESM operates by lowering all modules down to CommonJS (whereas SES Compartment lifts all modules up to the level of ECMAScirpt modules).

When an RESM reaches for ses, it gets a version of SES that was compiled down CommonJS by Rollup. When it reaches for ses/src/error/assert[.js] it gets the original source, RESM compiles it down to CommonJS, Node.js attempts to load the resulting .js file, but since SES has {"type": "module"}, it is expecting that file to be in ESM format, not CJS.

It should be sufficient to import ses and use the globalThis.assert here. If we need to reach for the module from source, we will have to add ses/assert to the exported modules of SES and add build rules for Rollup.

erights · 2020-10-20T03:11:51Z

@kriskowal
Of course with the first commit hardwiring things to paths on my local file system, nothing passes under CI. Is there any way to get things working under CI ?

erights · 2020-10-20T03:13:23Z

@warner I need to understand anylogger and how it fits into SwingSet. Thanks.

kriskowal · 2020-10-20T03:17:31Z

The best option is probably to use “npm pack” to make a tarball of SES, check that into SDK, and yarn add the archive with a file URL. See https://classic.yarnpkg.com/en/docs/cli/add/

…

On Mon, Oct 19, 2020 at 8:12 PM Mark S. Miller ***@***.***> wrote: @kriskowal <https://github.com/kriskowal> Of course with the first commit hardwiring things to paths on my local file system, nothing passes under CI. Is there any way to get things working under CI ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1884 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAOXBXGMJ54CF5HEJLZUP3SLT5YLANCNFSM4SWTAXKA> .

kriskowal · 2020-11-04T04:55:00Z

packages/SwingSet/src/kernel/virtualObjectManager.js

+ * @param {*} syscall  Vat's syscall object, used to access the `vatstoreGet`
+ *   and `vatstoreSet` operations.
 * @param {() => number} allocateExportID  Function to allocate the next object
- *    export ID for the enclosing vat.
- * @param valToSlotTable {*}  The vat's table that maps object identities to their
- *    corresponding export IDs
- * @param m {*}  The vat's marshaler.
- * @param cacheSize {number}  How many virtual objects this manager should cache
+ *   export ID for the enclosing vat.
+ * @param {*} valToSlotTable  The vat's table that maps object identities to
+ *   their corresponding export IDs
+ * @param {*} m The vat's marshaler.
+ * @param {number} cacheSize How many virtual objects this manager should cache
 *   in memory.
- *
- * @returns a new virtual object manager.
+ * @returns {*} a new virtual object manager.


@FUDCo I adjusted this to appease the linter. It appears to have become more picky.

kriskowal · 2020-11-04T04:57:07Z

I’m recusing myself as a reviewer, since I’m now a coäuthor. @michaelfig Perhaps we can get your review, particularly for the TS changes.

michaelfig

LGTM!

In case we have to revert this PR, please ensure that you Squash and Merge. Then, since the yarn.lock change is included, we will go back to the old implementation without having to pin anything explicitly.

kriskowal reviewed Oct 19, 2020

View reviewed changes

erights force-pushed the scratch-integrate-full branch from 522fc8e to 9138d06 Compare October 20, 2020 03:09

erights force-pushed the scratch-integrate-full branch from 59987cd to c6cc1a6 Compare October 30, 2020 02:38

kriskowal added 4 commits November 3, 2020 20:37

chore: Upgrade SES to v0.11.0

a00a053

refactor(assert): Rebuild assert package around assert global

0f5f3e1

refactor: Thread assert endowments

49a92a7

chore: Update yarn.lock

cad0634

kriskowal force-pushed the scratch-integrate-full branch from 504aab1 to cad0634 Compare November 4, 2020 04:39

kriskowal changed the title ~~WIP DO NOT MERGE --- hand wired local dependency for experimenting~~ Thread stack traces to console, add entangled assert Nov 4, 2020

kriskowal self-requested a review November 4, 2020 04:53

kriskowal marked this pull request as ready for review November 4, 2020 04:53

kriskowal requested a review from michaelfig November 4, 2020 04:55

kriskowal reviewed Nov 4, 2020

View reviewed changes

kriskowal requested review from kriskowal and removed request for kriskowal November 4, 2020 04:56

michaelfig approved these changes Nov 4, 2020

View reviewed changes

kriskowal merged commit 5d4f35f into master Nov 4, 2020

kriskowal deleted the scratch-integrate-full branch November 4, 2020 05:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Thread stack traces to console, add entangled assert #1884

Thread stack traces to console, add entangled assert #1884

erights commented Oct 19, 2020 •

edited by kriskowal

Loading

erights commented Oct 19, 2020

kriskowal Oct 19, 2020

erights commented Oct 20, 2020

erights commented Oct 20, 2020

kriskowal commented Oct 20, 2020 via email

kriskowal Nov 4, 2020

kriskowal commented Nov 4, 2020

michaelfig left a comment

Thread stack traces to console, add entangled assert #1884

Thread stack traces to console, add entangled assert #1884

Conversation

erights commented Oct 19, 2020 • edited by kriskowal Loading

erights commented Oct 19, 2020

kriskowal Oct 19, 2020

Choose a reason for hiding this comment

erights commented Oct 20, 2020

erights commented Oct 20, 2020

kriskowal commented Oct 20, 2020 via email

kriskowal Nov 4, 2020

Choose a reason for hiding this comment

kriskowal commented Nov 4, 2020

michaelfig left a comment

Choose a reason for hiding this comment

erights commented Oct 19, 2020 •

edited by kriskowal

Loading