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

Asynchronous threading phase2 #3771

Merged
merged 34 commits into from
May 16, 2023
Merged

Conversation

john-sharratt
Copy link
Contributor

This patch does a series of refactoring to prevent similar bugs to the one that was found in the asynchronous threading patch.

In particular:

  • inner() is scoped to the wasi module and no longer usable outside
  • inner() is marked as unsafe and any use of it is audited and wrapped with unsafe
  • Cloning WasiEnv no longer copies the inner()
  • Rewinding the stack now passes a return value back to the rewound stack rather than using the memory (which is unsafe)
  • The after callback on asynchronous threading has been removed and replaced with rewind return values

@john-sharratt john-sharratt changed the base branch from master to asynchronous-threading April 17, 2023 07:50
Base automatically changed from asynchronous-threading to master May 15, 2023 05:30
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.

Why do we need that thread local?

If we really do need it, we can guard against bugs by storing some thread-relevant value in the thread local and panic on a mismatch.
(We can just store the whole WasiThread, since it's clone, and do Arc::ptr_eq for equality)

(rather than making the functions unsafe)

lib/wasi-web/src/pool.rs Show resolved Hide resolved
lib/wasi/src/lib.rs Outdated Show resolved Hide resolved
lib/wasi/src/lib.rs Outdated Show resolved Hide resolved
lib/wasi/src/state/env.rs Show resolved Hide resolved
lib/wasi/src/state/env.rs Show resolved Hide resolved
lib/wasi/src/state/func_env.rs Show resolved Hide resolved
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.

Why do we need that thread local?

If we really do need it, we can guard against bugs by storing some thread-relevant value in the thread local and panic on a mismatch.
(We can just store the whole WasiThread, since it's clone, and do Arc::ptr_eq for equality)

(rather than making the functions unsafe)

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.

Much cleaner now without the thread locals!

@theduke theduke enabled auto-merge (rebase) May 15, 2023 17:25
auto-merge was automatically disabled May 15, 2023 18:06

Rebase failed

@john-sharratt john-sharratt enabled auto-merge May 16, 2023 01:56
@john-sharratt john-sharratt merged commit b5e7c27 into master May 16, 2023
@john-sharratt john-sharratt deleted the asynchronous-threading-phase2 branch May 16, 2023 02:21
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.

3 participants