Stream: git-wasmtime

Topic: wasmtime / PR #8611 wasi: Handle read(0) with file streams

Wasmtime GitHub notifications bot (May 14 2024 at 00:09):

elliottt requested alexcrichton for a review on PR #8611.

Wasmtime GitHub notifications bot (May 14 2024 at 00:09):

elliottt requested wasmtime-core-reviewers for a review on PR #8611.

Wasmtime GitHub notifications bot (May 14 2024 at 00:09):

elliottt opened PR #8611 from elliottt:trevor/wasi-file-stream-read-zero to bytecodealliance:main:

The read method on streams should support a size argument of zero, to test if the stream is still open. However, the implementation of read for open files was unconditionally treating a read that would return zero bytes as an indication that the stream was closed.

This PR fixes this bug by handling a read(0) as an explicit test to see if the current position is at the end of the file.

<!--
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.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (May 14 2024 at 00:15):

elliottt updated PR #8611.

Wasmtime GitHub notifications bot (May 14 2024 at 14:49):

alexcrichton submitted PR review:

I'm a little hesitant to provide such a strong guarantee that a read of size 0 is effectively a stat on all files. Would it work to instead pass the read size to read_result and don't return Closed if the read size is zero?

Wasmtime GitHub notifications bot (May 14 2024 at 14:58):

elliottt commented on PR #8611:

I don't think that works, as cap_std::fs::File::read_at won't signal any error for a read off the end of 0 bytes. That means that a read(0) will succeed indicating that it ready zero bytes, and a subsequent non-zero read will fail because the file is closed. I agree that this is a bit heavyweight, but I think something like it is necessary to satisfy this part of the streams contract: https://github.com/bytecodealliance/wasmtime/blob/7a91375623051da84e60f513564a144078eeb43c/crates/wasi/wit/deps/io/streams.wit#L51-L53

Wasmtime GitHub notifications bot (May 14 2024 at 15:02):

elliottt edited a comment on PR #8611:

I don't think that works, as cap_std::fs::File::read_at won't signal any error for a read at the end of a file, of 0 bytes. That means that a read(0) will succeed indicating that it read zero bytes, and a subsequent non-zero read will fail because the file is closed. I agree that this is a bit heavyweight, but I think something like it is necessary to satisfy this part of the streams contract: https://github.com/bytecodealliance/wasmtime/blob/7a91375623051da84e60f513564a144078eeb43c/crates/wasi/wit/deps/io/streams.wit#L51-L53

Wasmtime GitHub notifications bot (May 14 2024 at 15:02):

alexcrichton commented on PR #8611:

I think it still satisfies the contract though because the streams interface is supposed to be super general and it's always possible for a generic stream to get disconnected at any time. For example if you do a zero-length read on a socket it may say it's ready only for the next read to say it's closed.

Wasmtime GitHub notifications bot (May 14 2024 at 15:07):

elliottt updated PR #8611.

Wasmtime GitHub notifications bot (May 14 2024 at 15:07):

elliottt commented on PR #8611:

Fair enough. I've pushed a change to that effect.

Wasmtime GitHub notifications bot (May 14 2024 at 15:16):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (May 14 2024 at 15:16):

elliottt updated PR #8611.

Wasmtime GitHub notifications bot (May 14 2024 at 16:45):

elliottt merged PR #8611.

Last updated: Nov 22 2024 at 16:03 UTC