7639: io: fix the hang-forever issue of Mock when the write buf is empty

[martin-augment]

7639: To review by AI

[coderabbitai]

Walkthrough

This pull request updates the tokio-test crate's Mock I/O implementation and test suite. A new dev-dependency (futures-test) is added to Cargo.toml. The I/O module refines action handling by narrowing read/write match conditions to treat specific action types as would-block or no-op scenarios, adds a fast-path optimization for zero-length write operations, and clarifies pattern matching throughout. The test suite expands with four new test cases covering edge cases: reading without sequenced actions, unconsumed error handling, panic behavior on builder cloning with errors, and zero-length write completion.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch pr-7639-2025-11-19-14-13-29

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[claude]

Pull Request Review: Fix hang-forever issue in Mock when write buffer is empty

Overview

This PR addresses issue tokio-rs#7639, fixing a hang-forever bug in tokio_test::io::Mock when writing with an empty buffer. The fix is minimal and focused, which is appropriate given the goal of avoiding breaking changes.

Code Quality & Best Practices

✅ Strengths

1. Minimal, targeted fix: The core fix (line 444 in tokio-test/src/io.rs) correctly handles the edge case of zero-length writes:

   Ok(0) if buf.is_empty() => return Poll::Ready(Ok(0)),

   This follows the Tokio convention of allowing empty writes to succeed immediately without blocking.

2. Pattern matching improvements: The changes improve exhaustiveness and clarity:

   • Lines 238-240: Explicit pattern matching for all non-read actions
   • Line 285: Explicit pattern for read/error actions instead of wildcard
   • Line 498: Explicit pattern in Drop implementation

   These changes make the code more maintainable and prevent silent bugs if new Action variants are added.

3. Code cleanup: Removed obsolete TODO comment (line 287-288 in the original), reducing technical debt.

4. Comprehensive test coverage: The new test should_not_hang_forever_on_zero_length_write directly tests the fixed behavior using low-level poll_write, which is the right approach for this edge case.

⚠️ Concerns & Suggestions

1. Test documentation clarity: The tests document known bugs with TODO comments, which is good practice. However, three tests are explicitly documenting unfixed issues:

   • should_hang_forever_on_read_but_no_sequenced_read_action (line 194)
   • do_not_panic_unconsumed_error (line 215)
   • panic_if_clone_the_build_with_error_action (line 235)

   Recommendation: Consider adding a tracking issue reference in these TODO comments for better traceability.

2. Dependency version: Added futures-test = "0.3.5" to dev-dependencies. Consider using "0.3" instead of pinning to "0.3.5" to allow automatic minor/patch updates, following the pattern of futures-util = "0.3.0" already in the file.

