Skip to content

clippy: Disallow lock and instant types from std #1458

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Feb 2, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 8 additions & 7 deletions .clippy.toml
Original file line number Diff line number Diff line change
Expand Up @@ -6,16 +6,17 @@ disallowed-methods = [
"std::env::set_var",
"std::env::remove_var",

# Use parking_lot instead.
"std::sync::Mutex",
"std::sync::RwLock",

# Use tokio::time::Instant instead.
"std::time::Instant::now",

# Avoid instances of https://github.com/rust-lang/rust/issues/86470 until tokio/tokio#4461 is
# available.
"tokio::time::Instant::duration_since",
"tokio::time::Instant::elapsed",
# Clippy doesn't let us ban tokio::time::Instant::sub, but it knows what it did.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol

]
disallowed-types = [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think we might also need to add #![deny(clippy::disallowed_types)] to every crate in the project. based on a quick grep, there are definitely a few uses of std::sync locks in the codebase, so i'm surprised this isn't catching them...

most uses are in tests, but there's at least one std::sync::RwLock in linkerd/stack/src/fail_on_error.rs according to grep.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks. fixed

# Use parking_lot instead.
"std::sync::Mutex",
"std::sync::RwLock",

# Use tokio::time::Instant instead.
"std::time::Instant",
Comment on lines +16 to +21
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking at the clippy docs, it seems like we can also add reasons to these, so that they're displayed with the warning when a disallowed type is used. we may want to add that, but not a blocker.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

afaict this isn't supported in our version of clippy

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, got it. could be nice to add later when it's available.

]
2 changes: 1 addition & 1 deletion linkerd/app/admin/src/server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ where
mod tests {
use super::*;
use http::method::Method;
use tokio::time::Duration;
use std::time::Duration;
use tokio::{sync::mpsc, time::timeout};
use tower::util::ServiceExt;

Expand Down
2 changes: 1 addition & 1 deletion linkerd/app/core/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use crate::{
svc::{stack::CloneParam, Param},
transport::{Keepalive, ListenAddr},
};
use tokio::time::Duration;
use std::time::Duration;

#[derive(Clone, Debug)]
pub struct ServerConfig {
Expand Down
2 changes: 1 addition & 1 deletion linkerd/app/core/src/control.rs
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,7 @@ mod resolve {

mod balance {
use crate::proxy::http;
use tokio::time::Duration;
use std::time::Duration;

const EWMA_DEFAULT_RTT: Duration = Duration::from_millis(30);
const EWMA_DECAY: Duration = Duration::from_secs(10);
Expand Down
2 changes: 1 addition & 1 deletion linkerd/app/core/src/dns.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
pub use linkerd_dns::*;
use std::path::PathBuf;
use tokio::time::Duration;
use std::time::Duration;

#[derive(Clone, Debug)]
pub struct Config {
Expand Down
2 changes: 1 addition & 1 deletion linkerd/app/core/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ pub use tonic::Code as Grpc;

#[derive(Debug, Error)]
#[error("connect timed out after {0:?}")]
pub struct ConnectTimeout(pub(crate) tokio::time::Duration);
pub struct ConnectTimeout(pub(crate) std::time::Duration);

/// Obtain the source error at the end of a chain of `Error`s.
pub fn root_cause<'e>(
Expand Down
2 changes: 1 addition & 1 deletion linkerd/app/core/src/transport.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ pub use self::allow_ips::AllowIps;
pub struct Metrics(metrics::Registry<labels::Key>);

impl Metrics {
pub fn new(retain_idle: tokio::time::Duration) -> (Self, metrics::Report<labels::Key>) {
pub fn new(retain_idle: std::time::Duration) -> (Self, metrics::Report<labels::Key>) {
let (reg, report) = metrics::new(retain_idle);
(Self(reg), report)
}
Expand Down
4 changes: 2 additions & 2 deletions linkerd/app/inbound/src/detect.rs
Original file line number Diff line number Diff line change
Expand Up @@ -504,7 +504,7 @@ mod tests {
negotiated_protocol: None,
}),
policy: allow(Protocol::Detect {
timeout: tokio::time::Duration::from_secs(10),
timeout: std::time::Duration::from_secs(10),
}),
};

Expand Down Expand Up @@ -533,7 +533,7 @@ mod tests {
negotiated_protocol: None,
}),
policy: allow(Protocol::Detect {
timeout: tokio::time::Duration::from_secs(10),
timeout: std::time::Duration::from_secs(10),
}),
};

Expand Down
2 changes: 1 addition & 1 deletion linkerd/app/inbound/src/http/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -513,7 +513,7 @@ fn connect_timeout(
Box::pin(
async move {
tracing::info!("sleeping so that the proxy hits a connect timeout");
tokio::time::sleep(tokio::time::Duration::from_secs(3)).await;
tokio::time::sleep(std::time::Duration::from_secs(3)).await;
// The proxy hits a connect timeout so we don't need to worry
// about returning a service here.
unreachable!();
Expand Down
2 changes: 1 addition & 1 deletion linkerd/app/inbound/src/policy/defaults.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use linkerd_app_core::{IpNet, Ipv4Net, Ipv6Net};
use linkerd_server_policy::{Authentication, Authorization, Protocol, ServerPolicy, Suffix};
use tokio::time::Duration;
use std::time::Duration;

pub fn all_authenticated(timeout: Duration) -> ServerPolicy {
mk(
Expand Down
6 changes: 3 additions & 3 deletions linkerd/app/inbound/src/test_util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ use linkerd_app_core::{
};
pub use linkerd_app_test as support;
use linkerd_server_policy::{Authentication, Authorization, Protocol, ServerPolicy};
use tokio::time::Duration;
use std::time::Duration;

pub fn default_config() -> Config {
let cluster_local = "svc.cluster.local."
Expand Down Expand Up @@ -53,7 +53,7 @@ pub fn default_config() -> Config {
policy: policy::Config::Fixed {
default: ServerPolicy {
protocol: Protocol::Detect {
timeout: tokio::time::Duration::from_secs(10),
timeout: std::time::Duration::from_secs(10),
},
authorizations: vec![Authorization {
authentication: Authentication::Unauthenticated,
Expand All @@ -73,7 +73,7 @@ pub fn default_config() -> Config {
pub fn runtime() -> (ProxyRuntime, drain::Signal) {
let (drain_tx, drain) = drain::channel();
let (tap, _) = tap::new();
let (metrics, _) = metrics::Metrics::new(tokio::time::Duration::from_secs(10));
let (metrics, _) = metrics::Metrics::new(std::time::Duration::from_secs(10));
let runtime = ProxyRuntime {
identity: rustls::creds::default_for_test().1.into(),
metrics: metrics.proxy,
Expand Down
2 changes: 1 addition & 1 deletion linkerd/app/integration/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,10 @@ pub use std::net::SocketAddr;
use std::pin::Pin;
pub use std::sync::Arc;
use std::task::{Context, Poll};
pub use std::time::Duration;
pub use tokio::io::{AsyncRead, AsyncReadExt, AsyncWrite, AsyncWriteExt, ReadBuf};
use tokio::net::TcpListener;
pub use tokio::sync::oneshot;
pub use tokio::time::Duration;
pub use tonic as grpc;
pub use tower::Service;
pub use tracing::*;
Expand Down
2 changes: 1 addition & 1 deletion linkerd/app/integration/src/tests/profile_dst_overrides.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ async fn wait_for_profile_stage(client: &client::Client, metrics: &client::Clien
break;
}

tokio::time::sleep(tokio::time::Duration::from_millis(200)).await;
tokio::time::sleep(std::time::Duration::from_millis(200)).await;
}
}

Expand Down
2 changes: 1 addition & 1 deletion linkerd/app/integration/src/tests/profiles.rs
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ macro_rules! profile_test {
break;
}

tokio::time::sleep(tokio::time::Duration::from_millis(200)).await;
tokio::time::sleep(std::time::Duration::from_millis(200)).await;
}

$with_client(client).await;
Expand Down
2 changes: 1 addition & 1 deletion linkerd/app/integration/src/tests/transparency.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1367,7 +1367,7 @@ async fn retry_reconnect_errors() {

drop(tx); // start `listen` now
// This may be flaky on CI.
tokio::time::sleep(tokio::time::Duration::from_millis(100)).await;
tokio::time::sleep(std::time::Duration::from_millis(100)).await;

let res = fut.await.expect("response");
assert_eq!(res.status(), http::StatusCode::OK);
Expand Down
2 changes: 1 addition & 1 deletion linkerd/app/outbound/src/resolve.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use linkerd_app_core::{
},
svc::{layer, NewService},
};
use tokio::time::Duration;
use std::time::Duration;

pub fn layer<T, R, N>(
resolve: R,
Expand Down
2 changes: 1 addition & 1 deletion linkerd/app/outbound/src/test_util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ pub(crate) fn default_config() -> Config {
pub(crate) fn runtime() -> (ProxyRuntime, drain::Signal) {
let (drain_tx, drain) = drain::channel();
let (tap, _) = tap::new();
let (metrics, _) = metrics::Metrics::new(tokio::time::Duration::from_secs(10));
let (metrics, _) = metrics::Metrics::new(std::time::Duration::from_secs(10));
let runtime = ProxyRuntime {
identity: linkerd_meshtls_rustls::creds::default_for_test().1.into(),
metrics: metrics.proxy,
Expand Down
2 changes: 1 addition & 1 deletion linkerd/app/src/tap.rs
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ impl Tap {
impl<T> ExtractParam<tls::server::Timeout, T> for TlsParams {
#[inline]
fn extract_param(&self, _: &T) -> tls::server::Timeout {
tls::server::Timeout(tokio::time::Duration::from_secs(1))
tls::server::Timeout(std::time::Duration::from_secs(1))
}
}

Expand Down
2 changes: 1 addition & 1 deletion linkerd/exp-backoff/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,9 @@ use rand::{rngs::SmallRng, thread_rng, SeedableRng};
use std::future::Future;
use std::pin::Pin;
use std::task::{Context, Poll};
use std::time::Duration;
use thiserror::Error;
use tokio::time;
use tokio::time::Duration;

/// A jittered exponential backoff strategy.
// The raw fields are exposed so this type can be constructed statically.
Expand Down
2 changes: 1 addition & 1 deletion linkerd/meshtls/boring/src/tests.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use linkerd_identity::{Credentials, DerX509};
use linkerd_tls_test_util::*;
use tokio::time::Duration;
use std::time::Duration;

fn load(ent: &Entity) -> crate::creds::Store {
let roots_pem = std::str::from_utf8(ent.trust_anchors).expect("valid PEM");
Expand Down
2 changes: 1 addition & 1 deletion linkerd/meshtls/rustls/src/tests.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use linkerd_identity::{Credentials, DerX509};
use linkerd_tls_test_util::*;
use tokio::time::Duration;
use std::time::Duration;

fn load(ent: &Entity) -> crate::creds::Store {
let roots_pem = std::str::from_utf8(ent.trust_anchors).expect("valid PEM");
Expand Down
2 changes: 1 addition & 1 deletion linkerd/proxy/discover/src/buffer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@ use pin_project::pin_project;
use std::future::Future;
use std::pin::Pin;
use std::task::{Context, Poll};
use std::time::Duration;
use tokio::sync::{mpsc, oneshot};
use tokio::time::Duration;
use tokio::time::{self, Sleep};
use tokio_util::sync::PollSender;
use tower::discover;
Expand Down
2 changes: 1 addition & 1 deletion linkerd/proxy/http/src/timeout.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use linkerd_error::Error;
use linkerd_stack::{layer, MapErr, NewService, Param, Timeout, TimeoutError};
use std::time::Duration;
use thiserror::Error;
use tokio::time::Duration;

#[derive(Clone, Debug)]
pub struct ResponseTimeout(pub Option<Duration>);
Expand Down
2 changes: 1 addition & 1 deletion linkerd/proxy/transport/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@ pub use self::{
};
use linkerd_io as io;
use socket2::TcpKeepalive;
use std::time::Duration;
use tokio::net::TcpStream;
use tokio::time::Duration;

#[derive(Copy, Clone, Debug, Default)]
pub struct Keepalive(pub Option<Duration>);
Expand Down
2 changes: 1 addition & 1 deletion linkerd/stack/src/failfast.rs
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ where
#[cfg(test)]
mod test {
use super::FailFast;
use tokio::time::Duration;
use std::time::Duration;
use tokio_test::{assert_pending, assert_ready, assert_ready_ok};
use tower::layer::Layer;
use tower_test::mock::{self, Spawn};
Expand Down