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

Added a cleanup step that will mark all the memory as inaccessible and thus force termination of threads #3688

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

Conversation

john-sharratt
Copy link
Contributor

This pull request adds the first steps for a forced termination of processes by making all the memory that a WASM process uses inaccessible (including the stack it uses) which should terminate most threads forcefully

Copy link
Contributor

@theduke theduke left a comment

Choose a reason for hiding this comment

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

Looks mostly good to me, apart from two smaller comments, and one request:

We really need a test that validates this is working as expected.

lib/api/src/js/externals/memory.rs Outdated Show resolved Hide resolved
@john-sharratt john-sharratt marked this pull request as ready for review March 21, 2023 03:07
lib/wasi/src/state/env.rs Show resolved Hide resolved
@@ -85,6 +85,9 @@ pub enum MemoryError {
/// A user defined error value, used for error cases not listed above.
#[error("A user-defined error occurred: {0}")]
Generic(String),
/// Not implemented
#[error("This operation is not yet implemented")]
NotImplemented,
Copy link
Contributor

Choose a reason for hiding this comment

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

This enum does not have non_exhaustive, so adding a new variant is a breaking change. We can't do this quite yet.

I suggest:

  • Use the Generic variant
  • Add a TODO and create a follow up issue for the 4.0 milestone to track adding a new variant
  • Just ignore the error in the cleanup() code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand why you would mention non_exhaustive enums but this change in the upcoming beta does not sound like a big issue to me considering all the other breaking changes that are going into that release - either way I would rather go back to the way I had it returning Ok(()) if we are not going to do it properly - this small part of the change already cost too much time.

Copy link
Contributor

Choose a reason for hiding this comment

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

The problem is this is in the core Wasmer types, not in WASI.

There currently is no breaking change planned for Wasmer immediately, only later down the line.

So the next release will be 3.2, a non-breaking change.

/// to both reads and writes.
/// `start` and `len` must be native page-size multiples and describe a range within
/// `self`'s reserved memory.
pub fn make_inaccessible(&self, start: usize, len: usize) -> Result<(), String> {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably keep the additional API surface we expose minimal, so we can change/remove it again without a breaking change.

Wouldn't exposing make_inaccessible(&self) that equals make_all_inaccessible() be enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah that's true - i can make this change and hide some of the API internals

Copy link
Contributor

@theduke theduke left a comment

Choose a reason for hiding this comment

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

Also, really need a test or a few tests to validate the behvaiour , to make sure make_inaccessible() actually works.

Probably most easily done by starting an instance with some code that sleeps and reads some memory in a loop, then make the memory inaccessible and wait for the instance to fail?

@john-sharratt
Copy link
Contributor Author

Also, really need a test or a few tests to validate the behvaiour , to make sure make_inaccessible() actually works.

Probably most easily done by starting an instance with some code that sleeps and reads some memory in a loop, then make the memory inaccessible and wait for the instance to fail?

Of course I did this I just didn't make a unit test for it - the process terminates properly when memory is made inaccessible

@theduke
Copy link
Contributor

theduke commented Apr 21, 2023

I updated to master.

But, @john-sharratt , I just realized: to actually use this, we need a StoreRef.

Which we won't have available when triggering a forced shutdown externally, because the store is owned by the code calling into the instance and can't be shared.

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