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

refactor: allow Subscribe::ready to be fallible #7713

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rvolosatovs
Copy link
Member

@rvolosatovs rvolosatovs commented Dec 20, 2023

In the current implementation Subscribe::ready is assumed to be infallible, which does not work in a generic case, where the implementation may require, in fact, to cause a trap in the guest.
This is important, because implementations that require ability to trap in host wasi::io::poll/pollable would otherwise require to implement their own wasi::io::poll/pollable, meaning that they would not be able to reuse all the existing WASI crate implementations (https://docs.rs/wasmtime-wasi/latest/wasmtime_wasi/preview2/trait.Subscribe.html#implementors), potentially locking developers out of most of WASI functionality (due to the Pollable concrete type being used in signatures, which cannot be externally constructed)

@rvolosatovs rvolosatovs requested a review from a team as a code owner December 20, 2023 17:10
@rvolosatovs rvolosatovs requested review from alexcrichton and removed request for a team December 20, 2023 17:10
@rvolosatovs rvolosatovs marked this pull request as draft December 20, 2023 17:46
@rvolosatovs rvolosatovs marked this pull request as ready for review December 20, 2023 17:51
@github-actions github-actions bot added the wasi Issues pertaining to WASI label Dec 20, 2023
Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
@alexcrichton
Copy link
Member

Could you describe a bit more when an embedder might want to return a trap via this method? I agree the current interface doesn't allow it, but one reason we chose to not do this is to avoid a common mistake of performing I/O and returning the error in the ready method instead of buffering up the error to get returned from an accessor elsewhere.

@rvolosatovs
Copy link
Member Author

rvolosatovs commented Jan 8, 2024

Could you describe a bit more when an embedder might want to return a trap via this method? I agree the current interface doesn't allow it, but one reason we chose to not do this is to avoid a common mistake of performing I/O and returning the error in the ready method instead of buffering up the error to get returned from an accessor elsewhere.

First I'll comment on this part:

buffering up the error to get returned from an accessor elsewhere

This may not be possible in a generic case, because for this to work the host would need to know which accessor is coupled with the pollable and this relationship is not in any way encoded in WIT. In other words, the host must be aware of the executed Wasm component runtime behavior/logic in order to expose the error in the accessor. Consider wasi:http/types.future-trailers, for example:

resource future-trailers {
  subscribe: func() -> pollable;
  get: func() -> option<result<result<option<trailers>, error-code>>>;
}

Because the host at compile time is aware of the behavior of wasi:http/future-trailers, it knows that if an error was encountered in Subscribe::ready implementation of the returned pollable, wasi:http/future-trailers.get should return it.

But in case of some generic resource, which is only known at runtime, e.g.:

resource custom-trailers {
  subscribe-one: func() -> pollable;
  subscribe-two: func() -> pollable;
  get: func() -> option<result<result<option<trailers>, error-code>>>;
  foo: func() -> string;
}

The host may not in any way know how to communicate the error (and which one) to the guest. The interface, may, in fact, not even utilize an accessor and simply rely on the fact that the pollable has returned "ready". This is precisely the behavior of wasi-clocks timeouts https://github.com/WebAssembly/wasi-clocks/blob/52335b59451193cb36830588298085a76fc78ff1/wit/monotonic-clock.wit#L33-L37.

Coming back to the first part:

My exact use case is a "pollable", which is a remote entity accessible via network:

  1. Subscribe::ready establishes the connection to the remote peer
  2. Any relevant data is communicated over the connection, an empty response is received by the caller
  3. Subscribe::ready unblocks

If network connection in 1. fails, it can just be retried, perhaps with backoff - that part works well with existing API.
If, however, (unrecoverable) protocol error is encountered in 2. - causing a trap in the guest seems to be the only reasonable approach, since the remote entity state is not known and there is no generic way to communicate the error to the guest (and such may not be available even when the interface is known at compile time). Returning in this case could break assumptions of the Wasm component logic and blocking forever would stall execution forever (e.g. if no timeout is used in the Wasm component poll)

@alexcrichton
Copy link
Member

Ok makes sense, thanks for explaining. Can this be fixed though by updating the WIT-level APIs?

For example in your custom-trailers I would say that what subscribe and the pollables mean depends on how the WIT APIs are defined. It would be an API contract that subscribe-one would indicate readiness of get and subscribe-two would indicate readiness of foo where foo would have to semantically define what it means to be called before it's ready, etc.

In your use case of a remote-owned resource, this is something that I'd at least personally expect to go through an error somewhere in the WIT. One option would be that when the pollable says "ready" any future operations on it trap (communicating the fatal error) or by updating the APIs there to surface an error instead.

@rvolosatovs rvolosatovs marked this pull request as draft January 8, 2024 18:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wasi Issues pertaining to WASI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants