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.

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

Unserialized errors are remote #2769

Merged
merged 6 commits into from
Apr 2, 2021
Merged

Unserialized errors are remote #2769

merged 6 commits into from
Apr 2, 2021

Conversation

erights
Copy link
Member

@erights erights commented Mar 31, 2021

If linked with endojs/endo#649 , then marshal will cause unserialized errors to be identified as such when shown on the console. This tagging is invisible to unprivileged code.

If linked before that PR, then this PR has no effect. Thus, it can be merged before a SES containing that other PR is released.

Well, almost no effect. As a place holder for @dtribble's better pseudo-pseudo-random-number-generator, the remote error tagging simply starts the counter at 10000 instead of 0. This will help a bit but will still be confusing.

@erights
Copy link
Member Author

erights commented Mar 31, 2021

I did #2773 to test whether the job stuck above in CI has anything to do with this PR.
An hour later, its
https://github.com/Agoric/agoric-sdk/pull/2773/checks?check_run_id=2240580543
is still stuck in CI, from which I conclude that whatever the problem is, it has nothing to do with this PR.

@erights erights force-pushed the see-remote-errors branch 2 times, most recently from ef88ed1 to 3d962a7 Compare April 1, 2021 01:35
@erights
Copy link
Member Author

erights commented Apr 1, 2021

We wanted to have the errorIdNum values generated by this PR be usefully unique, possibly paired with the errorTagNum values from endojs/endo#649 also being usefully unique. Given the Agoric sparse-int package, this all seemed trivial, until we hit tensions between various engineering principles. We agree to omit such enhanced uniqueness from these PRs, and instead file a bug on what still needs to be done: #2780

@erights erights requested a review from warner April 1, 2021 02:46
@warner warner removed the request for review from michaelfig April 1, 2021 17:20
Copy link
Member

@warner warner left a comment

Choose a reason for hiding this comment

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

Code changes seem fine. Consider the errorCount=0 thing but not too long.

Overall, as we figure out #2780, I'm expecting to see this converge into a form where the original error is tagged with a composite identifier that holds 1: a weakly-globally-unique-ish randomly-generated Swingset instance identifier, 2: a unique-within-that-Swingset vat ID, 3: a unique-within-that-vat Error counter. When this serialized error migrates to a different system, the Error object created during deserialization should retain all that, plus get marked with the triple of identifiers from the point of deserialization. If it travels further, I'm on the fence as to whether we accumulate the full trail of breadcrumbs, or we throw out all but the first (origin) and last.

packages/marshal/src/marshal.js Outdated Show resolved Hide resolved
packages/marshal/src/marshal.js Outdated Show resolved Hide resolved
@erights
Copy link
Member Author

erights commented Apr 2, 2021

This PR does not provide enhanced console output until it is using a ses that incorporates endojs/endo#649 , which is not yet released, and will be after the ses 0.12.6 release. Until then, this PR doesn't break anything but also doesn't contribute much value.

@erights erights merged commit 7d7a8ee into master Apr 2, 2021
@erights erights deleted the see-remote-errors branch April 2, 2021 03:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants