From 2ab60c20a3b5609b2868670c8c5081aeb58322ce Mon Sep 17 00:00:00 2001 From: "Stainsby, Hayden" Date: Mon, 27 Jun 2022 17:37:31 +0200 Subject: [PATCH 1/3] io: add track_caller to public APIs Functions that may panic can be annotated with #[track_caller] so that in the event of a panic, the function where the user called the panicking function is shown instead of the file and line within Tokio source. This change adds #[track_caller] to all the non-unstable public io APIs in the main tokio crate where the documentation describes how the function may panic due to incorrect context or inputs. Additionally, the documentation for `AsyncFd` was updated to indicate that the functions `new` and `with_intent` can panic. Tests are included to cover each potentially panicking function. The logic to test the location of a panic (which is a little complex), has been moved to a test support module. Refs: #4413 --- tokio/src/io/async_fd.rs | 16 +++- tokio/src/io/driver/mod.rs | 1 + tokio/src/io/read_buf.rs | 4 + tokio/src/io/split.rs | 1 + tokio/tests/io_panic.rs | 168 +++++++++++++++++++++++++++++++++++ tokio/tests/rt_panic.rs | 35 +------- tokio/tests/support/panic.rs | 34 +++++++ tokio/tests/time_panic.rs | 35 +------- 8 files changed, 228 insertions(+), 66 deletions(-) create mode 100644 tokio/tests/io_panic.rs create mode 100644 tokio/tests/support/panic.rs diff --git a/tokio/src/io/async_fd.rs b/tokio/src/io/async_fd.rs index 93f9cb458a7..7b3953d5ca2 100644 --- a/tokio/src/io/async_fd.rs +++ b/tokio/src/io/async_fd.rs @@ -167,12 +167,18 @@ pub struct AsyncFdReadyMutGuard<'a, T: AsRawFd> { const ALL_INTEREST: Interest = Interest::READABLE.add(Interest::WRITABLE); impl AsyncFd { - #[inline] /// Creates an AsyncFd backed by (and taking ownership of) an object /// implementing [`AsRawFd`]. The backing file descriptor is cached at the /// time of creation. /// /// This method must be called in the context of a tokio runtime. + /// + /// # Panics + /// + /// This function panics if there is no current reactor set and `rt` feature + /// flag is not enabled. + #[inline] + #[track_caller] pub fn new(inner: T) -> io::Result where T: AsRawFd, @@ -180,9 +186,15 @@ impl AsyncFd { Self::with_interest(inner, ALL_INTEREST) } - #[inline] /// Creates new instance as `new` with additional ability to customize interest, /// allowing to specify whether file descriptor will be polled for read, write or both. + /// + /// # Panics + /// + /// This function panics if there is no current reactor set and `rt` feature + /// flag is not enabled. + #[inline] + #[track_caller] pub fn with_interest(inner: T, interest: Interest) -> io::Result where T: AsRawFd, diff --git a/tokio/src/io/driver/mod.rs b/tokio/src/io/driver/mod.rs index 66bc3182206..4b420e1c8a2 100644 --- a/tokio/src/io/driver/mod.rs +++ b/tokio/src/io/driver/mod.rs @@ -258,6 +258,7 @@ cfg_rt! { /// /// This function panics if there is no current reactor set and `rt` feature /// flag is not enabled. + #[track_caller] pub(super) fn current() -> Self { crate::runtime::context::io_handle().expect("A Tokio 1.x context was found, but IO is disabled. Call `enable_io` on the runtime builder to enable IO.") } diff --git a/tokio/src/io/read_buf.rs b/tokio/src/io/read_buf.rs index 8c34ae6c817..0dc595a87dd 100644 --- a/tokio/src/io/read_buf.rs +++ b/tokio/src/io/read_buf.rs @@ -152,6 +152,7 @@ impl<'a> ReadBuf<'a> { /// /// Panics if `self.remaining()` is less than `n`. #[inline] + #[track_caller] pub fn initialize_unfilled_to(&mut self, n: usize) -> &mut [u8] { assert!(self.remaining() >= n, "n overflows remaining"); @@ -195,6 +196,7 @@ impl<'a> ReadBuf<'a> { /// /// Panics if the filled region of the buffer would become larger than the initialized region. #[inline] + #[track_caller] pub fn advance(&mut self, n: usize) { let new = self.filled.checked_add(n).expect("filled overflow"); self.set_filled(new); @@ -211,6 +213,7 @@ impl<'a> ReadBuf<'a> { /// /// Panics if the filled region of the buffer would become larger than the initialized region. #[inline] + #[track_caller] pub fn set_filled(&mut self, n: usize) { assert!( n <= self.initialized, @@ -241,6 +244,7 @@ impl<'a> ReadBuf<'a> { /// /// Panics if `self.remaining()` is less than `buf.len()`. #[inline] + #[track_caller] pub fn put_slice(&mut self, buf: &[u8]) { assert!( self.remaining() >= buf.len(), diff --git a/tokio/src/io/split.rs b/tokio/src/io/split.rs index 8258a0f7a08..2e0da95665f 100644 --- a/tokio/src/io/split.rs +++ b/tokio/src/io/split.rs @@ -74,6 +74,7 @@ impl ReadHalf { /// same `split` operation this method will panic. /// This can be checked ahead of time by comparing the stream ID /// of the two halves. + #[track_caller] pub fn unsplit(self, wr: WriteHalf) -> T { if self.is_pair_of(&wr) { drop(wr); diff --git a/tokio/tests/io_panic.rs b/tokio/tests/io_panic.rs new file mode 100644 index 00000000000..65f429a68b1 --- /dev/null +++ b/tokio/tests/io_panic.rs @@ -0,0 +1,168 @@ +#![warn(rust_2018_idioms)] +#![cfg(feature = "full")] + +use std::os::unix::prelude::{AsRawFd, RawFd}; +use std::task::{Context, Poll}; +use std::{error::Error, pin::Pin}; +use tokio::io::{self, split, unix::AsyncFd, AsyncRead, AsyncWrite, ReadBuf}; +use tokio::runtime::Builder; + +mod support { + pub mod panic; +} +use support::panic::test_panic; + +struct RW; + +impl AsyncRead for RW { + fn poll_read( + self: Pin<&mut Self>, + _cx: &mut Context<'_>, + buf: &mut ReadBuf<'_>, + ) -> Poll> { + buf.put_slice(&[b'z']); + Poll::Ready(Ok(())) + } +} + +impl AsyncWrite for RW { + fn poll_write( + self: Pin<&mut Self>, + _cx: &mut Context<'_>, + _buf: &[u8], + ) -> Poll> { + Poll::Ready(Ok(1)) + } + + fn poll_flush(self: Pin<&mut Self>, _cx: &mut Context<'_>) -> Poll> { + Poll::Ready(Ok(())) + } + + fn poll_shutdown(self: Pin<&mut Self>, _cx: &mut Context<'_>) -> Poll> { + Poll::Ready(Ok(())) + } +} + +struct MockFd; + +impl AsRawFd for MockFd { + fn as_raw_fd(&self) -> RawFd { + 0 + } +} + +#[test] +fn read_buf_initialize_unfilled_to_panic_caller() -> Result<(), Box> { + let panic_location_file = test_panic(|| { + let mut buffer = Vec::::new(); + let mut read_buf = ReadBuf::new(&mut buffer); + + read_buf.initialize_unfilled_to(2); + }); + + // The panic location should be in this file + assert_eq!(&panic_location_file.unwrap(), file!()); + + Ok(()) +} + +#[test] +fn read_buf_advance_panic_caller() -> Result<(), Box> { + let panic_location_file = test_panic(|| { + let mut buffer = Vec::::new(); + let mut read_buf = ReadBuf::new(&mut buffer); + + read_buf.advance(2); + }); + + // The panic location should be in this file + assert_eq!(&panic_location_file.unwrap(), file!()); + + Ok(()) +} + +#[test] +fn read_buf_set_filled_panic_caller() -> Result<(), Box> { + let panic_location_file = test_panic(|| { + let mut buffer = Vec::::new(); + let mut read_buf = ReadBuf::new(&mut buffer); + + read_buf.set_filled(2); + }); + + // The panic location should be in this file + assert_eq!(&panic_location_file.unwrap(), file!()); + + Ok(()) +} + +#[test] +fn read_buf_put_slice_panic_caller() -> Result<(), Box> { + let panic_location_file = test_panic(|| { + let mut buffer = Vec::::new(); + let mut read_buf = ReadBuf::new(&mut buffer); + + let new_slice = [0x40_u8, 0x41_u8]; + + read_buf.put_slice(&new_slice); + }); + + // The panic location should be in this file + assert_eq!(&panic_location_file.unwrap(), file!()); + + Ok(()) +} + +#[test] +fn unsplit_panic_caller() -> Result<(), Box> { + let panic_location_file = test_panic(|| { + let (r1, _w1) = split(RW); + let (_r2, w2) = split(RW); + r1.unsplit(w2); + }); + + // The panic location should be in this file + assert_eq!(&panic_location_file.unwrap(), file!()); + + Ok(()) +} + +#[test] +#[cfg(unix)] +fn async_fd_new_panic_caller() -> Result<(), Box> { + let panic_location_file = test_panic(|| { + // Runtime without `enable_io` so it has no current timer set. + let rt = Builder::new_current_thread().build().unwrap(); + rt.block_on(async { + let fd = MockFd; + + let _ = AsyncFd::new(fd); + }); + }); + + // The panic location should be in this file + assert_eq!(&panic_location_file.unwrap(), file!()); + + Ok(()) +} + +#[test] +#[cfg(unix)] +fn async_fd_with_interest_panic_caller() -> Result<(), Box> { + use tokio::io::Interest; + + let panic_location_file = test_panic(|| { + // Runtime without `enable_io` so it has no current timer set. + let rt = Builder::new_current_thread().build().unwrap(); + rt.block_on(async { + let fd = MockFd; + + let _ = AsyncFd::with_interest(fd, Interest::READABLE); + }); + }); + + // The panic location should be in this file + assert_eq!(&panic_location_file.unwrap(), file!()); + + Ok(()) +} diff --git a/tokio/tests/rt_panic.rs b/tokio/tests/rt_panic.rs index 240c3d0c606..90014736211 100644 --- a/tokio/tests/rt_panic.rs +++ b/tokio/tests/rt_panic.rs @@ -2,42 +2,13 @@ #![cfg(feature = "full")] use futures::future; -use parking_lot::{const_mutex, Mutex}; use std::error::Error; -use std::panic; -use std::sync::Arc; use tokio::runtime::{Builder, Handle, Runtime}; -fn test_panic(func: Func) -> Option { - static PANIC_MUTEX: Mutex<()> = const_mutex(()); - - { - let _guard = PANIC_MUTEX.lock(); - let panic_file: Arc>> = Arc::new(Mutex::new(None)); - - let prev_hook = panic::take_hook(); - { - let panic_file = panic_file.clone(); - panic::set_hook(Box::new(move |panic_info| { - let panic_location = panic_info.location().unwrap(); - panic_file - .lock() - .clone_from(&Some(panic_location.file().to_string())); - })); - } - - let result = panic::catch_unwind(func); - // Return to the previously set panic hook (maybe default) so that we get nice error - // messages in the tests. - panic::set_hook(prev_hook); - - if result.is_err() { - panic_file.lock().clone() - } else { - None - } - } +mod support { + pub mod panic; } +use support::panic::test_panic; #[test] fn current_handle_panic_caller() -> Result<(), Box> { diff --git a/tokio/tests/support/panic.rs b/tokio/tests/support/panic.rs new file mode 100644 index 00000000000..7f60c76f00a --- /dev/null +++ b/tokio/tests/support/panic.rs @@ -0,0 +1,34 @@ +use parking_lot::{const_mutex, Mutex}; +use std::panic; +use std::sync::Arc; + +pub fn test_panic(func: Func) -> Option { + static PANIC_MUTEX: Mutex<()> = const_mutex(()); + + { + let _guard = PANIC_MUTEX.lock(); + let panic_file: Arc>> = Arc::new(Mutex::new(None)); + + let prev_hook = panic::take_hook(); + { + let panic_file = panic_file.clone(); + panic::set_hook(Box::new(move |panic_info| { + let panic_location = panic_info.location().unwrap(); + panic_file + .lock() + .clone_from(&Some(panic_location.file().to_string())); + })); + } + + let result = panic::catch_unwind(func); + // Return to the previously set panic hook (maybe default) so that we get nice error + // messages in the tests. + panic::set_hook(prev_hook); + + if result.is_err() { + panic_file.lock().clone() + } else { + None + } + } +} diff --git a/tokio/tests/time_panic.rs b/tokio/tests/time_panic.rs index 6a262516f63..3ed936f52a8 100644 --- a/tokio/tests/time_panic.rs +++ b/tokio/tests/time_panic.rs @@ -2,44 +2,15 @@ #![cfg(feature = "full")] use futures::future; -use parking_lot::{const_mutex, Mutex}; use std::error::Error; -use std::panic; -use std::sync::Arc; use std::time::Duration; use tokio::runtime::{Builder, Runtime}; use tokio::time::{self, interval, interval_at, timeout, Instant}; -fn test_panic(func: Func) -> Option { - static PANIC_MUTEX: Mutex<()> = const_mutex(()); - - { - let _guard = PANIC_MUTEX.lock(); - let panic_file: Arc>> = Arc::new(Mutex::new(None)); - - let prev_hook = panic::take_hook(); - { - let panic_file = panic_file.clone(); - panic::set_hook(Box::new(move |panic_info| { - let panic_location = panic_info.location().unwrap(); - panic_file - .lock() - .clone_from(&Some(panic_location.file().to_string())); - })); - } - - let result = panic::catch_unwind(func); - // Return to the previously set panic hook (maybe default) so that we get nice error - // messages in the tests. - panic::set_hook(prev_hook); - - if result.is_err() { - panic_file.lock().clone() - } else { - None - } - } +mod support { + pub mod panic; } +use support::panic::test_panic; #[test] fn pause_panic_caller() -> Result<(), Box> { From 655adc3e73269ac0f5a34166f6e9d5869d545f8d Mon Sep 17 00:00:00 2001 From: "Stainsby, Hayden" Date: Tue, 28 Jun 2022 09:08:24 +0200 Subject: [PATCH 2/3] Move unix specific structs behind unix cfg RawFd and AsRawFd aren't available on all systems, so the helper struct for the AsyncFd tests need to be conditionally compiled for unix only. --- tokio/tests/io_panic.rs | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/tokio/tests/io_panic.rs b/tokio/tests/io_panic.rs index 65f429a68b1..403edb4fcc1 100644 --- a/tokio/tests/io_panic.rs +++ b/tokio/tests/io_panic.rs @@ -1,7 +1,6 @@ #![warn(rust_2018_idioms)] #![cfg(feature = "full")] -use std::os::unix::prelude::{AsRawFd, RawFd}; use std::task::{Context, Poll}; use std::{error::Error, pin::Pin}; use tokio::io::{self, split, unix::AsyncFd, AsyncRead, AsyncWrite, ReadBuf}; @@ -43,11 +42,16 @@ impl AsyncWrite for RW { } } -struct MockFd; +#[cfg(unix)] +mod unix { + use std::os::unix::prelude::{AsRawFd, RawFd}; + + pub struct MockFd; -impl AsRawFd for MockFd { - fn as_raw_fd(&self) -> RawFd { - 0 + impl AsRawFd for MockFd { + fn as_raw_fd(&self) -> RawFd { + 0 + } } } @@ -134,7 +138,7 @@ fn async_fd_new_panic_caller() -> Result<(), Box> { // Runtime without `enable_io` so it has no current timer set. let rt = Builder::new_current_thread().build().unwrap(); rt.block_on(async { - let fd = MockFd; + let fd = unix::MockFd; let _ = AsyncFd::new(fd); }); @@ -155,7 +159,7 @@ fn async_fd_with_interest_panic_caller() -> Result<(), Box> { // Runtime without `enable_io` so it has no current timer set. let rt = Builder::new_current_thread().build().unwrap(); rt.block_on(async { - let fd = MockFd; + let fd = unix::MockFd; let _ = AsyncFd::with_interest(fd, Interest::READABLE); }); From 68927e4b0530b9ad9afe2f30aa24be503f03ba66 Mon Sep 17 00:00:00 2001 From: "Stainsby, Hayden" Date: Tue, 28 Jun 2022 11:22:35 +0200 Subject: [PATCH 3/3] Fixed comments referring to time (should have been IO) Also fixed clippy warnings on windows where certain imports weren't being used (because they are only used in tests with the unix cfg). --- tokio/src/io/async_fd.rs | 8 ++++---- tokio/tests/io_panic.rs | 12 ++++++++---- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/tokio/src/io/async_fd.rs b/tokio/src/io/async_fd.rs index 7b3953d5ca2..d91710a40b9 100644 --- a/tokio/src/io/async_fd.rs +++ b/tokio/src/io/async_fd.rs @@ -175,8 +175,8 @@ impl AsyncFd { /// /// # Panics /// - /// This function panics if there is no current reactor set and `rt` feature - /// flag is not enabled. + /// This function panics if there is no current reactor set, or if the `rt` + /// feature flag is not enabled. #[inline] #[track_caller] pub fn new(inner: T) -> io::Result @@ -191,8 +191,8 @@ impl AsyncFd { /// /// # Panics /// - /// This function panics if there is no current reactor set and `rt` feature - /// flag is not enabled. + /// This function panics if there is no current reactor set, or if the `rt` + /// feature flag is not enabled. #[inline] #[track_caller] pub fn with_interest(inner: T, interest: Interest) -> io::Result diff --git a/tokio/tests/io_panic.rs b/tokio/tests/io_panic.rs index 403edb4fcc1..07d824d1660 100644 --- a/tokio/tests/io_panic.rs +++ b/tokio/tests/io_panic.rs @@ -3,8 +3,7 @@ use std::task::{Context, Poll}; use std::{error::Error, pin::Pin}; -use tokio::io::{self, split, unix::AsyncFd, AsyncRead, AsyncWrite, ReadBuf}; -use tokio::runtime::Builder; +use tokio::io::{self, split, AsyncRead, AsyncWrite, ReadBuf}; mod support { pub mod panic; @@ -134,8 +133,11 @@ fn unsplit_panic_caller() -> Result<(), Box> { #[test] #[cfg(unix)] fn async_fd_new_panic_caller() -> Result<(), Box> { + use tokio::io::unix::AsyncFd; + use tokio::runtime::Builder; + let panic_location_file = test_panic(|| { - // Runtime without `enable_io` so it has no current timer set. + // Runtime without `enable_io` so it has no IO driver set. let rt = Builder::new_current_thread().build().unwrap(); rt.block_on(async { let fd = unix::MockFd; @@ -153,10 +155,12 @@ fn async_fd_new_panic_caller() -> Result<(), Box> { #[test] #[cfg(unix)] fn async_fd_with_interest_panic_caller() -> Result<(), Box> { + use tokio::io::unix::AsyncFd; use tokio::io::Interest; + use tokio::runtime::Builder; let panic_location_file = test_panic(|| { - // Runtime without `enable_io` so it has no current timer set. + // Runtime without `enable_io` so it has no IO driver set. let rt = Builder::new_current_thread().build().unwrap(); rt.block_on(async { let fd = unix::MockFd;