From d13769269ba99111003a51319462b60262ef14be Mon Sep 17 00:00:00 2001 From: Qi Date: Sun, 21 Sep 2025 19:26:07 +0800 Subject: [PATCH 1/2] io: fix the hang-forever issue of `Mock` when the write buf is empty Signed-off-by: ADD-SP --- tokio-test/Cargo.toml | 1 + tokio-test/src/io.rs | 11 +++--- tokio-test/tests/io.rs | 87 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 94 insertions(+), 5 deletions(-) diff --git a/tokio-test/Cargo.toml b/tokio-test/Cargo.toml index 5601a112205..23eb1b06281 100644 --- a/tokio-test/Cargo.toml +++ b/tokio-test/Cargo.toml @@ -24,6 +24,7 @@ futures-core = "0.3.0" [dev-dependencies] tokio = { version = "1.2.0", path = "../tokio", features = ["full"] } futures-util = "0.3.0" +futures-test = "0.3.5" [package.metadata.docs.rs] all-features = true diff --git a/tokio-test/src/io.rs b/tokio-test/src/io.rs index c3317092775..bcacc592f14 100644 --- a/tokio-test/src/io.rs +++ b/tokio-test/src/io.rs @@ -235,7 +235,9 @@ impl Inner { let err = Arc::try_unwrap(err).expect("There are no other references."); Err(err) } - Some(_) => { + Some(&mut Action::Write(_)) + | Some(&mut Action::WriteError(_)) + | Some(&mut Action::Wait(_)) => { // Either waiting or expecting a write Err(io::ErrorKind::WouldBlock.into()) } @@ -280,10 +282,8 @@ impl Inner { Action::Wait(..) | Action::WriteError(..) => { break; } - _ => {} + Action::Read(_) | Action::ReadError(_) => (), } - - // TODO: remove write } Ok(ret) @@ -441,6 +441,7 @@ impl AsyncWrite for Mock { panic!("unexpected WouldBlock {}", self.pmsg()); } } + Ok(0) if buf.is_empty() => return Poll::Ready(Ok(0)), Ok(0) => { // TODO: Is this correct? if !self.inner.actions.is_empty() { @@ -494,7 +495,7 @@ impl Drop for Mock { "There is still data left to write. {}", self.pmsg() ), - _ => (), + Action::ReadError(_) | Action::WriteError(_) | Action::Wait(_) => (), }); } } diff --git a/tokio-test/tests/io.rs b/tokio-test/tests/io.rs index effac9a51fa..b308d6e0e71 100644 --- a/tokio-test/tests/io.rs +++ b/tokio-test/tests/io.rs @@ -1,8 +1,14 @@ #![warn(rust_2018_idioms)] +use futures_test::task::{noop_context, panic_waker}; +use futures_util::pin_mut; +use std::future::Future; use std::io; +use std::task::{Context, Poll}; +use tokio::io::AsyncWrite; use tokio::io::{AsyncReadExt, AsyncWriteExt}; use tokio::time::{Duration, Instant}; +use tokio_test::assert_pending; use tokio_test::io::Builder; #[tokio::test] @@ -170,3 +176,84 @@ async fn multiple_wait() { start.elapsed().as_millis() ); } + +// No matter which usecase, it doesn't make sense for a read +// hang forever. However, currently, if there is no sequenced read +// action, it will hang forever. +// +// Since we want be aware of the fixing of this bug, +// no matter intentionally or unintentionally, +// we add this test to catch the behavior change. +// +// It looks like fixing it is not hard, but not sure the downstream +// impact, which might be a breaking change due to the +// `Mock::inner::read_wait` field, so we keep it as is for now. +// +// TODO: fix this bug +#[test] +fn should_hang_forever_on_read_but_no_sequenced_read_action() { + let mut mock = Builder::new() + .write_error(io::Error::new(io::ErrorKind::Other, "cruel")) + .build(); + + let mut buf = [0; 1]; + let read_exact_fut = mock.read(&mut buf); + pin_mut!(read_exact_fut); + assert_pending!(read_exact_fut.poll(&mut Context::from_waker(&panic_waker()))); +} + +// The `Mock` is expected to always panic if there is an unconsumed error action, +// rather than silently ignoring it. However, +// currently it only panics on unconsumed read/write actions, +// not on error actions. Fixing this requires a breaking change. +// +// This test verifies that it does not panic yet, +// to prevent accidentally introducing the breaking change prematurely. +// +// TODO: fix this bug in the next major release +#[test] +fn do_not_panic_unconsumed_error() { + let _mock = Builder::new() + .read_error(io::Error::new(io::ErrorKind::Other, "cruel")) + .build(); +} + +// The `Mock` must never panic, even if cloned multiple times. +// However, at present, cloning the builder under certain +// conditions causes a panic. +// +// Fixing this would require making `Mock` non-`Clone`, +// which is a breaking change. +// +// Since we want be aware of the fixing of this bug, +// no matter intentionally or unintentionally, +// we add this test to catch the behavior change. +// +// TODO: fix this bug in the next major release +#[tokio::test] +#[should_panic = "There are no other references.: Custom { kind: Other, error: \"cruel\" }"] +async fn panic_if_clone_the_build_with_error_action() { + let mut builder = Builder::new(); + builder.write_error(io::Error::new(io::ErrorKind::Other, "cruel")); + let mut builder2 = builder.clone(); + + let mut mock = builder.build(); + let _mock2 = builder2.build(); + + // this write_all will panic due to unwrapping the error from `Arc` + mock.write_all(b"hello").await.unwrap(); + unreachable!(); +} + +#[tokio::test] +async fn should_not_hang_forever_on_zero_length_write() { + let mock = Builder::new().write(b"write").build(); + pin_mut!(mock); + match mock.as_mut().poll_write(&mut noop_context(), &[0u8; 0]) { + // drain the remaining write action to avoid panic at drop of the `mock` + Poll::Ready(Ok(0)) => mock.write_all(b"write").await.unwrap(), + Poll::Ready(Ok(n)) => panic!("expected to write 0 bytes, wrote {n} bytes instead"), + Poll::Ready(Err(e)) => panic!("expected to write 0 bytes, got error {e} instead"), + Poll::Pending => panic!("expected to write 0 bytes immediately, but pending instead"), + } +} From 35e15da2d91d19a6ae0420052cc8a1db6c26eb4e Mon Sep 17 00:00:00 2001 From: Qi Date: Thu, 16 Oct 2025 00:00:40 +0800 Subject: [PATCH 2/2] fix two typos Co-authored-by: Martin Grigorov --- tokio-test/tests/io.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tokio-test/tests/io.rs b/tokio-test/tests/io.rs index b308d6e0e71..84dd54c6907 100644 --- a/tokio-test/tests/io.rs +++ b/tokio-test/tests/io.rs @@ -177,7 +177,7 @@ async fn multiple_wait() { ); } -// No matter which usecase, it doesn't make sense for a read +// No matter which usecase, it doesn't make sense for a read to // hang forever. However, currently, if there is no sequenced read // action, it will hang forever. // @@ -225,7 +225,7 @@ fn do_not_panic_unconsumed_error() { // Fixing this would require making `Mock` non-`Clone`, // which is a breaking change. // -// Since we want be aware of the fixing of this bug, +// Since we want to be aware of the fixing of this bug, // no matter intentionally or unintentionally, // we add this test to catch the behavior change. //