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

Enable cfg_fs for wasi #6822

Merged
merged 4 commits into from
Sep 11, 2024
Merged

Enable cfg_fs for wasi #6822

merged 4 commits into from
Sep 11, 2024

Conversation

namse
Copy link
Contributor

@namse namse commented Sep 4, 2024

Motivation

We are currently using wasip1-threads and have implemented our own wasi fs. However, there is a restriction that prevents using the file system feature when cfg_fs is set to wasi. I believe this restriction should be lifted, as it contradicts the intention stated in the Tokio code, where enabling the tokio_unstable flag allows the use of the file system feature (relevant code link).

Solution

I have removed the wasi condition from cfg_fs to enable the file system feature. After forking Tokio and applying this change, I tested it and found that it works perfectly. This change will allow support for the file system feature in the wasip1-threads environment.

@namse
Copy link
Contributor Author

namse commented Sep 7, 2024

Is there a maintainer who can review this PR? Our team primarily uses WASI, but this issue is a major obstacle in using Tokio with WASI. We would greatly appreciate any help.

@Darksonn Darksonn added A-tokio Area: The main tokio crate M-fs Module: tokio/fs T-wasm Topic: Web Assembly labels Sep 10, 2024
@Darksonn
Copy link
Contributor

I believe that this was added because std::fs relies on spawn_blocking but wasm is single-threaded. I understand that the threading situation has improved recently. Are you able to confirm that this actually works?

@namse
Copy link
Contributor Author

namse commented Sep 10, 2024

Yes, this works well. I have already forked Tokio and am using Tokio FS in a web browser with the fixes from this PR. However, I decided to create this PR because using the fork continuously could cause compatibility issues with other crates.

Not all WASM, or more specifically, WASI, is single-threaded. Among Rust targets, there are wasm32-wasip1, wasm32-wasip1-thread, and wasm32-wasip2. wasm32-wasip1 does not support threads, but wasm32-wasip1-thread supports pthread-based threading. Honestly, I’m not too familiar with wasip2.

WASI support in Tokio is already experimental. Also, since WASI itself is quite experimental, I believe most WASI users would typically check whether the WASI runtime they are using supports threads or not. Therefore, I don't think there would be a significant issue in opening up the configuration to allow using Tokio FS in WASI environments that do support threads.

@Darksonn
Copy link
Contributor

Ok, I don't mind adding this. There's a compilation failure, though.

/// Returns a mutable reference to the underlying `std::fs::OpenOptions`
pub(super) fn as_inner_mut(&mut self) -> &mut StdOpenOptions {
&mut self.0
}

We can fix it like this:

/// Returns a mutable reference to the underlying `std::fs::OpenOptions`
#[cfg(any(windows, unix))]
pub(super) fn as_inner_mut(&mut self) -> &mut StdOpenOptions {
    &mut self.0
}

@namse
Copy link
Contributor Author

namse commented Sep 10, 2024

If the compile error is due to the function not being used in WASI, would it be okay to include a fix for that in this PR as well?

It might be a good idea for me to modify the PR to ensure there are no compile errors overall and then request a review from you.

@Darksonn
Copy link
Contributor

Go ahead.

@namse
Copy link
Contributor Author

namse commented Sep 11, 2024

I thought I would need more changes, but it's not. There's nothing more except for the modifications you mentioned. Please check.

Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

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

Thank you.

@Darksonn Darksonn enabled auto-merge (squash) September 11, 2024 06:52
@namse
Copy link
Contributor Author

namse commented Sep 11, 2024

Github Action Canceled https://github.com/tokio-rs/tokio/actions/runs/10803632984/job/29967823508?pr=6822
What should I do?

@Darksonn
Copy link
Contributor

You don't need to do anything. I will rerun.

@Darksonn Darksonn merged commit d621359 into tokio-rs:master Sep 11, 2024
81 checks passed
@namse
Copy link
Contributor Author

namse commented Sep 11, 2024

@Darksonn If it's not too much trouble, may I ask when this change is expected to be released?

@namse namse deleted the patch-1 branch September 11, 2024 13:00
@Darksonn
Copy link
Contributor

We don't have very many changes since the last release, so it may be another month. But if you are willing to do the work of preparing the release, we can make one now.

@namse
Copy link
Contributor Author

namse commented Sep 11, 2024

After reviewing this document (https://github.com/tokio-rs/tokio/blob/master/CONTRIBUTING.md#releasing), I believe I can handle it. I have some other tasks to finish first, but once I'm done, I'll create a PR for the release.

@Darksonn
Copy link
Contributor

The part about path dependencies isn't needed anymore, so you can skip that step.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate M-fs Module: tokio/fs T-wasm Topic: Web Assembly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants