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

Fix WasiProcess::terminate() #4397

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

theduke
Copy link
Contributor

@theduke theduke commented Jan 9, 2024

fix(wasix): Fix WasiProcess::terminate()

Previously the code would mix up the output status of a thread/process with
an instruction to exit with a specific code.

The Process::terminate() method would just set the output status,
leaving no way to check if a thread had actually terminated.

This commit adds an additonal atomic flag that records if the process
should exit, and changes the relevant code (signal processing etc) to
check that flag.

The terminate method now just sets this code and then sends a kill
signal.

NOTE: The PR also contains some cleanup of the WasiProcess internal state.
Best to review commit by commit.

  • chore(wasix): Clean up WasiProcess state
  • refactor(wasix): Rename WasiProcessCheckpoint to ProcessCheckpoint
  • chore(vm): Fix clippy lints
  • fix(wasix): Fix WasiProcess::terminate()

Closes #4396

Previously there was a redundant nested Arc and some general
shennanigans for how the state was shared and accessed.

This commit wraps the whole state in an Arc<> and removes the old nested
arc or the inner state.

It also makes a lot of fields private and exposes them through getters.

NOTE: the WasiProcessInner should also be made private
Previously the code would mix up the output status of a thread/process with
an instruction to exit with a specific code.

The Process::terminate() method would just set the output status,
leaving no way to check if a thread had actually terminated.

This commit adds an additonal atomic flag that records if the process
should exit, and changes the relevant code (signal processing etc) to
check that flag.

The terminate method now just sets this code and then sends a kill
signal.
Copy link
Member

@syrusakbary syrusakbary left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@theduke
Copy link
Contributor Author

theduke commented Jan 9, 2024

Got some changes in the integration tests.

Need to investigate.

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.

WASIX: Fix thread termination logic
2 participants