stream wit definition eliminates stream-error#6846
Merged
Conversation
pchickey
requested review from
elliottt and
jameysharp
and removed request for
a team and
jameysharp
Contributor
Author
|
|
In WebAssembly/wasi-io#38 we got review feedback to eliminate the stream-error in favor of the empty error in wit `result<a>`. This means we cant use trappable error anymore, and therefore leads to all this other unsightly transformation of the streams trait definition and all its call sites. We'll fix the wasmtime-wit-bindgen macro to support this case better in the future, but rn we gotta stay synchronized with upstream On the upside this showed us that the host stream trait design doesnt differentiate between a runtime and a trapping error, so lets fix that next introduce a StreamRuntimeError, use it in filesystem streams and fix an incorrect error transformation in the filesystem read impl fill in fixmes for distinguishing a stream runtime error delete outdated fixmes: downcast is now guaranteed by child resource tracking
pchickey
force-pushed
the
pch/no_more_stream_error
branch
from
a78e8cd to
c4279b8
Compare
elliottt
approved these changes
Aug 15, 2023
Member
elliottt
left a comment
elliottt
left a comment
There was a problem hiding this comment.
This looks great! I'm looking forward to the day that we can have custom impls for Try, as it would be great to remove the double-nesting of Result :)
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
…runtime I don't think we should trap on any of the errors possible here, reporting them as failures is sufficient
eduardomourar
pushed a commit
to eduardomourar/wasmtime
that referenced
this pull request
Aug 18, 2023
* stream wit definition eliminates stream-error In WebAssembly/wasi-io#38 we got review feedback to eliminate the stream-error in favor of the empty error in wit `result<a>`. This means we cant use trappable error anymore, and therefore leads to all this other unsightly transformation of the streams trait definition and all its call sites. We'll fix the wasmtime-wit-bindgen macro to support this case better in the future, but rn we gotta stay synchronized with upstream On the upside this showed us that the host stream trait design doesnt differentiate between a runtime and a trapping error, so lets fix that next introduce a StreamRuntimeError, use it in filesystem streams and fix an incorrect error transformation in the filesystem read impl fill in fixmes for distinguishing a stream runtime error delete outdated fixmes: downcast is now guaranteed by child resource tracking * dont try to detect rustix io error - just call all read/write errors runtime I don't think we should trap on any of the errors possible here, reporting them as failures is sufficient
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In WebAssembly/wasi-io#38 we got review feedback to eliminate the stream-error in favor of the empty error (wit
result<a>). Although upstream PR 38 is not merged yet, we are downstreaming this interface change now, expecting it will merge upstream soon.The wit change means we cant use wasmtime-wit-bindgen's
trappable_errorfunctionality anymore, so the signature of the streams binding trait its call sites are much less idiomatic than before. We'll fix the wasmtime-wit-bindgen macro to support this case better in the future, but for now we can live with this code being a little ugly, since most users will be using the (much nicer)HostInputStreamandHostOutputStreamtypes.As we made this change, it occurred to us that the host stream trait design doesn't distinguish between an error to be returned via a stream method at runtime, and an error that traps execution. We defined the
StreamRuntimeErrortype for runtime errors, and all other errors trap.