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

feat(wasm): add support for wasip2, aka webassembly components, using wasi-http #2290

Draft
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

brooksmtownsend
Copy link

@brooksmtownsend brooksmtownsend commented May 17, 2024

Fixes #2294

Draft as this depends on servo/rust-url#960

  • Should resolve the below comment re: future polling

Hey there! This is a work-in-progress PR that adds support for using reqwest with wasi-http. It was really neat to see the support for wasm_bindgen and JavaScript bindings for requests, and this PR helps support the component model in addition to wasm_bindgen.

I wanted to open this PR to see if you had any suggestions, requests from the initial working code/refactor perspective, and if there's any additional material you'd like me to clarify. If this fits within your general contribution guidelines, I'll wrap it up and mark as ready for review!

This code is still very littered with unwraps and comments, so go easy on me if you take a look at the implementation 😅 . To name a few, here's the things I want to do still for this PR:

  • General code cleanliness, no doc comments simply added as dox, kill unwraps, expects, and panics
  • Add example that I'm using for testing under the examples dir
  • Gate wit-bindgen behind the wasm32 family of deps
  • See if I can better toggle between the JS/bindgen Wasm support and the component model wasm support using directives instead of a feature (there is the wasm32-wasip2 target but that's not very widely used yet.) The goal here is to not affect any existing use cases of wasm -> reqwest and add support for components.
  • Consider a custom pollable to remove the technically blocking code in the request

Last but not least, thanks for this library! Love reqwest ❤️

@seanmonstar
Copy link
Owner

Thanks for taking a look at this! I think support WASI is likely a good goal, in general. However, I usually try to have the discussion in an issue first. I find it's helpful to find consensus before spending too much time going down a certain implementation path, or even working on a feature that won't be accepted at all.

So while I do appreciate exploring the area, and exploration is often needed to inform more complex features, I do want to set proper expectations: we might not do this, or do it very differently. 🙈

@brooksmtownsend
Copy link
Author

@seanmonstar that's why I opened the draft PR! Glad I ended up doing that before I got too far down the road. I can open up an issue then and point at this branch as a reference if that's what you prefer

@brooksmtownsend
Copy link
Author

brooksmtownsend commented May 17, 2024

Oh fun, #1956 and #1977 exist. Do we need another issue? Looks like the implementation in #1956 is also an option for comparison, albeit not quite on [email protected].

@brooksmtownsend
Copy link
Author

Created #2294 to further the discussion, @seanmonstar please feel free to either close this PR for now while we continue in the issue or leave it open, totally up to the way you like to track the OSS work.

@brooksmtownsend brooksmtownsend force-pushed the feat/wasi-p2-component-support branch 2 times, most recently from 7a17bbe to f623025 Compare May 22, 2024 16:33
@brooksmtownsend
Copy link
Author

brooksmtownsend commented May 22, 2024

Rebased this PR off of main and added a component example so folks can see this in action. With reqwest it is really nice looking

@pimeys
Copy link

pimeys commented Jul 29, 2024

Hey. I've been playing around with reqwest, tokio and wasm components, and presented a bit of a demo to a customer how to use these together. You might want to check and pull some of my changes. I implemented async response handling and my fork can now also send a body with the request.

https://github.com/pimeys/reqwest/tree/feat/wasi-p2-component-support

Any comments and ideas are welcome, and I might also join the effort later on to get this to the mainline. We just need the functionality now, so took your code and modified it a bit.

@brooksmtownsend
Copy link
Author

Thanks @pimeys ! I'll have to take a look at your implementation. I was honestly considering making the component implementation blocking/sync for now, since wasi-http doesn't have any native async.

I'm happy to integrate any improvements into this fork

@pimeys
Copy link

pimeys commented Jul 29, 2024

Wait... There is enough functionality to make it work. I measured a tokio join to an endpoint with your original PR and with my changes to an endpoint with a steady 1s response time.

Mine was 1s and yours 3s for three concurrent requests.

The wit definitions do have async versions of the needed apis.

@pimeys
Copy link

pimeys commented Jul 29, 2024

This here allows async writing of body in the request:

https://github.com/pimeys/reqwest/blob/feat/wasi-p2-component-support/src/wasm/component/wit/deps/io/streams.wit#L117-L132

This one here after a request is done returns a future of a response:

https://github.com/pimeys/reqwest/blob/feat/wasi-p2-component-support/src/wasm/component/wit/deps/http/types.wit#L551

Which can be polled in a non-blocking way, and then determined if the response is received or not.

Edit: I based my changes to your branch @brooksmtownsend, so you should be able to rebase my changes if you want.

@brooksmtownsend
Copy link
Author

@pimeys do you have an example component that you used with these changes?

@pimeys
Copy link

pimeys commented Jul 31, 2024

Here's one

https://github.com/grafbase/grafbase/tree/main/examples/gateway-hooks

@brooksmtownsend
Copy link
Author

The wit definitions do have async versions of the needed apis.

Yep! You're correct, all I meant to say was that currently functions that you can export with components have to be sync, so it requires either blocking with an async runtime or executor. So I was considering making this sync to make the API easier, but I think it's a common enough operation in Rust to do this.

Looking at brooksmtownsend/reqwest@feat/wasi-p2-component-support...pimeys:reqwest:feat/wasi-p2-component-support it looks slick, I may just take it entirely!

@pimeys
Copy link

pimeys commented Aug 2, 2024

So what wasmtime does here is they have so called co-operative execution of the guest function. You can either use fuel or implement an epoch based yielding with it. This means you can run a tokio runtime with guest components, which will block but wasmtime yields every now and then and lets other futures execute things while going.

If you want async execution in a guest component, you will need a second runtime. We have some customers that expect to run async http requests in the guest code, so we really need an http client for Rust that can do that. And, luckily it is not that hard to implement!

@brooksmtownsend
Copy link
Author

brooksmtownsend commented Aug 15, 2024

@pimeys curious if you have any recommendations for 33aa9d7, I've pulled in your changes but unfortunately for the component included I wasn't able to actually use the async support you added, the future was just polled in a hot loop. The sleep isn't ideal and I'm wondering if you get the same result as me, but I assume your example worked.

I updated the example included to build using the nightly target wasm32-wasip2 and the wasi crate, which removes the need to use generated WIT bindings in the component or the crate. I also pushed a refactor that removed some of the extra implementations that weren't correct or necessary here, like multipart, in order to simplify the initial review process. We can certainly add that back in as necessary

Comment on lines +71 to +76
// NOTE(brooksmtownsend): We shouldn't be waking here since we don't know that
// the future is ready to be polled again. Sleeping for a nanosecond appears to
// allow the future to be polled again without causing a busy loop.
std::thread::sleep(std::time::Duration::from_nanos(1));
cx.waker().wake_by_ref();
return Poll::Pending;
Copy link
Author

Choose a reason for hiding this comment

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

This sleep is the only part of this PR that needs to be addressed before review. We should consider storing the waker & using a background task to wait for the future to be ready before waking the context. Not sleeping for a nanosecond here will infinitely hot-loop polling the future

@brooksmtownsend brooksmtownsend force-pushed the feat/wasi-p2-component-support branch 2 times, most recently from 7d636e2 to 3ef4eb4 Compare August 21, 2024 21:04
pimeys and others added 6 commits August 22, 2024 11:47
Signed-off-by: Brooks Townsend <[email protected]>

feat(wasm): update component to use wasip2 target

Signed-off-by: Brooks Townsend <[email protected]>

feat(wasm): update component to use wasip2 target

Signed-off-by: Brooks Townsend <[email protected]>
Signed-off-by: Brooks Townsend <[email protected]>
Signed-off-by: Brooks Townsend <[email protected]>
Signed-off-by: Brooks Townsend <[email protected]>

io::copy instead of streams

Signed-off-by: Brooks Townsend <[email protected]>
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.

Support WebAssembly Component bindings, aka wasip2, in addition to javascript bindings
3 participants