3. Test naming consistency: The new test name should_not_hang_forever_on_zero_length_write uses a different naming convention than existing tests (which don't use "should"). Consider renaming to zero_length_write for consistency.

Potential Bugs & Edge Cases

✅ Correct Handling

1. Empty buffer early return: The fix correctly returns immediately for empty buffers before any other logic, which is the right behavior per AsyncWrite specification.

2. Pattern exhaustiveness: The explicit pattern matching prevents accidental fallthrough and makes future modifications safer.

🤔 Questions to Consider

1. Read side symmetry: Does the read path have similar zero-length handling? Looking at the Inner::read implementation (lines 218-246), it appears to handle empty reads implicitly through the buffer mechanism, but this could be worth verifying for consistency.

2. Test with actions queued: The new test has a pending write action (Builder::new().write(b"write")). It might be worth adding a test case for zero-length write with NO actions queued to ensure that path also works correctly (though it likely does given the early return).

Performance Considerations

✅ Performance is Good

1. Early return optimization: The fix adds an early return for empty buffers (line 444), which is optimal - it avoids unnecessary processing for a no-op.

2. No additional allocations: The changes don't introduce any new allocations or expensive operations.

3. Pattern matching: The explicit pattern matching is compiled to efficient match statements by the Rust compiler, so there's no performance penalty.

Security Concerns

✅ No Security Issues

1. Buffer handling: No unsafe code or buffer overruns introduced.
2. Error handling: Error paths remain properly handled.
3. Arc unwrap safety: The comment on line 235 about Arc::try_unwrap panic is documented, and this is a testing library where panics on misuse are acceptable.

Test Coverage

✅ Excellent Test Coverage

1. Direct test for the fix: should_not_hang_forever_on_zero_length_write (line 249-259) directly tests the fixed behavior using poll_write with an empty buffer.

2. Comprehensive edge case documentation: The three tests documenting known limitations are valuable regression guards:

   • They document current behavior
   • They'll catch if behavior changes (intentionally or not)
   • They include clear TODOs for future fixes

3. Test structure: Uses appropriate testing utilities (pin_mut!, noop_context(), panic_waker(), assert_pending!) from futures-test.

📝 Suggestions

1. Consider adding a test for zero-length write with no pending actions to ensure that edge case is also covered.

2. The test at line 249 could benefit from a comment explaining why it's important (similar to the detailed comments on the other new tests).

Overall Assessment

Summary

This is a well-crafted, minimal fix for a real bug. The implementation is correct, the test coverage is good, and the approach of avoiding breaking changes is appropriate for a minor fix.

Recommendations

Must address:

• None (the PR is functionally correct)

Should consider:

• Update futures-test version to "0.3" instead of "0.3.5" for consistency
• Consider renaming test to match existing naming conventions
• Add issue references to TODO comments in tests

Nice to have:

• Add comment explaining the importance of the zero-length write test
• Consider a test case for zero-length write with no pending actions

Verdict

✅ Approve with minor suggestions

The core fix is sound and addresses the reported issue effectively. The suggested improvements are minor and don't block merging.

---

Reviewed with attention to code quality, correctness, performance, security, and test coverage per repository standards.

[augmentcode]

Review completed. 1 suggestions posted.

Comment augment review to trigger a new review at any time.

[augmentcode]

Consider short‑circuiting on an empty buf before calling self.inner.write(buf) so zero‑length writes return immediately even when a Wait action is pending; currently, a zero‑length write during Wait will still be delayed by the sleep. This aligns with typical AsyncWrite semantics where writing 0 bytes should not block.

_{🤖 Was this useful? React with 👍 or 👎}

[martin-augment]

value:incorrect-but-reasonable; category:bug; feedback:The Augment AI reviewer is not correct! There is an extra logic in the Mock that deals with timers and other extra actions which should be handled before doing the "write" operation, so there is no much benefit of moving the check earlier.

[coderabbitai]

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a73aa28 and 25070b1.

📒 Files selected for processing (3)

• tokio-test/Cargo.toml (1 hunks)
• tokio-test/src/io.rs (4 hunks)
• tokio-test/tests/io.rs (2 hunks)

🧰 Additional context used

🧠 Learnings (5)

📓 Common learnings

Learnt from: martin-augment
Repo: martin-augment/tokio PR: 13
File: tokio-util/src/io/simplex.rs:184-209
Timestamp: 2025-11-05T14:24:03.551Z
Learning: In AsyncWrite implementations, zero-length writes (when buf.is_empty()) should return Poll::Ready(Ok(0)) immediately without blocking or registering wakers. Only non-empty buffers that cannot be written due to backpressure should return Poll::Pending. This prevents indefinite hangs when empty slices are written.

📚 Learning: 2025-11-05T14:24:03.551Z

Learnt from: martin-augment
Repo: martin-augment/tokio PR: 13
File: tokio-util/src/io/simplex.rs:184-209
Timestamp: 2025-11-05T14:24:03.551Z
Learning: In AsyncWrite implementations, zero-length writes (when buf.is_empty()) should return Poll::Ready(Ok(0)) immediately without blocking or registering wakers. Only non-empty buffers that cannot be written due to backpressure should return Poll::Pending. This prevents indefinite hangs when empty slices are written.

Applied to files:

• tokio-test/tests/io.rs
• tokio-test/src/io.rs

📚 Learning: 2025-10-28T15:30:50.368Z

Learnt from: martin-augment
Repo: martin-augment/tokio PR: 9
File: tokio/src/fs/file.rs:824-855
Timestamp: 2025-10-28T15:30:50.368Z
Learning: In io-uring write implementations for tokio::fs::File, writes must loop until the buffer is fully drained. A single Op::write_at call may perform a partial write. The code must: (1) loop calling Op::write_at until buf.is_empty(), (2) return io::ErrorKind::WriteZero when 0 bytes are written, (3) only return Operation::Write(Ok(())) after the buffer is completely empty. This applies to both poll_write and poll_write_vectored implementations.

Applied to files:

• tokio-test/tests/io.rs
• tokio-test/src/io.rs

📚 Learning: 2025-10-28T15:30:54.516Z

Learnt from: martin-augment
Repo: martin-augment/tokio PR: 9
File: tokio/src/fs/file.rs:970-981
Timestamp: 2025-10-28T15:30:54.516Z
Learning: In tokio fs file io-uring write operations (both vectored and non-vectored), the write must loop until the buffer is fully drained. Operation::Write(Ok()) should only be returned when buf.is_empty() is true. If 0 bytes are written, return Operation::Write(Err(io::ErrorKind::WriteZero.into())). Otherwise, continue executing new writes until the buffer is empty or an error occurs.

Applied to files:

• tokio-test/src/io.rs

📚 Learning: 2025-10-30T14:46:01.443Z

Learnt from: martin-augment
Repo: martin-augment/tokio PR: 10
File: tokio-util/src/io/mod.rs:17-17
Timestamp: 2025-10-30T14:46:01.443Z
Learning: In tokio-util, the entire `io` module at `tokio-util/src/lib.rs` is already gated with `cfg_io!` macro, which applies `#[cfg(feature = "io")]` to all contents. Submodules declared within `tokio-util/src/io/mod.rs` inherit this feature gate and don't need additional `cfg_io!` wrapping unless they have specific requirements. The `cfg_io_util!` macro in the same file is for items that specifically require the `io-util` feature, not the general `io` feature.

Applied to files:

• tokio-test/Cargo.toml

🧬 Code graph analysis (1)

tokio-test/tests/io.rs (1)

tokio-test/src/io.rs (2)

• new (80-82)
• new (195-212)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: claude-review

🔇 Additional comments (9)

tokio-test/src/io.rs (4)

238-240: LGTM! Pattern matching refinement improves clarity.

The explicit enumeration of Write, WriteError, and Wait as would-block conditions for reads makes the logic clearer than the previous catch-all pattern. This ensures that only write-related or wait actions block read operations, which is the correct behavior.

---

285-285: LGTM! Explicit pattern makes the no-op behavior clear.

Making Read and ReadError actions explicit no-ops during write operations improves code clarity. The write loop correctly skips over read-related actions to find the next write action.

---

444-444: Excellent fix! Zero-length writes now return immediately.

This fast-path correctly prevents indefinite hangs when empty buffers are written. The check Ok(0) if buf.is_empty() ensures that zero-length writes return Poll::Ready(Ok(0)) immediately without blocking or registering wakers, which is the expected behavior for AsyncWrite implementations.

Based on learnings

---

498-498: LGTM! Explicit enumeration in Drop is clearer.

The explicit pattern matching for ReadError, WriteError, and Wait actions makes it clear that these don't require assertions during drop. Only Read and Write actions need to verify that their data is fully consumed.

tokio-test/tests/io.rs (5)

3-11: LGTM! Imports are necessary for the new tests.

All the new imports are appropriately used in the added test cases for manual polling and assertion utilities.

---

180-203: Good documentation of known bug.

This test correctly documents the hang-forever behavior when reading with no sequenced read action. The use of panic_waker ensures the test will catch if this bug is fixed (intentionally or unintentionally). The extensive comments explain the rationale for not fixing this yet.

---

205-219: Good regression test for documented behavior.

This test correctly captures the current behavior where unconsumed error actions don't cause panics. The TODO comment appropriately marks this as something to address in a future major release.

---

221-246: Well-documented test for builder cloning issue.

This test correctly demonstrates the panic that occurs when cloning a builder with error actions due to the Arc::try_unwrap call failing when multiple references exist. The detailed comments explain why this isn't fixed yet (would require breaking changes).

---

248-259: Excellent test for the zero-length write fix!

This test directly verifies that zero-length writes return Poll::Ready(Ok(0)) immediately without hanging. The use of manual polling with poll_write is the correct approach to test this behavior. The follow-up write_all appropriately drains the remaining action to prevent a panic during drop.

Based on learnings

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

Verify that this is the latest version of futures-test.

The version 0.3.5 appears to be from 2020. Ensure you're using the latest available version for security and compatibility with other dependencies.

---

🏁 Script executed:

#!/bin/bash
# Check for the latest version of futures-test on crates.io
curl -s https://crates.io/api/v1/crates/futures-test | jq -r '.crate.max_stable_version'

Length of output: 6

---

Update futures-test dependency to the latest version.

The version 0.3.5 is significantly outdated. Update to 0.3.31 (the latest stable version) for security, bug fixes, and compatibility improvements.

🤖 Prompt for AI Agents

In tokio-test/Cargo.toml around line 27, the futures-test dependency is pinned
to the outdated version "0.3.5"; update the dependency line to use the newer
stable version "0.3.31" (i.e., replace 0.3.5 with 0.3.31) and run cargo update
or cargo build to refresh lockfile and verify compatibility.

[martin-augment]

value:useful; category:bug; feedback:The CodeRabbit AI reviewer is correct that the version of the futures-test dependency is very old and should be updated to a newer one. It is used only in the tests for it does not bring any security related concerns.

[martin-augment]

2. Dependency version: Added futures-test = "0.3.5" to dev-dependencies. Consider using "0.3" instead of pinning to "0.3.5" to allow automatic minor/patch updates, following the pattern of futures-util = "0.3.0" already in the file.

value:good-to-have; category:bug; feedback:The Claude AI reviewer is correct that by using a version ending on ".0" cargo will use the latest available, e.g. "0.3.0" will resolve to the latest available 0.3.x