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

Remove Send and Sync from wasmer types backed by JavaScript objects #4157

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions lib/api/src/js/externals/function.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,7 @@ pub struct Function {
pub(crate) handle: VMFunction,
}

// Function can't be Send in js because it dosen't support `structuredClone`
// https://developer.mozilla.org/en-US/docs/Web/API/structuredClone
// unsafe impl Send for Function {}
assert_not_implemented!(Function: !Send + !Sync);

impl From<VMFunction> for Function {
fn from(handle: VMFunction) -> Self {
Expand Down
4 changes: 1 addition & 3 deletions lib/api/src/js/externals/global.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,7 @@ pub struct Global {
pub(crate) handle: VMGlobal,
}

// Global can't be Send in js because it dosen't support `structuredClone`
// https://developer.mozilla.org/en-US/docs/Web/API/structuredClone
// unsafe impl Send for Global {}
assert_not_implemented!(Global: !Send + !Sync);

impl Global {
pub(crate) fn to_vm_extern(&self) -> VMExtern {
Expand Down
15 changes: 1 addition & 14 deletions lib/api/src/js/externals/memory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,20 +53,7 @@ pub struct Memory {
pub(crate) handle: VMMemory,
}

// Only SharedMemories can be Send in js, becuase they support `structuredClone`.
// Normal memories will fail while doing structuredClone.
// In this case, we implement Send just in case as it can be a shared memory.
// https://developer.mozilla.org/en-US/docs/Web/API/structuredClone
// ```js
// const memory = new WebAssembly.Memory({
// initial: 10,
// maximum: 100,
// shared: true // <--- It must be shared, otherwise structuredClone will fail
// });
// structuredClone(memory)
// ```
unsafe impl Send for Memory {}
unsafe impl Sync for Memory {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Side note: should't clippy have required a Safety: ... comment here?

assert_not_implemented!(Memory: !Send + !Sync);

impl Memory {
pub fn new(store: &mut impl AsStoreMut, ty: MemoryType) -> Result<Self, MemoryError> {
Expand Down
4 changes: 1 addition & 3 deletions lib/api/src/js/externals/table.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,7 @@ pub struct Table {
pub(crate) handle: VMTable,
}

// Table can't be Send in js because it dosen't support `structuredClone`
// https://developer.mozilla.org/en-US/docs/Web/API/structuredClone
// unsafe impl Send for Table {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we keep a comment on all assert_not_implemented usage points so future readers know what's going on here?

assert_not_implemented!(Table: !Send + !Sync);

fn set_table_item(table: &VMTable, item_index: u32, item: &Function) -> Result<(), RuntimeError> {
table.table.set(item_index, item).map_err(|e| e.into())
Expand Down
4 changes: 1 addition & 3 deletions lib/api/src/js/instance.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,7 @@ pub struct Instance {
pub(crate) _handle: VMInstance,
}

// Instance can't be Send in js because it dosen't support `structuredClone`
// https://developer.mozilla.org/en-US/docs/Web/API/structuredClone
// unsafe impl Send for Instance {}
assert_not_implemented!(Instance: !Send + !Sync);

impl Instance {
pub(crate) fn new(
Expand Down
32 changes: 32 additions & 0 deletions lib/api/src/js/macros.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
/// Assert that a type does **not** implement a set of traits.
macro_rules! assert_not_implemented {
($t:ty : !$first:ident $(+ !$rest:ident)*) => {
Copy link
Member

Choose a reason for hiding this comment

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

Just a small nit: assert_not_implemented!(Instance: !Send + !Sync); sounds like a double negative. so one might initially expect Instance to not implement !Send, i.e. that Instance should be Send. I don't have suggestions for alternate syntax though.

const _: fn() = || {
// Generic trait with a blanket impl over `()` for all types.
trait AmbiguousIfImpl<A> {
// Required for actually being able to reference the trait.
fn some_item() {}
}

impl<T: ?Sized> AmbiguousIfImpl<()> for T {}

// Creates multiple scoped `Invalid` types for each trait,
// over which a specialized `AmbiguousIfImpl<Invalid>` is
// implemented for every type that implements the trait.
#[allow(dead_code)]
struct InvalidFirst;
impl<T: ?Sized + $first> AmbiguousIfImpl<InvalidFirst> for T {}

$({
#[allow(dead_code)]
struct Invalid;
impl<T: ?Sized + $rest> AmbiguousIfImpl<Invalid> for T {}
})*

// If there is only one specialized trait impl, type inference
// with `_` can be resolved and this can compile. Fails to
// compile if `$t` implements any `AmbiguousIfImpl<Invalid>`.
let _ = <$t as AmbiguousIfImpl<_>>::some_item;
};
}
}
3 changes: 3 additions & 0 deletions lib/api/src/js/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@ mod lib {
}
}

#[macro_use]
mod macros;

mod as_js;
pub(crate) mod engine;
pub(crate) mod errors;
Expand Down
14 changes: 3 additions & 11 deletions lib/api/src/js/module.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,16 +45,7 @@ pub struct Module {
raw_bytes: Option<Bytes>,
}

// Module implements `structuredClone` in js, so it's safe it to make it Send.
// https://developer.mozilla.org/en-US/docs/Web/API/structuredClone
// ```js
// const module = new WebAssembly.Module(new Uint8Array([
// 0x00, 0x61, 0x73, 0x6d, 0x01, 0x00, 0x00, 0x00
// ]));
// structuredClone(module)
// ```
unsafe impl Send for Module {}
unsafe impl Sync for Module {}
assert_not_implemented!(Module: !Send + !Sync);

impl From<Module> for JsValue {
fn from(val: Module) -> Self {
Expand All @@ -81,12 +72,13 @@ impl Module {
}

/// Creates a new WebAssembly module skipping any kind of validation from a javascript module
///
pub(crate) unsafe fn from_js_module(
module: WebAssembly::Module,
binary: impl IntoBytes,
) -> Self {
#[allow(unused_variables)]
let binary = binary.into_bytes();

// The module is now validated, so we can safely parse it's types
#[cfg(feature = "wasm-types-polyfill")]
let (type_hints, name) = {
Expand Down
66 changes: 54 additions & 12 deletions lib/api/src/js/trap.rs
Original file line number Diff line number Diff line change
@@ -1,14 +1,17 @@
use crate::RuntimeError;
use std::error::Error;
use std::fmt;
use std::{
error::Error,
fmt::{self, Display},
};

use wasm_bindgen::{prelude::*, JsValue};
use wasm_bindgen_downcast::DowncastJS;

use crate::RuntimeError;

#[derive(Debug)]
enum InnerTrap {
User(Box<dyn Error + Send + Sync>),
Js(JsValue),
Js(JsTrap),
}

/// A struct representing a Trap
Expand All @@ -18,9 +21,6 @@ pub struct Trap {
inner: InnerTrap,
}

unsafe impl Send for Trap {}
unsafe impl Sync for Trap {}

impl Trap {
pub fn user(error: Box<dyn Error + Send + Sync>) -> Self {
Self {
Expand Down Expand Up @@ -67,8 +67,8 @@ impl std::error::Error for Trap {
impl fmt::Display for Trap {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match &self.inner {
InnerTrap::User(e) => write!(f, "user: {}", e),
InnerTrap::Js(value) => write!(f, "js: {:?}", value.as_string()),
InnerTrap::User(e) => write!(f, "user: {e}"),
InnerTrap::Js(value) => write!(f, "js: {value}"),
}
}
}
Expand All @@ -78,9 +78,51 @@ impl From<JsValue> for RuntimeError {
// We try to downcast the error and see if it's
// an instance of RuntimeError instead, so we don't need
// to re-wrap it.
let trap = Trap::downcast_js(original).unwrap_or_else(|o| Trap {
inner: InnerTrap::Js(o),
});
let trap: Trap = match Trap::downcast_js(original) {
Ok(trap) => trap,
Err(other) => Trap {
inner: InnerTrap::Js(JsTrap::from(other)),
},
};

trap.into()
}
}

/// A `Send+Sync` version of a JavaScript error.
#[derive(Debug)]
enum JsTrap {
/// An error message.
Message(String),
/// Unable to determine the underlying error.
Unknown,
}

impl From<JsValue> for JsTrap {
fn from(value: JsValue) -> Self {
// Let's try some easy special cases first
if let Some(error) = value.dyn_ref::<js_sys::Error>() {
return JsTrap::Message(error.message().into());
}

if let Some(s) = value.as_string() {
return JsTrap::Message(s);
}

// Otherwise, we'll try to stringify the error and hope for the best
if let Some(obj) = value.dyn_ref::<js_sys::Object>() {
return JsTrap::Message(obj.to_string().into());
}

JsTrap::Unknown
}
}

impl Display for JsTrap {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match self {
JsTrap::Message(m) => write!(f, "{m}"),
JsTrap::Unknown => write!(f, "unknown"),
}
}
}
12 changes: 4 additions & 8 deletions lib/api/src/js/vm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,7 @@ pub struct VMMemory {
pub(crate) ty: MemoryType,
}

unsafe impl Send for VMMemory {}
unsafe impl Sync for VMMemory {}
assert_not_implemented!(VMMemory: !Send + !Sync);

#[derive(Serialize, Deserialize)]
struct DummyBuffer {
Expand Down Expand Up @@ -140,8 +139,7 @@ impl VMGlobal {
}
}

unsafe impl Send for VMGlobal {}
unsafe impl Sync for VMGlobal {}
assert_not_implemented!(VMGlobal: !Send + !Sync);

/// The VM Table type
#[derive(Clone, Debug, PartialEq)]
Expand All @@ -150,8 +148,7 @@ pub struct VMTable {
pub(crate) ty: TableType,
}

unsafe impl Send for VMTable {}
unsafe impl Sync for VMTable {}
assert_not_implemented!(VMTable: !Send + !Sync);

impl VMTable {
pub(crate) fn new(table: JsTable, ty: TableType) -> Self {
Expand All @@ -171,8 +168,7 @@ pub struct VMFunction {
pub(crate) ty: FunctionType,
}

unsafe impl Send for VMFunction {}
unsafe impl Sync for VMFunction {}
assert_not_implemented!(VMFunction: !Send + !Sync);

impl VMFunction {
pub(crate) fn new(function: JsFunction, ty: FunctionType) -> Self {
Expand Down