-
Notifications
You must be signed in to change notification settings - Fork 23
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
Add stream-status to stream write return values, and expand documentation #38
Conversation
…tion Documentation focuses on how the non-blocking read and write functions respond when not ready for reading/writing, and how that maps to pollable. stream-error is now a record with a dummy member, to follow the new component model excluding zero-sized types.
/// When writing, this indicates that the stream will no longer be read. | ||
/// Further writes are still permitted. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you briefly explain why writing to a closed stream is now explicitly permitted? This isn't obvious to me. What happens to the written data?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The write end of a stream is a mechanism for the reader to tell the writer that it won't be reading any more input. The writer is still welcome to send input, but it will be dropped because no reader exists.
In many existing codebases when the reader doesn't care to read any more input, it may simply stop reading and won't close the stream. So, there is never a guarantee that something written ends up being read - we are exposing this as a mechanism to allow optimizing the writer.
libc might choose to translate this into EPIPE when appropriate, or even emulate a SIGPIPE exit when appropriate (e.g. on stdin). (I'm not a libc expert but those are Dan's suggestions.)
This feels to me like a weaker condition than an error, so I think its a case where we can be liberal in what we accept - its easy enough to let the writer keep writing and drop its input, if it doesn't honor the flag indicating the stream has closed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All clear to me now. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've been having some issues with the write backpressure design and I've come around to the opposite view on this topic... I'm working on the design and implementation of a different approach now which I intend will replace this PR.
Co-authored-by: Dave Bakker <[email protected]>
c4553d6
to
c22110e
Compare
/// Implementations may trap if this `output-stream` is dropped while | ||
/// child `pollable` resources are still alive. | ||
/// After this `output-stream` is dropped, implementations may report any | ||
/// corresponding `input-stream` has `stream-state.closed`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this constraint specific to streams and their pollables, or do you see this as a general pattern where there is a parent/child relationship between resources?
For example, do you foresee a similar constraint on the Drop method of file descriptors, stating it may trap if a child directory-entry-stream
, input-stream
or output-stream
is still open?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The general pattern where one resource might be the child of another resource is something we aim to make possible to describe in the component model type system, but we aren't ready to do so in the preview 2 time frame.
We didn't find that this child constraint was appropriate for the streams of files. The way I drew the line was, "is this resource useful after its parent has been dropped?". With pollables of streams, readiness is telling you something particular about the stream itself: it doesn't mean anything without a stream to try reading from/writing to. On the other hand, with files, a descriptor is a "full" view of all the capabilities possible, but if you just care about reading out the contents of a directory or file, its reasonable to just "narrow" your view to the particular stream you care about, and drop the descriptor because you don't care about any other operations.
So: interfaces can decide that a resource is a child of another resource when appropriate, and for now they have to communicate that with docs, and hopefully one day with types. Not all resources will be the child of another resource.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah nice. Hadn't thought about it that way.
I completely agree.
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
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
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
* 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
* 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
Documentation focuses on how the non-blocking read and write functions respond when not ready for reading/writing, and how that maps to pollable.
stream-error is now a record with a dummy member, to follow the new component model excluding zero-sized types.stream-error has been eliminated, since we can't determine any error value to provide there. All fallible funcs simply return
result<ok>
now, which can still represent an error but without any error type as the payload.For more context on these changes: I designed and implemented them as part of bytecodealliance/wasmtime#6556