wasmtime / PR #10113 wasmtime-wasi: Fix spurious wake-ups... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #10113 wasmtime-wasi: Fix spurious wake-ups...

Wasmtime GitHub notifications bot (Jan 25 2025 at 07:44):

Heap-Hop opened PR #10113 from Heap-Hop:fix_stream_blocking to bytecodealliance:main:

issue #9667

The InputStream::ready can be prematurely triggered by io::ErrorKind::WouldBlock. This PR fixes blocking_read to return the correct data.

Wasmtime GitHub notifications bot (Jan 25 2025 at 07:44):

Heap-Hop requested pchickey for a review on PR #10113.

Wasmtime GitHub notifications bot (Jan 25 2025 at 07:44):

Heap-Hop requested wasmtime-core-reviewers for a review on PR #10113.

Wasmtime GitHub notifications bot (Jan 25 2025 at 19:09):

pchickey submitted PR review.

Wasmtime GitHub notifications bot (Jan 25 2025 at 19:10):

pchickey commented on PR #10113:

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.

Wasmtime GitHub notifications bot (Jan 25 2025 at 22:01):

badeend commented on PR #10113:

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

Wasmtime GitHub notifications bot (Jan 26 2025 at 07:15):

Heap-Hop commented on PR #10113:

        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?

Wasmtime GitHub notifications bot (Jan 26 2025 at 07:37):

badeend commented on PR #10113:

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.

Wasmtime GitHub notifications bot (Jan 26 2025 at 08:20):

Heap-Hop commented on PR #10113:

https://github.com/bytecodealliance/wasmtime/blob/5dfccc078bb3dcad152d37013dde8067f197e6e7/crates/wasi/src/tcp.rs#L844-L850

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.

Wasmtime GitHub notifications bot (Jan 26 2025 at 08:29):

Heap-Hop commented on PR #10113:

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 maximum value and how to define the error.

Wasmtime GitHub notifications bot (Jan 26 2025 at 08:31):

Heap-Hop edited a comment on PR #10113:

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.

Wasmtime GitHub notifications bot (Jan 27 2025 at 15:43):

alexcrichton commented on PR #10113:

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

Wasmtime GitHub notifications bot (Jan 28 2025 at 05:30):

Heap-Hop updated PR #10113.

Wasmtime GitHub notifications bot (Jan 28 2025 at 05:32):

Heap-Hop edited PR #10113.

Wasmtime GitHub notifications bot (Jan 28 2025 at 05:33):

Heap-Hop edited PR #10113:

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.

Wasmtime GitHub notifications bot (Jan 31 2025 at 17:01):

alexcrichton commented on PR #10113:

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.

Wasmtime GitHub notifications bot (Feb 03 2025 at 07:56):

Heap-Hop updated PR #10113.

Wasmtime GitHub notifications bot (Feb 03 2025 at 12:18):

alexcrichton merged PR #10113.

Last updated: Feb 28 2025 at 02:27 UTC