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

Make async-std compile for wasm32-unknown-unknown #225

Closed
wants to merge 4 commits into from

Conversation

tomaka
Copy link

@tomaka tomaka commented Sep 21, 2019

Addresses the first point of #220: making the crate compile for wasm32-unknown-unknown.

All we have to do is to only depend on mio when appropriate, and to return an error when the user attempts to create a TCP or UDP connection.

I can see mostly three use cases for WASM: no-std, browers, and desktop. In the first two situations, TCP and UDP aren't available. In the third situation, the only way we could implement TCP/UDP are through some custom syscalls, which is exactly what WASI is.
In other words, to me, if you're compiling for wasm32-unknown-unknown, there is no situation where you would have TCP/UDP available to you.

It is debatable whether we should return an error at runtime or make TcpListener, TcpStream and UdpSocket unavailable at compile-time, but I went for erroring at runtime in order to mimic what the standard library does.

I'm opening this PR as a draft. I've updated async_std::net::udp to return an error when the operating system is "unknown", but since the changes are a bit opinionated, I wanted some feedback before doing the same for TCP.

@skade
Copy link
Collaborator

skade commented Sep 23, 2019

It seems to fail on a missed cfg (TcpListener) and a warning about unused code, which we should deactivate for those locations.

I'm not sure if I agree with the strategy, but also don't disagree: libstd to my knowledge does it for compatibility of legacy codebases, which we don't have. On the other hand, I see this legacy building, because people may not start with WASM support immediately.

https://travis-ci.com/async-rs/async-std/jobs/237796226

@tomaka
Copy link
Author

tomaka commented Sep 23, 2019

It seems to fail on a missed cfg (TcpListener) and a warning about unused code, which we should deactivate for those locations.

I know it's broken, the PR isn't finished 😅

Before finishing it I wanted to ask if I'm doing the right thing, notably w.r.t. disabling at runtime vs at compile-time.

@skade
Copy link
Collaborator

skade commented Sep 23, 2019

@tomaka Yeah, I mainly got into the habit of documenting what's currently broken, as Travis interface got really slow to click through... Should have marked it as such.

@tomaka tomaka marked this pull request as ready for review September 24, 2019 07:36
@tomaka
Copy link
Author

tomaka commented Sep 24, 2019

This is now ready for review.

From a usability perspective, I tend to prefer the "panic at runtime" strategy.
This way, people can compile their existing code to wasm32-unknown-unknown without having to add tedious cfg_if! blocks.

Additionally, when it comes to sockets and file descriptors, even on Linux you can't rely on the fact that they're going to work. The OS might just refuse them to you for whatever reason pleases it.

Returning an error that says "TCP and UDP aren't available on this platform" on WASM isn't very different from returning an error that says "the kernel has decided to not allow you to open a socket" on Windows or Linux.

@skade
Copy link
Collaborator

skade commented Sep 24, 2019

I can agree with that reasoning. If this is merged this way, we should document it in the docs/book.

Would it make sense to return io::Error of kind "Other" in that case?

@tomaka
Copy link
Author

tomaka commented Sep 25, 2019

Would it make sense to return io::Error of kind "Other" in that case?

That's what I'm doing. I don't really see any kind that's more appropriate. Maybe "PermissionDenied".

@tomaka
Copy link
Author

tomaka commented Oct 2, 2019

I suppose that this PR is blocked on hesitations about the design?

@yoshuawuyts yoshuawuyts added the enhancement New feature or request label Oct 8, 2019
@yoshuawuyts
Copy link
Contributor

@tomaka sorry about the delay; I guess between flu and travel everyone kind of fell off the map. I'm meeting up with @stjepang tomorrow, and we should probably chat about how to move this forward. WASM support is definitely something we want, and being able to move this forward would be fantastic!

@skade
Copy link
Collaborator

skade commented Oct 28, 2019

I would like to get that landed, soon, to make sure we still have some time to best it before release.

@skade skade added the nominated for 1.0 Nominated for 1.0 release label Oct 28, 2019
@tomaka
Copy link
Author

tomaka commented Oct 28, 2019

I would like to get that landed, soon, to make sure we still have some time to best it before release.

This PR is kind of the starting point to supporting WASM, but we need a couple more changes before it actually works.

If the 1.0 release is planned for November 7th, it's almost a bit too late.

@yoshuawuyts
Copy link
Contributor

yoshuawuyts commented Oct 28, 2019

@tomaka sorry about this PR being held up. This def feels like it's been on me.

So in its current form this changes the code around quite a bit by delegating to specific methods. Which usually would be fine, but we have a lot of code, and I fear this might complicate it somewhat.

I think ideally we would have an attribute to blank out internal impls in WASM, somewhat like this:

impl PathBuf {
    #[not_browser]
    pub async fn canonicalize(/* args */) -> io::Result<()>;
}

But for compilation performance reasons we don't really want to depend on proc macros. So instead I've been thinking about is perhaps we could write a macro_rules macro similar to our extension_trait! macro in src/utils.rs that would stub out implementations for WASM.

io_struct! {
    struct PathBuf {}
    impl PathBuf { /*impl goes here */ }
}

Currently I believe all IO structs' methods become no-ops. If we wanted more fine-grained control we could perhaps follow pin-project-lite's lead and introduce a similar attribute (#[no_browser] or similar).

I still think that WASM support would be fantastic, and the sooner we can get there the better (though it's probably also okay to not have it in the next week, heh). How does this sound?

@tomaka
Copy link
Author

tomaka commented Oct 28, 2019

I'm not sure how to implement that without introducing more complexity in the code than what this PR introduces.

For example we need to override the struct fields, the From<std::net::...> implementations, the Incoming iterator of the TcpListener, and the Read/Write traits. Doing so through a macro that wraps around everything is far from being easy.

@yoshuawuyts
Copy link
Contributor

yoshuawuyts commented Oct 28, 2019

@tomaka ah yeah, that's really fair.

Alternative suggestion: let's model this directly like std:

src/
    sys/
        wasm/
            net.rs
            fs.rs
            os.rs
        windows/
            # etc.
        unix/
            # etc.

Perhaps windows and unix should be merged into a single file (common?, native?), or perhaps re-export from each other. But by following the way std is setup it might make this nicer to work with overall?

I guess my current thinking is that the cfg_if blocks inside the different files don't feel great. If someone wants to find out a TcpListener is created, the implementation will live somewhere in the middle of a file together with all other platform code. Sometimes hidden behind an else. This is not as nice to work with as when it would have a logical place in the file hierarchy (e.g. src/sys/native/net.rs).

Does this make sense?

@skade
Copy link
Collaborator

skade commented Oct 31, 2019

I've been thinking about his a little and I think there's an opportunity for a different design here. Currently, we're approaching this like libstd, where cargo or our build tooling can't help us. We're not in the sysroot, though.

How about having a feature flag wasm32, which switches everything that we can't support off? I'm not sure how feasible this is maintenance wise or if there's a better approach to reach this.

Essentially, the goal would be that users that want to make their application compatible with WASM environments could rely on async-std with the wasm feature on, so that they will never accidentally use API that isn't intended for WASM.

The danger I see is having to label a lot of async-std as "not compatible with wasm", but we already kinda have to do that?

tl;dr:

async-std = { version = "0.99", features = "wasm" }

would reduce the interface to something that works. This would make porting over easier, too, as things will fail to build.

@dignifiedquire
Copy link
Member

Thank you, fixed in #757

@tomaka tomaka deleted the wasmify branch May 8, 2020 09:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request wasm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants