From 2ab60c20a3b5609b2868670c8c5081aeb58322ce Mon Sep 17 00:00:00 2001 From: "Stainsby, Hayden" Date: Mon, 27 Jun 2022 17:37:31 +0200 Subject: [PATCH] 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> {