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

Improve xsnap-worker's reaction to EOF/SIGPIPE by explicit logging to stderr (#4103) #10099

Merged
merged 26 commits into from
Sep 25, 2024

Conversation

siarhei-agoric
Copy link
Contributor

closes: #4103
refs: agoric-labs/xsnap-pub PR #53

Description

This is the agoric-sdk side of the changes for xsnap-worker, containing updating the git subproject and adding new tests.

Security Considerations

New version of xsnap-worker explicitly exits and logs clear text errors to stderr when it detects an unexpected EOF or SIGPIPE condition, which does disclose some information regarding its internal state.

Scaling Considerations

Overall behavior is the same as xsnap-worker dies/exits just as before.

Documentation Considerations

none for agoric-sdk.

Testing Considerations

a new test had been added.

Upgrade Considerations

none.

@siarhei-agoric siarhei-agoric added hygiene Tidying up around the house logging xsnap the XS execution tool labels Sep 17, 2024
Copy link
Member

@mhofman mhofman left a comment

Choose a reason for hiding this comment

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

Quick preliminary feedback.

Dealing with streams and child processes in node is probably one of the most difficult thing to do cleanly and robustly. Let's pair at some point to try an make this test logic a little more idiomatic.

packages/xsnap/test/xsnap-eof.test.js Outdated Show resolved Hide resolved
packages/xsnap/test/xsnap-eof.test.js Outdated Show resolved Hide resolved
packages/xsnap/test/xsnap-eof.test.js Outdated Show resolved Hide resolved
packages/xsnap/test/xsnap-eof.test.js Outdated Show resolved Hide resolved
packages/xsnap/test/xsnap-eof.test.js Outdated Show resolved Hide resolved
packages/xsnap/test/xsnap-eof.test.js Outdated Show resolved Hide resolved
packages/xsnap/test/xsnap-eof.test.js Outdated Show resolved Hide resolved
packages/xsnap/test/xsnap-eof.test.js Outdated Show resolved Hide resolved
packages/xsnap/test/xsnap-eof.test.js Outdated Show resolved Hide resolved
packages/xsnap/test/xsnap-eof.test.js Outdated Show resolved Hide resolved
Copy link
Member

@gibson042 gibson042 left a comment

Choose a reason for hiding this comment

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

I agree with @mhofman's comments as well.

packages/xsnap/test/xsnap-eof.test.js Outdated Show resolved Hide resolved
packages/xsnap/test/xsnap-eof.test.js Outdated Show resolved Hide resolved
packages/xsnap/test/xsnap-eof.test.js Outdated Show resolved Hide resolved
packages/xsnap/test/xsnap-eof.test.js Outdated Show resolved Hide resolved
packages/xsnap/test/xsnap-eof.test.js Outdated Show resolved Hide resolved
packages/xsnap/test/xsnap-eof.test.js Outdated Show resolved Hide resolved
packages/xsnap/test/xsnap-eof.test.js Outdated Show resolved Hide resolved
Copy link

cloudflare-workers-and-pages bot commented Sep 20, 2024

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: d5b9dff
Status: ✅  Deploy successful!
Preview URL: https://3443d1d4.agoric-sdk.pages.dev
Branch Preview URL: https://sliakh-4103-xsnap-exit-eof.agoric-sdk.pages.dev

View logs

packages/xsnap/test/fixture-xsnap-eof.js Outdated Show resolved Hide resolved
packages/xsnap/test/xsnap-eof.test.js Outdated Show resolved Hide resolved
packages/xsnap/test/xsnap-eof.test.js Show resolved Hide resolved
packages/xsnap/test/xsnap-eof.test.js Outdated Show resolved Hide resolved
packages/xsnap/test/xsnap-eof.test.js Outdated Show resolved Hide resolved
Copy link
Member

@mhofman mhofman left a comment

Choose a reason for hiding this comment

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

I think we're missing a crucial test: pipe closed while trying to write a query (worker JS doing issueCommand).

We can probably do that by having the handleCommand in the worker do 2 issueCommand in a row, or close the "from xsnap" stream immediately after issuing the command.

packages/xsnap/test/xsnap-eof.test.js Outdated Show resolved Hide resolved
packages/xsnap/test/xsnap-eof.test.js Outdated Show resolved Hide resolved
packages/xsnap/test/xsnap-eof.test.js Outdated Show resolved Hide resolved
packages/xsnap/test/xsnap-eof.test.js Show resolved Hide resolved
Copy link
Member

@gibson042 gibson042 left a comment

Choose a reason for hiding this comment

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

Style suggestions only; I like the new tests!

packages/xsnap/test/xsnap-eof.test.js Show resolved Hide resolved
packages/xsnap/test/xsnap-eof.test.js Outdated Show resolved Hide resolved
packages/xsnap/test/xsnap-eof.test.js Outdated Show resolved Hide resolved
packages/xsnap/test/xsnap-eof.test.js Outdated Show resolved Hide resolved
packages/xsnap/test/xsnap-eof.test.js Outdated Show resolved Hide resolved
packages/xsnap/test/xsnap-eof.test.js Outdated Show resolved Hide resolved
packages/xsnap/test/xsnap-eof.test.js Outdated Show resolved Hide resolved
Copy link
Member

@mhofman mhofman left a comment

Choose a reason for hiding this comment

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

The new structure of the test looks great. Once the few nits are addressed, we'll be almost ready for merging (I assume as a squash merge in this case, unless you plan on doing a local rebase to fixup commits)

Before merging this PR, we'll want to first merge agoric-labs/xsnap-pub#53, and use the outcome commit from the Agoric branch there as the new submodule commit here (and reflected in the build.env file)

Approving assuming this is done before merging.

packages/xsnap/src/xsnap.js Outdated Show resolved Hide resolved
packages/xsnap/test/xsnap-eof.test.js Outdated Show resolved Hide resolved
packages/xsnap/test/xsnap-eof.test.js Outdated Show resolved Hide resolved
packages/xsnap/test/xsnap-eof.test.js Outdated Show resolved Hide resolved
packages/xsnap/test/xsnap-eof.test.js Outdated Show resolved Hide resolved
packages/xsnap/test/xsnap-eof.test.js Outdated Show resolved Hide resolved
packages/xsnap/test/xsnap-eof.test.js Outdated Show resolved Hide resolved
@siarhei-agoric siarhei-agoric added the automerge:squash Automatically squash merge label Sep 25, 2024
@mergify mergify bot merged commit d0fe49e into master Sep 25, 2024
80 checks passed
@mergify mergify bot deleted the sliakh-4103-xsnap-exit-EOF branch September 25, 2024 19:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:squash Automatically squash merge hygiene Tidying up around the house logging xsnap the XS execution tool
Projects
None yet
Development

Successfully merging this pull request may close these issues.

xsnap should exit quietly upon command pipe EOF
3 participants