Skip to content
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

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

4 participants