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: add wasi-0.3.0 draft #111

Open
wants to merge 30 commits into
base: main
Choose a base branch
from

Conversation

rvolosatovs
Copy link

@rvolosatovs rvolosatovs commented Jan 14, 2025

refs https://github.com/orgs/bytecodealliance/projects/16?pane=issue&itemId=88153506

Followed the example of wasi-http and wasi-clocks duplicating the package in a subdirectory

This vendors wasi-clocks from WebAssembly/wasi-clocks#77

  • In wasip3 return values of functions are pollable by guests, therefore remove abstractions for which there is no need anymore:
    • Replace start-X/finish-X function pairs by X
    • Remove would-block error code
    • Remove not-in-progress error code
  • Replace wasi:io/error.error usage by error-context
  • Replace wasi:io/streams.input-stream return values by stream<u8> in return position
  • Replace wasi:io/streams.output-stream return values by stream<u8> in parameter position - Guests should be able to rely on stream.new to construct streams

@rvolosatovs rvolosatovs force-pushed the feat/0.3.0-draft branch 2 times, most recently from 605a6b4 to 92f2c42 Compare January 14, 2025 11:44
@rvolosatovs rvolosatovs marked this pull request as ready for review January 14, 2025 11:44
Followed the example of `wasi-http` and `wasi-clocks` duplicating the
package in a subdirectory

- In wasip3 return values of functions are pollable by guests,
  therefore remove abstractions for which there is no need anymore:
     - Replace `start-X`/`finish-X` function pairs by `X`
     - Remove `would-block` error code
     - Remove `not-in-progress` error code
- Replace `wasi:io/error.error` usage by `error-context`
- Replace `wasi:io/streams.input-stream` return values by `stream<u8>`
  in return position
- Replace `wasi:io/streams.output-stream` return values by `stream<u8>`
  in parameter position
     - Guests should be able to rely on `stream.new` to construct
       streams

Signed-off-by: Roman Volosatovs <[email protected]>
This seems to be better aligned with latest specification on error context

https://github.com/WebAssembly/component-model/blob/cbdd15d9033446558571824af52a78022aaa3f58/design/mvp/Explainer.md#error-context-type

> A consequence of this, however, is that components *must not* depend on the
> contents of `error-context` values for behavioral correctness. In particular,
> case analysis of the contents of an `error-context` should not determine
> *error recovery*; explicit `result` or `variant` types must be used in the
> function return type instead (e.g.,
> `(func (result (tuple (stream u8) (future $my-error)))`).

Signed-off-by: Roman Volosatovs <[email protected]>
@rvolosatovs
Copy link
Author

After reading the error-context spec a bit more, it seems these are not meant to be introspected, therefore I added 8cc04c8

https://github.com/WebAssembly/component-model/blob/cbdd15d9033446558571824af52a78022aaa3f58/design/mvp/Explainer.md#error-context-type

