Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

wasmtime-wasi: Fix spurious wake-ups in blocking_ of InputStream and OutputStream #10113

Merged
merged 3 commits into from
Feb 3, 2025

Conversation

Heap-Hop
Copy link
Contributor

@Heap-Hop Heap-Hop commented Jan 25, 2025

issue #9667

The InputStream::ready can be prematurely triggered by io::ErrorKind::WouldBlock. This PR ensure that blocking_ functions return a valid non-empty result.

@pchickey
Copy link
Contributor

Thanks. As you said in the other thread, there's no real way to test this operation from inside because it blocks. I am fine landing this without a test.

@badeend
Copy link
Contributor

badeend commented Jan 25, 2025

The change seems fine indeed.

Aside from blocking_read, its counterpart of the writable side (write_ready) probably needs similar treatment as well. I think it should loop while check_write returns Ok(0).

@Heap-Hop
Copy link
Contributor Author

        loop {
            // This `ready` call may return early due to `io::ErrorKind::WouldBlock`.
            self.ready().await;
            let data = self.read(size)?;
            if data.is_empty() {
                continue;
            }
            return Ok(data);
        }

Can we ensure that there won't be infinite or excessive invalid loops here?

@badeend
Copy link
Contributor

badeend commented Jan 26, 2025

As a fail-safe, you could keep track of the number of iterations and return a trap if it exceeds {some number}. The reason I suggest to trap as opposed to return an empty buffer, is because after a certain threshold it's more likely to be a bug in the InputStream implementation.

@Heap-Hop
Copy link
Contributor Author

Err(e) if e.kind() == std::io::ErrorKind::WouldBlock => {
// As `try_write` indicated that it would have blocked, we'll perform the write
// in the background to allow us to return immediately.
self.background_write(bytes);
return Ok(());
}

I noticed that io::ErrorKind::WouldBlock is already handled here.
However, considering that this trait might still be implemented elsewhere, it indeed makes sense to perform a loop in check_write.

@Heap-Hop
Copy link
Contributor Author

Heap-Hop commented Jan 26, 2025

you could keep track of the number of iterations and return a trap if it exceeds {some number}.

I believe this requires further discussion before implementation.
How to determine the number and how to define the error.

@alexcrichton
Copy link
Member

I think it'd be reasonable to pick a small arbitrary number, for example 10, as the limit of turns of the loop. If something is spuriously readable/writable 10 times in a row that seems indicative of a bug

@Heap-Hop Heap-Hop changed the title wasmtime-wasi: Fix spurious wake-ups in InputStream blocking_read wasmtime-wasi: Fix spurious wake-ups in blocking_ of InputStream and OutputStream Jan 28, 2025
@alexcrichton
Copy link
Member

I believe the test failure can be fixed by checking if the blocking_read size is zero, and in such a case avoiding the loop entirely or basically doing the read once and returning the result instead of turning the loop again.

@alexcrichton alexcrichton added this pull request to the merge queue Feb 3, 2025
Merged via the queue into bytecodealliance:main with commit 473675c Feb 3, 2025
39 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants