sunfishcode opened PR #7029 from sunfishcode:resources
to bytecodealliance:main
:
This imports the new APIs posted in WebAssembly/wasi-io#46.
<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
sunfishcode updated PR #7029.
sunfishcode updated PR #7029.
sunfishcode updated PR #7029.
sunfishcode updated PR #7029.
sunfishcode updated PR #7029.
sunfishcode updated PR #7029.
sunfishcode updated PR #7029.
alexcrichton submitted PR review.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
This is I believe going to return an error at runtime because borrows can't be returned, they can only be taken.
Two ways I can think of to handle this are:
- Don't store preopens as
Resource<Descriptor>
. Instead insert them into the table every timeget-directories
is called.- Alternatively on
HostDescriptor::drop
don't actually delete preopens from the table. This will "leak" them in the table but that's ok since it's a static set that doesn't grow.
alexcrichton created PR review comment:
I think this can all be skipped in favor fo
closure_future.await
?
poll-list
is a bit funky since there's more than one future, but here there's just one so I think it should be ok to do a normal.await
alexcrichton created PR review comment:
Bah this is unfortunate (as are all the other instances of this in this file). If I understand this correctly, this is because the bindings generated. have two distinct
Descriptor
types meaning that they're not compatible?If so I can try to add a feature to
bindgen!
so resources can be specified as opposed to always-generated
sunfishcode updated PR #7029.
sunfishcode submitted PR review.
sunfishcode created PR review comment:
Yes, exactly. All the
sync
adapter code has to jump through these hoops for that reason.
sunfishcode updated PR #7029.
sunfishcode submitted PR review.
sunfishcode created PR review comment:
Done.
sunfishcode submitted PR review.
sunfishcode created PR review comment:
I made it insert them into the table every tine
get-directories
is called.
sunfishcode updated PR #7029.
sunfishcode updated PR #7029.
sunfishcode updated PR #7029.
alexcrichton submitted PR review.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
Could this perhaps use
OnceCell
?
alexcrichton created PR review comment:
Would you be ok splitting this to a separate PR?
alexcrichton created PR review comment:
Could
.as_ref().trapping_unwrap()
be used instead? (or was it common enough you felt this was nicer?)
alexcrichton created PR review comment:
While this works and there's nothing wrong with it, if you're curious this is what
drop_in_place
is for
alexcrichton created PR review comment:
If possible we'll definitely want to avoid bringing this back. Was this required though because of the bindings generated?
alexcrichton created PR review comment:
If possible I'd prefer to avoid the usage of
UnsafeCell
here since otherwise it's very easy to trip into corruption by accident. Ifwith_mut
stays gone though then theRefCell
layer I think can be removed entirely since it's always returning&T
sunfishcode updated PR #7029.
sunfishcode updated PR #7029.
sunfishcode updated PR #7029.
sunfishcode submitted PR review.
sunfishcode created PR review comment:
I tried that, but
OnceCell
pulls in Rust error handling which breaks the adapter build.
sunfishcode submitted PR review.
sunfishcode created PR review comment:
Oh, good idea.
sunfishcode updated PR #7029.
pchickey submitted PR review.
pchickey created PR review comment:
I had gotten rid of State::with_mut in https://github.com/bytecodealliance/wasmtime/pull/6973, is there a reason this PR brought it back that I haven't spotted, or is this just a git history artifact?
sunfishcode updated PR #7029.
sunfishcode submitted PR review.
sunfishcode created PR review comment:
I've now removed the
with_mut
and this outerUnsafeCell
.I think the scariest part now is the
fn descriptors_mut(&self) -> &mut Descriptors
, because I couldn't getRefCell
/RefMut
to work now thatget_input_stream
needs to return a&Descriptor
instead of au32
that it can just copy out. I'll poke around at this to see if I can figure out a better option.
sunfishcode submitted PR review.
sunfishcode created PR review comment:
get_input_stream()
now needs to return a&Descriptor
instead of aDescriptor
akau32
. That lifetime constraint appears to make it incompatible with holding the descriptor table in aRefCell
. ThisState::with_mut
and the currentdescriptors(&self) -> &mut Descriptors
code are me searching for a way to describe what this code is doing. I don't think I've found the right answer yet.
sunfishcode submitted PR review.
sunfishcode created PR review comment:
Yeah, I can do that. It's convenient for now, because with stderr being a host stream that gets buffered, it's extra important for debugging to have some kind of debug facilty that can dump text directly to the host stderr. But when this PR is closer to landing I can split it out.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
Hm that seems like an issue with our stderr design then as well, that should definitely be suitable for debugging. We don't have any writes though that we don't push out via tokio though right? Is the issue that it's a slightly deferred write rather than immediate?
sunfishcode updated PR #7029.
sunfishcode updated PR #7029.
sunfishcode updated PR #7029.
sunfishcode updated PR #7029.
sunfishcode updated PR #7029.
sunfishcode submitted PR review.
sunfishcode created PR review comment:
I've now fully removed this.
sunfishcode updated PR #7029.
sunfishcode submitted PR review.
sunfishcode created PR review comment:
I've now removed the logging code.
sunfishcode submitted PR review.
sunfishcode created PR review comment:
I've now removed
State::with_mut
.
sunfishcode updated PR #7029.
sunfishcode updated PR #7029.
sunfishcode updated PR #7029.
sunfishcode updated PR #7029.
alexcrichton submitted PR review:
Ok I've re-reviewed everything here and it all looks good-to-go to me. There's some follow-up cleanups I think but nothing that needs to block this PR :+1:
sunfishcode has marked PR #7029 as ready for review.
sunfishcode requested pchickey for a review on PR #7029.
sunfishcode requested wasmtime-core-reviewers for a review on PR #7029.
sunfishcode requested wasmtime-default-reviewers for a review on PR #7029.
pchickey submitted PR review:
Looks good. Some minor stuff I stumbled on, and we'd like to land https://github.com/bytecodealliance/wasmtime/pull/7091 first this afternoon, which will create some minor conflicts.
pchickey submitted PR review:
Looks good. Some minor stuff I stumbled on, and we'd like to land https://github.com/bytecodealliance/wasmtime/pull/7091 first this afternoon, which will create some minor conflicts.
pchickey created PR review comment:
Is this still broken? Just want to note it for a follow-up so we don't forget to go back and fix it.
pchickey created PR review comment:
We can revert the line-oriented changes and move this back to stderr, right?
pchickey created PR review comment:
We can run
get_stderr().blocking_write_and_flush()
here now, correct?
sunfishcode updated PR #7029.
sunfishcode updated PR #7029.
sunfishcode created PR review comment:
Yes, I've now reverted them.
sunfishcode submitted PR review.
sunfishcode submitted PR review.
sunfishcode created PR review comment:
Earlier I had assumed it was failing due to the issue with calling
get_stdout
multiple times, but it turns out to have been a bug in the poll_oneoff adapter. I've now pushed a patch fixing it.
sunfishcode updated PR #7029.
sunfishcode submitted PR review.
sunfishcode created PR review comment:
Yep, this is now done.
alexcrichton submitted PR review.
sunfishcode updated PR #7029.
sunfishcode updated PR #7029.
sunfishcode updated PR #7029.
sunfishcode updated PR #7029.
sunfishcode merged PR #7029.
Last updated: Dec 23 2024 at 12:05 UTC