A consequence of this, however, is that components must not depend on the
contents of error-context values for behavioral correctness. In particular,
case analysis of the contents of an error-context should not determine
error recovery; explicit result or variant types must be used in the
function return type instead (e.g.,
(func (result (tuple (stream u8) (future $my-error)))).

@lukewagner perhaps you could confirm that the changes in that commit are moving in the direction you'd expect?

@lukewagner
Copy link
Member

Yes, you're right about the direction; thanks for asking! Indeed, error-context is a value type (not a resource type) and so borrow<error-context> shouldn't even validate.

I'm not that familiar with wasi-sockets, but I wonder if we could simplify connect a bit so that we didn't have one function returning such a complex type. Could it look a bit more like:

resource tcp-socket {
  connect:func(n: borrow<network>, remote: ip-socket-address) -> result<tcp-connection, error-code>;
}
resource tcp-connection {
  write: func(tx: stream<u8>) -> result<option<error-code>>;
  read: func() -> result<tuple<stream<u8>, future<option<error-code>>>>;
}

(where read and write return fail if called while a preceding read/write is active)? This is roughly analogous to what we do in wasi-http for consuming bodies.

To wit, once we add stream<T,U> (not in 0.3.0, but in some 0.3.x follow-up), tcp-connection.read could be simplified to:

  read: func() -> result<stream<u8, option<error-code>>

cc @dicej for any other thoughts

rvolosatovs and others added 19 commits January 15, 2025 12:12
Signed-off-by: Roman Volosatovs <[email protected]>
…ymore, `listen` is free to do an implicit bind again. Just like POSIX.
… already covers all its use-cases. Was an oversight in v0.2
…ymore, `stream` is free to do an implicit bind again. Just like POSIX.
…imal, but now that streams are actually a thing, it was even more akward.
…i-sockets into pr/111

# Conflicts:
#	wit-0.3.0-draft/ip-name-lookup.wit
```
Error: failed to resolve directory while parsing WIT for path [wit-0.3.0-draft]

Caused by:
    0: failed to process feature gate for type [duration] in package [wasi:[email protected]]
    1: feature gate cannot reference unreleased version 0.3.0 of package [wasi:[email protected]] (current version 0.3.0-draft)
```
@badeend
Copy link
Collaborator

badeend commented Jan 16, 2025

Thanks @rvolosatovs for getting this off the ground!


I have taken liberty to directly implement some of my suggestions, because doing it this way is hopefully faster than needing to type it all out upfront. Let me know what you think:

  • I've renamed tcp-connection write to send, and read to receive, to match pre-existing terminology.
  • Now that we have proper streams with cancellation etc, the TCP shutdown method is redundant. I've removed the function and updated the docs on send & receive.
  • I've merged listen & accept into a single method that returns a stream of client sockets.
  • I've renamed udp-socket.%stream to udp-socket.transfer. The %stream name was already suboptimal, but now that streams are actually a thing, it was even more akward.
  • UDP send was the only function remaining working on lists. I've removed the separate send function and replaced that with a stream parameter on the transfer function. Similar to how you had the TCP connect signature initially.
  • I've changed resolve-addresses to return a list instead of a stream. In practice the amount of results are usually countable on one hand. So I think a list is more appropriate here.

Also, since we're breaking compatibility, this seems like a good moment to freshen up the interface a bit.

Even though wasi-sockets is technically a v0.2.0 interface, some aspects of were very much still designed with a v0.1.0 mindset. So:

  • I've removed the runtime network capability. AFAIK, no one actually implemented that in any useful way, nor could they, because wasi-libc depends on there being only one network instance. Now, the interface relies solely on link-time capability, like the other proposals. This means TCP listen & UDP transfer are free to do an implicit bind again. Just like POSIX.
  • I've removed the create-tcp/udp-socket methods & interfaces in favor of regular constructors on the resource types.
  • Renamed network.wit to types.wit, and moved tcp-socket & udp-socket into types.wit to match other proposals.
  • I've remove superfluous concurrency-conflict error code. invalid-state already covers all its use-cases. Was an oversight in v0.2

Documentation updates:

  • Added TcpSocketOperationalSemantics-0.3.0-draft.md to contain the updated documentation.
  • Add @since annotations to the new tcp-connection type.
  • The docs on TCP connect still mentioned the previous parameter which has been superseded by tcp-connection.
  • Remove some last references to wasi:io

Finally, I don't know if I'm missing some CLI flags or something, but for me wit-bindgen fails with:

Error: failed to resolve directory while parsing WIT for path [wit-0.3.0-draft]

Caused by:
    0: failed to process feature gate for type [duration] in package [wasi:[email protected]]
    1: feature gate cannot reference unreleased version 0.3.0 of package [wasi:[email protected]] (current version 0.3.0-draft)

I've changed the package version to make it happy. Don't know if this was the correct thing to do, but at least it works for me now :)

I'm on [email protected]


Let me know what you think!

@sunfishcode
Copy link
Member

Here and in similar functions in wasi-filesystem, I wonder if we could avoid the inner result in functions like this:

receive: func() -> result<tuple<stream<u8>, future<result<_, error-code>>>>;

which could look like this instead:

receive: func() -> result<tuple<stream<u8>, future<error-code>>>;

The idea is, stream.read already returns a read-status indicating whether the stream is closed and whether it failed, so we don't need a separate result to tell us whether a failure occurred. Users can simply drop the future rather than awaiting it if the stream succeeds. Does that sound reasonable?

@rvolosatovs
Copy link
Author

rvolosatovs commented Jan 16, 2025

Here and in similar functions in wasi-filesystem, I wonder if we could avoid the inner result in functions like this:

receive: func() -> result<tuple<stream<u8>, future<result<_, error-code>>>>;

which could look like this instead:

receive: func() -> result<tuple<stream<u8>, future<error-code>>>;

The idea is, stream.read already returns a read-status indicating whether the stream is closed and whether it failed, so we don't need a separate result to tell us whether a failure occurred. Users can simply drop the future rather than awaiting it if the stream succeeds. Does that sound reasonable?

I was going back-and-forth on this one and I felt like perhaps a "future that may never resolve" is a concept harder to grasp for developers as opposed to a "future that always resolves to something". It also lets us avoid outer result altogether and instead return tuple<stream<T>, future<result>> directly

So I'd go for:

receive: func() -> tuple<stream<u8>, future<result<_, error-code>>>;

And soon:

receive: nonblocking func() -> tuple<stream<u8>, future<result<_, error-code>>>;

@badeend
Copy link
Collaborator

badeend commented Jan 16, 2025

The suggestion of @rvolosatovs seems the most natural to me:

receive: func() -> tuple<stream<u8>, future<result<_, error-code>>>;

especially since that may be shortened even further to:

receive: func() -> stream<u8, error-code>;

depending on the progress of error-contexts

@rvolosatovs
Copy link
Author

Thanks @badeend, your changes look good to me, just left one comment.

Signed-off-by: Roman Volosatovs <[email protected]>
rvolosatovs added a commit to rvolosatovs/wasi-filesystem that referenced this pull request Jan 17, 2025
This ensures that e.g.:

```
wit-bindgen markdown wit-0.3.0-draft -w imports --html-in-md
```

works with wit-bindgen 0.37

refs:
WebAssembly/wasi-sockets@3abda6e

refs:
WebAssembly/wasi-sockets#111 (comment)

Signed-off-by: Roman Volosatovs <[email protected]>
Co-authored-by: Roman Volosatovs <[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.

4 participants