Skip to content

AsyncInputStream: if input-stream.read returns empty, try again.#60

Merged
sunfishcode merged 1 commit intomainfrom
pch/input_stream_ready_retry
Jan 28, 2025
Merged

AsyncInputStream: if input-stream.read returns empty, try again.#60
sunfishcode merged 1 commit intomainfrom
pch/input_stream_ready_retry

Conversation

@pchickey
Copy link
Contributor

@pchickey pchickey commented Jan 25, 2025

For details: bytecodealliance/wasmtime#9667 (comment)

We can release this as 0.5.1.

@sunfishcode
Copy link
Member

Another option, instead of looping, would be to return Err(std::io::Error::from(std::io::ErrorKind::Interrupted)). The Interrupted error code is the documented value for Rust read functions to return to indicate that zero bytes were received. Rust code doing reads should already be checking for Interrupted and using a loop (eg. like this). This approach would avoid having a redundant loop here, and would give control back to user code sooner in case their loop has other things to do while they're waiting for actual data to arrive.

@pchickey
Copy link
Contributor Author

That would be the correct behavior for synchronous code. However, in this async context, the ready().await in this loop is effectively giving control back to the user's code to do other things while it waits, including the possibility of dropping the read Future to cancel the operation. So, in async, I think its this library's job to ensure the user never sees an ErrorKind::Interrupted.

Copy link
Member

@sunfishcode sunfishcode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Investigating more, I found that it's common for async Read traits to retry on ErrorKind::Interrupted, such as futures_io::AsyncRead does. So that sounds like the thing to do here too.

@sunfishcode sunfishcode merged commit 211a8c2 into main Jan 28, 2025
4 checks passed
@sunfishcode sunfishcode deleted the pch/input_stream_ready_retry branch January 28, 2025 06:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants