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

Wasm32-wasi target support #1576

Merged
merged 1 commit into from Jun 4, 2022
Merged

Wasm32-wasi target support #1576

merged 1 commit into from Jun 4, 2022

Conversation

rjzak
Copy link
Contributor

@rjzak rjzak commented May 24, 2022

I'm working on getting Tokio working on Rust, and some changes were needed for Mio.

Companion to tokio-rs/tokio#4716

Signed-off-by: Richard Zak richard@profian.com

src/poll.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@Thomasdezeeuw Thomasdezeeuw left a comment

Choose a reason for hiding this comment

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

@rjzak we're not going to accept this as is. Poll needs to have mutable access on Windows as it can only handle a single poll call at a time. Furthermore we don't want to force everyone to use an Arc.

Could you please explain why you need this change? "Wasm improvements" are not really clear.

src/poll.rs Outdated
})
}
}

/// Create a separate `Registry` which can be used to register
/// `event::Source`s.
#[cfg(target_os = "wasi")]
pub fn registry(&self) -> Arc<Registry> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We're not going to accept a different public API based on the compile target.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a work-around for try_clone() not being available to the wasi registry here https://github.com/tokio-rs/tokio/blob/2d90ef5861029a9cfc8e89c05ddc65ed754bb77b/tokio/src/io/driver/mod.rs#L133 requires https://github.com/tokio-rs/mio/blob/master/src/poll.rs#L638, but try_clone() isn't working on the underlying object with Wasi, that's a compiler error.

@rjzak
Copy link
Contributor Author

rjzak commented May 25, 2022

@Thomasdezeeuw I certainly wouldn't want this PR accepted as-is, as the test application here https://github.com/rjzak/tokio-echo-test compiles to Wasm but doesn't work. I thought that by sharing the code I have so far, I could receive some feedback and pointers.

@Thomasdezeeuw
Copy link
Collaborator

Ping @rjzak.

@rjzak
Copy link
Contributor Author

rjzak commented May 31, 2022

@Thomasdezeeuw I'm still working on it!

@rjzak
Copy link
Contributor Author

rjzak commented Jun 1, 2022

@Thomasdezeeuw I have Axum & Tokio working on WebAssembly.
Still baffled at the moment with that to do with Arc<Registry>. This is where it's used in Tokio: https://github.com/rjzak/tokio/blob/wasi_wip/tokio/src/io/driver/mod.rs#L145. I could use some assistance.

@Thomasdezeeuw
Copy link
Collaborator

@Thomasdezeeuw I have Axum & Tokio working on WebAssembly. Still baffled at the moment with that to do with Arc<Registry>. This is where it's used in Tokio: https://github.com/rjzak/tokio/blob/wasi_wip/tokio/src/io/driver/mod.rs#L145. I could use some assistance.

Maybe you can put the Selector in an Arc?

@rjzak rjzak changed the title Wasm improvements Wasm32-wasi target support Jun 3, 2022
@rjzak
Copy link
Contributor Author

rjzak commented Jun 3, 2022

@Thomasdezeeuw @carllerche I moved the Arc<> to Registry::selector, and all seems to be okay, except for the tests on Windows. On Windows, the Poll::poll() function is failing with an error stating that a mutable reference is needed for Arc<Selector>, however, the function Selector::select() doesn't take a mutable reference to itself, so this shouldn't be a problem, unless I'm misunderstanding the error.

Line in question:

mio/src/poll.rs

Line 378 in 6312041

self.registry.selector.select(events.sys(), timeout)

Error:

error[E0596]: cannot borrow data in an `Arc` as mutable
   --> src\poll.rs:378:9
    |
378 |         self.registry.selector.select(events.sys(), timeout)
    |         ^^^^^^^^^^^^^^^^^^^^^^ cannot borrow as mutable
    |
    = help: trait `DerefMut` is required to modify through a dereference, but it is not implemented for `std::sync::Arc<sys::windows::selector::Selector>`

Otherwise, formatting issues and the previously API difference between platforms has been resolved.

@rjzak rjzak marked this pull request as ready for review June 3, 2022 18:40
@rjzak rjzak requested a review from Thomasdezeeuw June 3, 2022 23:11
Copy link
Collaborator

@Thomasdezeeuw Thomasdezeeuw left a comment

Choose a reason for hiding this comment

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

Two small nits, LGTM otherwise.

src/sys/wasi/mod.rs Outdated Show resolved Hide resolved
src/poll.rs Outdated Show resolved Hide resolved
Signed-off-by: Richard Zak <richard@profian.com>
@Thomasdezeeuw Thomasdezeeuw merged commit bcb6dd7 into tokio-rs:master Jun 4, 2022
@Thomasdezeeuw
Copy link
Collaborator

Thanks @rjzak.

@rjzak
Copy link
Contributor Author

rjzak commented Jun 4, 2022

Thank you!

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.

None yet

3 participants