wasi:[email protected]: Add tests for read/write/append #135
Conversation
wingo
force-pushed
the
filesystem-io
branch
2 times, most recently
from
b04d3fe to
7268371
Compare
wingo
changed the title
wingo
force-pushed
the
filesystem-io
branch
from
7268371 to
1f37650
Compare
There was a problem hiding this comment.
LGTM but I want to tag in @alexcrichton for this one because I'm not confident about subtleties of stream handling here.
| (result, data) = rx.read(data).await; | ||
| } | ||
| StreamResult::Dropped => { | ||
| assert_eq!(data.len(), bytes_read); |
There was a problem hiding this comment.
This assert may want to get deferred to the Ok return value of this function because a short-read might happen due to an error and the error may want to get returned instead of panicking here
There was a problem hiding this comment.
I am trying to understand the subtleties but I am a bit lost. If I understand you correctly, Dropped can be accompanied by data, is that right? If so we should add an n to Dropped: bytecodealliance/wit-bindgen#1396. If not I think this assertion will never cause a failed pread, because bytes_read and data.len() are both incremented as part of Complete.
There was a problem hiding this comment.
You're correct that Dropped can be accompanied by data, yeah. The lack of n here is definitely an oversight and the assumption was that you'd look at the length of the buffer before/after to figure out the size (but then that's inconsistent with n on StreamResult::Complete, so n should be on Dropped and Cancelled as well for consistency)
I think I actually confused myself about this assertion and what it originally meant. I thought it was somehow taking the size parameter to the function into account but it isn't. Otherwise though you're also correct that this can panic in the case that Dropped communicates some data, which wasn't actually what I was thinking about but is true nonetheless!
| let (mut result, mut buf) = tx.write(data.to_vec()).await; | ||
| loop { | ||
| match result { | ||
| StreamResult::Complete(n) => { | ||
| assert!(n <= len - written); | ||
| written += n; | ||
| assert_eq!(buf.remaining(), len - written); | ||
| if buf.remaining() != 0 { | ||
| (result, buf) = tx.write_buf(buf).await; | ||
| } else { | ||
| break; | ||
| } | ||
| } | ||
| StreamResult::Dropped => { | ||
| panic!("receiver dropped the stream?"); | ||
| } | ||
| StreamResult::Cancelled => { | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| assert_eq!(buf.remaining(), len - written); | ||
| drop(tx); |
There was a problem hiding this comment.
This might be replacable with StreamWriter::write_all perhaps? (plus an assert that the result of write_all is empty)
There was a problem hiding this comment.
FWIW... I have been avoiding write_all because I wanted to understand the stream mechanics. But, this is a WASI test suite, not a component model test suite, and so perhaps write_all is the right thing.
There was a problem hiding this comment.
True! Either way seems reasonable to me
|
I have granted @alexcrichton write privs so that his review of this makes it green. I think this is ready to go? |
3 participants
No description provided.