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

Start to lift restriction of stdio only once #3

Merged
merged 5 commits into from
Sep 28, 2023

Conversation

alexcrichton
Copy link

This is built on #2 so only the last commit is relevant for this particular piece.

This is definitely not perfect. In talking with Pat the way I've settled on thinking about this is that this is probably the best and/or most practical we can do for now in a preview2 time frame. The goal of preview2 is to collect feedback for a future iteration through preview3 or final WASI API or similar. For now though this is a best-effort with known bugs.

This commit adds new `{Stdin,Stdout}Stream` traits which take over the
job of the stdio streams in `WasiCtxBuilder` and `WasiCtx`. These traits
bake in the ability to create a stream at any time to satisfy the API
of `wasi:cli`. The TTY functionality is folded into them as while I was
at it.

The implementation for stdin is relatively trivial since the stdin
implementation already handles multiple streams reading it. Built-in
impls of the `StdinStream` trait are also provided for helper types in
`preview2::pipe` which resulted in the implementation of
`MemoryInputPipe` being updated to support `Clone` where all clones read
the same original data.
@alexcrichton
Copy link
Author

Ok I think the follow-up commits here should get the test suite passing as well

Copy link
Owner

@sunfishcode sunfishcode left a comment

Choose a reason for hiding this comment

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

Looks reasonable. It's a little inconvenient for users setting up a WasiCtx to implement these new StdinStream and StdoutStream traits, but not a lot, and it gives a chance for the impl to be aware of the clone and avoid the Arc/Mutex if it doesn't need them.

@sunfishcode sunfishcode merged commit b321022 into sunfishcode:resources Sep 28, 2023
sunfishcode pushed a commit that referenced this pull request Sep 28, 2023
* Start to lift restriction of stdio only once

This commit adds new `{Stdin,Stdout}Stream` traits which take over the
job of the stdio streams in `WasiCtxBuilder` and `WasiCtx`. These traits
bake in the ability to create a stream at any time to satisfy the API
of `wasi:cli`. The TTY functionality is folded into them as while I was
at it.

The implementation for stdin is relatively trivial since the stdin
implementation already handles multiple streams reading it. Built-in
impls of the `StdinStream` trait are also provided for helper types in
`preview2::pipe` which resulted in the implementation of
`MemoryInputPipe` being updated to support `Clone` where all clones read
the same original data.

* Get tests building

* Un-ignore now-passing test

* Remove unneeded argument from `WasiCtxBuilder::build`

* Fix tests
sunfishcode pushed a commit that referenced this pull request Sep 29, 2023
* Start to lift restriction of stdio only once

This commit adds new `{Stdin,Stdout}Stream` traits which take over the
job of the stdio streams in `WasiCtxBuilder` and `WasiCtx`. These traits
bake in the ability to create a stream at any time to satisfy the API
of `wasi:cli`. The TTY functionality is folded into them as while I was
at it.

The implementation for stdin is relatively trivial since the stdin
implementation already handles multiple streams reading it. Built-in
impls of the `StdinStream` trait are also provided for helper types in
`preview2::pipe` which resulted in the implementation of
`MemoryInputPipe` being updated to support `Clone` where all clones read
the same original data.

* Get tests building

* Un-ignore now-passing test

* Remove unneeded argument from `WasiCtxBuilder::build`

* Fix tests
@alexcrichton alexcrichton deleted the stdio-more-than-once branch October 25, 2023 19:13
sunfishcode added a commit that referenced this pull request Oct 30, 2023
…dealliance#7029)

* Rename `Host*` things to avoid name conflicts with bindings.

* Update to the latest resource-enabled wit files.

* Adapting the code to the new bindings.

* Update wasi-http to the resource-enabled wit deps.

* Start adapting the wasi-http code to the new bindings.

* Make `get_directories` always return new owned handles.

* Simplify the `poll_one` implementation.

* Update the wasi-preview1-component-adapter.

FIXME: temporarily disable wasi-http tests.

Add logging to the cli world, since stderr is now a reseource that
can only be claimed once.

* Work around a bug hit by poll-list, fix a bug in poll-one.

* Comment out `test_fd_readwrite_invalid_fd`, which panics now.

* Fix a few FIXMEs.

* Use `.as_ref().trapping_unwrap()` instead of `TrappingUnwrapRef`.

* Use `drop_in_place`.

* Remove `State::with_mut`.

* Remove the `RefCell` around the `State`.

* Update to wit-bindgen 0.12.

* Update wasi-http to use resources for poll and I/O.

This required making incoming-body and outgoing-body resourrces too, to
work with `push_input_stream_child` and `push_output_stream_child`.

* Re-enable disabled tests, remove logging from the worlds.

* Remove the `poll_list` workarounds that are no longer needed.

* Remove logging from the adapter.

That said, there is no replacement yet, so add a FIXME comment.

* Reenable a test that now passes.

* Remove `.descriptors_mut` and use `with_descriptors_mut` instead.

Replace `.descriptors()` and `.descriptors_mut()` with functions
that take closures, which limits their scope, to prevent them from
invalid aliasing.

* Implement dynamic borrow checking for descriptors.

* Add a cargo-vet audit for wasmtime-wmemcheck.

* Update cargo vet for wit-bindgen 0.12.

* Cut down on duplicate sync/async resource types (#1)

* Allow calling `get-directories` more than once (#2)

For now `Clone` the directories into new descriptor slots as needed.

* Start to lift restriction of stdio only once  (#3)

* Start to lift restriction of stdio only once

This commit adds new `{Stdin,Stdout}Stream` traits which take over the
job of the stdio streams in `WasiCtxBuilder` and `WasiCtx`. These traits
bake in the ability to create a stream at any time to satisfy the API
of `wasi:cli`. The TTY functionality is folded into them as while I was
at it.

The implementation for stdin is relatively trivial since the stdin
implementation already handles multiple streams reading it. Built-in
impls of the `StdinStream` trait are also provided for helper types in
`preview2::pipe` which resulted in the implementation of
`MemoryInputPipe` being updated to support `Clone` where all clones read
the same original data.

* Get tests building

* Un-ignore now-passing test

* Remove unneeded argument from `WasiCtxBuilder::build`

* Fix tests

* Remove some workarounds

Stdio functions can now be called multiple times.

* If `poll_oneoff` fails part-way through, clean up properly.

Fix the `Drop` implementation for pollables to only drop the pollables
that have been successfully added to the list.

This fixes the poll_oneoff_files failure and removes a FIXME.

---------

Co-authored-by: Alex Crichton <alex@alexcrichton.com>
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.

2 participants