Component support for WASI p2

[william-stacken]

Fixes #432

Feedback welcome!

[jeffcharles]

Thanks! These changes are awesome!

Can you add a fixture for exercising allow_tcp, allow_udp, and allow_ip_name_lookup and use it in some integration tests?

I'm thinking we can probably set up a listener on 127.0.0.1 for TCP and UDP and check Google or GitHub for DNS resolution but I'm not super married to a particular set of integration tests as long as we try to make them as not flakey as possible.

[jbourassa]

One thing worth keeping in mind: we don't release the GVL, so these IO operations will block other Ruby code from running (unlike regular IO in Ruby).

I think we should either:

1. Document that these network calls happen without the GVL released
2. Introduce an API for releasing the GVL
3. Figure out how to acquire / release the GVL automatically for IO (if at all possible?)

I think (1). is a low-hanging fruit, other options are more useful but significantly more work.

At any rate, thanks for your contribution.

[saulecabrera]

I think (1). is a low-hanging fruit, other options are more useful but significantly more work.

IIRC, we have adopted a similar stance to option 1 in other areas, e.g., https://github.com/bytecodealliance/wasmtime-rb/blob/main/ext/src/ruby_api/engine.rs#L151

[william-stacken]

IIRC, we have adopted a similar stance to option 1 in other areas

OK, then I'll add a comment about this to #inherit_network and the other methods.

As I was working on those integration tests I realized I had minunderstood how allow_tcp and allow_udp works. They don't allow any TCP/UDP connections, they must be combined with a socket_addr_check as by default all network access is disallowed. EDIT: Actually they can be combined with #inherit_network, so they have use even if there is no socket_addr_check.

In the issue it was mentioned that this could be left until later, do you think I should proceed with adding support for the socket addr callback? One idea to handle the case where the ruby block raises an exception is to catch the exception in rust-land(?) and then handle it by having the rust-land check return false to block the socket access.

[william-stacken]

I have now added a sample implementation of socket_addr_check, let me know what you think!

[saulecabrera]

Looks reasonable to me. Left one small comment around the determinism check. I'll defer to Jeff regarding the comments around tests.

[saulecabrera]

It is totally possible that there will be an increased combination of options that we will need to check here in the future to determine if this is deterministic or not, perhaps we could extract this into a function e.g., fn check_determinism() -> Result<()>

[saulecabrera]

One idea to handle the case where the ruby block raises an exception is to catch the exception in rust-land(?) and then handle it by having the rust-land check return false to block the socket access.

This is approach is reasonable, I think. One thing that is somewhat worrying is that this approach will effectively swallow any exception that happens in that path, right? I wonder if there is a possible middle ground here, in which can still report the error even if deferred? I do not recall if we have such pattern in the code base (@jbourassa might know best?)

Edit: typo

[jeffcharles]

Tests look good!

[jeffcharles]

Should this be wasip2? Or possibly removed?

[jbourassa]

I do not recall if we have such pattern in the code base (@jbourassa might know best?)

We do have that for host calls. If a host call raises, we can't bubble the exception immediately, thus save it on the store and raise it at the end of the execution.

It's harder here because we don't have access to the store. But maybe there's something we can do by creating some Send, Sync wrapper that we can set an error on, like an Arc? Far from ideal, though.

Ideally we could have access to the store, but I don't know if there are more use cases than ours (I haven't given it much thought).

[william-stacken]

It's harder here because we don't have access to the store. But maybe there's something we can do by creating some Send, Sync wrapper that we can set an error on, like an Arc? Far from ideal, though.

I made an attempt at preserving the exception by providing the store with a "WasiRetainedData" struct, which also is used to keep the socket addr check proc. Then the rust-level socket addr check handles errors from ruby by storing them in the WasiRetainedData, making it possible to raise the error after the wasm component finished executing. Let me know if you think it is an approach worth keeping or if it should be dropped or reworked.

[saulecabrera]

Is there a reason why we would need to expose this class? I think that this is an implementation detail and we should try to keep it that way.

Could we drop the Ruby class and store WasiRetainedData in the Store, similar to refs? I think the proc marking could be done transitively from the store?

[saulecabrera]

LGTM, thanks!