From bffdb1aad8bb5479316a401de148a290b001dd44 Mon Sep 17 00:00:00 2001 From: Oliver Gould Date: Tue, 1 Feb 2022 12:03:45 -0800 Subject: [PATCH] Ban uses of `Instant` operations that can panic (#1456) When comparing instances, we should use saturating varieties to help ensure that we can't hit panics. This change bans uses of `std::time::Instant::{duration_since, elapsed, sub}` via clippy. Uses are ported to using `Instant::saturating_duration_since`. Related to linkerd/linkerd2#7748 Signed-off-by: Oliver Gould Co-authored-by: Eliza Weisman --- .clippy.toml | 7 ++++++- linkerd/app/integration/src/lib.rs | 2 +- linkerd/app/integration/src/metrics.rs | 2 +- linkerd/detect/src/lib.rs | 2 +- linkerd/http-access-log/src/lib.rs | 2 +- linkerd/http-metrics/src/requests/service.rs | 3 ++- linkerd/proxy/tap/src/grpc/server.rs | 14 +++++++++----- linkerd/stack/metrics/src/service.rs | 6 +++--- linkerd/tracing/src/uptime.rs | 2 +- 9 files changed, 25 insertions(+), 15 deletions(-) diff --git a/.clippy.toml b/.clippy.toml index 9d20eb1dea..644ae3fd0f 100644 --- a/.clippy.toml +++ b/.clippy.toml @@ -5,4 +5,9 @@ disallowed-methods = [ # see https://github.com/rust-lang/rust/issues/90308 for details. "std::env::set_var", "std::env::remove_var", -] \ No newline at end of file + + # Avoid instances of https://github.com/rust-lang/rust/issues/86470 + "std::time::Instant::duration_since", + "std::time::Instant::elapsed", + # Clippy doesn't let us ban std::time::Instant::sub, but it knows what it did. +] diff --git a/linkerd/app/integration/src/lib.rs b/linkerd/app/integration/src/lib.rs index 8bb7972124..a16f255284 100644 --- a/linkerd/app/integration/src/lib.rs +++ b/linkerd/app/integration/src/lib.rs @@ -89,7 +89,7 @@ macro_rules! assert_eventually { } else if i == $retries { panic!( "assertion failed after {} (retried {} times): {}", - crate::HumanDuration(start_t.elapsed()), + crate::HumanDuration(Instant::now().saturating_duration_since(start_t)), i, format_args!($($arg)+) ) diff --git a/linkerd/app/integration/src/metrics.rs b/linkerd/app/integration/src/metrics.rs index c8f9a952c9..c2bfe52a17 100644 --- a/linkerd/app/integration/src/metrics.rs +++ b/linkerd/app/integration/src/metrics.rs @@ -221,7 +221,7 @@ impl MetricMatch { "{}\n retried {} times ({:?})", e, MAX_RETRIES, - start_t.elapsed() + Instant::now().saturating_duration_since(start_t) ), Err(_) => { tracing::trace!("waiting..."); diff --git a/linkerd/detect/src/lib.rs b/linkerd/detect/src/lib.rs index bd125a9647..79991d13ec 100644 --- a/linkerd/detect/src/lib.rs +++ b/linkerd/detect/src/lib.rs @@ -147,7 +147,7 @@ where let mut buf = BytesMut::with_capacity(capacity); let detected = match time::timeout(timeout, detect.detect(&mut io, &mut buf)).await { Ok(Ok(protocol)) => { - debug!(?protocol, elapsed = ?t0.elapsed(), "DetectResult"); + debug!(?protocol, elapsed = ?time::Instant::now().saturating_duration_since(t0), "DetectResult"); Ok(protocol) } Err(_) => Err(DetectTimeoutError(timeout, std::marker::PhantomData)), diff --git a/linkerd/http-access-log/src/lib.rs b/linkerd/http-access-log/src/lib.rs index a01a5677bb..2706184165 100644 --- a/linkerd/http-access-log/src/lib.rs +++ b/linkerd/http-access-log/src/lib.rs @@ -174,7 +174,7 @@ where let response: http::Response = match this.inner.try_poll(cx) { Poll::Pending => { - data.processing += Instant::now().duration_since(poll_start); + data.processing += Instant::now().saturating_duration_since(poll_start); return Poll::Pending; } Poll::Ready(Err(e)) => return Poll::Ready(Err(e)), diff --git a/linkerd/http-metrics/src/requests/service.rs b/linkerd/http-metrics/src/requests/service.rs index e2430c405d..48d50e501a 100644 --- a/linkerd/http-metrics/src/requests/service.rs +++ b/linkerd/http-metrics/src/requests/service.rs @@ -343,7 +343,8 @@ where .entry(Some(*this.status)) .or_insert_with(StatusMetrics::default); - status_metrics.latency.add(now - *this.stream_open_at); + let elapsed = now.saturating_duration_since(*this.stream_open_at); + status_metrics.latency.add(elapsed); *this.latency_recorded = true; } diff --git a/linkerd/proxy/tap/src/grpc/server.rs b/linkerd/proxy/tap/src/grpc/server.rs index f199ebc22f..c62a658a8e 100644 --- a/linkerd/proxy/tap/src/grpc/server.rs +++ b/linkerd/proxy/tap/src/grpc/server.rs @@ -364,10 +364,10 @@ impl iface::TapResponse for TapResponse { } else { None }; - + let since_request_init = response_init_at.saturating_duration_since(self.request_init_at); let init = api::tap_event::http::Event::ResponseInit(api::tap_event::http::ResponseInit { id: Some(self.tap.id.clone()), - since_request_init: Some((response_init_at - self.request_init_at).into()), + since_request_init: Some(since_request_init.into()), http_status: rsp.status().as_u16().into(), headers, }); @@ -398,9 +398,10 @@ impl iface::TapResponse for TapResponse { fn fail(self, err: &E) { let response_end_at = Instant::now(); let reason = err.h2_reason(); + let since_request_init = response_end_at.saturating_duration_since(self.request_init_at); let end = api::tap_event::http::Event::ResponseEnd(api::tap_event::http::ResponseEnd { id: Some(self.tap.id.clone()), - since_request_init: Some((response_end_at - self.request_init_at).into()), + since_request_init: Some(since_request_init.into()), since_response_init: None, response_bytes: 0, eos: Some(api::Eos { @@ -464,10 +465,13 @@ impl TapResponsePayload { } else { None }; + + let since_request_init = response_end_at.saturating_duration_since(self.request_init_at); + let since_response_init = response_end_at.saturating_duration_since(self.response_init_at); let end = api::tap_event::http::ResponseEnd { id: Some(self.tap.id), - since_request_init: Some((response_end_at - self.request_init_at).into()), - since_response_init: Some((response_end_at - self.response_init_at).into()), + since_request_init: Some(since_request_init.into()), + since_response_init: Some(since_response_init.into()), response_bytes: self.response_bytes as u64, eos: Some(api::Eos { end }), trailers, diff --git a/linkerd/stack/metrics/src/service.rs b/linkerd/stack/metrics/src/service.rs index 8409b554d6..d57a03fbca 100644 --- a/linkerd/stack/metrics/src/service.rs +++ b/linkerd/stack/metrics/src/service.rs @@ -39,7 +39,7 @@ where // updated even when we're "stuck" in pending. let now = Instant::now(); if let Some(t0) = self.blocked_since.take() { - let not_ready = now - t0; + let not_ready = now.saturating_duration_since(t0); self.metrics.poll_millis.add(not_ready.as_millis() as u64); } self.blocked_since = Some(now); @@ -48,7 +48,7 @@ where Poll::Ready(Ok(())) => { self.metrics.ready_total.incr(); if let Some(t0) = self.blocked_since.take() { - let not_ready = Instant::now() - t0; + let not_ready = Instant::now().saturating_duration_since(t0); self.metrics.poll_millis.add(not_ready.as_millis() as u64); } Poll::Ready(Ok(())) @@ -56,7 +56,7 @@ where Poll::Ready(Err(e)) => { self.metrics.error_total.incr(); if let Some(t0) = self.blocked_since.take() { - let not_ready = Instant::now() - t0; + let not_ready = Instant::now().saturating_duration_since(t0); self.metrics.poll_millis.add(not_ready.as_millis() as u64); } Poll::Ready(Err(e)) diff --git a/linkerd/tracing/src/uptime.rs b/linkerd/tracing/src/uptime.rs index 0b35d9c122..4667a1a7ab 100644 --- a/linkerd/tracing/src/uptime.rs +++ b/linkerd/tracing/src/uptime.rs @@ -21,7 +21,7 @@ impl Uptime { impl FormatTime for Uptime { fn format_time(&self, w: &mut format::Writer<'_>) -> fmt::Result { - Self::format(Instant::now() - self.start_time, w) + Self::format(Instant::now().saturating_duration_since(self.start_time), w) } }