Skip to content

Commit 1e3f9b5

Browse files
Lorak-mmkLorak-mmk
authored
Merge pull request #1404 from Lorak-mmk/fix_tracing_double_formatting
Fix panic on double formatting in traces
2 parents 8986057 + 97a8050 commit 1e3f9b5

File tree

14 files changed

+160
-27
lines changed

14 files changed

+160
-27
lines changed

‎clippy.toml‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1 +1,4 @@
11
avoid-breaking-exported-api = false
2+
disallowed-methods = [
3+
{ path = "itertools::Itertools::format", reason = "Footgun: panics on double formatting.", replacement = "crate::utils::safe_format::IteratorSafeFormatExt::safe_format" }
4+
]

‎scylla-cql/src/utils/mod.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,3 +2,4 @@
22
// TODO(2.0): Deconstruct this module by extracting submodules to the top level.
33

44
pub mod parse;
5+
pub(crate) mod safe_format;

‎scylla-cql/src/utils/safe_format.rs‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
use itertools::Itertools;
2+
use std::fmt::{Debug, Display, Formatter};
3+
4+
pub(crate) struct SafeFormat<'a, I: ?Sized> {
5+
sep: &'a str,
6+
cloneable_iter: I,
7+
}
8+
9+
impl<I> Display for SafeFormat<'_, I>
10+
where
11+
I: Iterator + Clone,
12+
<I as Iterator>::Item: Display,
13+
{
14+
fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
15+
#[allow(clippy::disallowed_methods)]
16+
self.cloneable_iter.clone().format(self.sep).fmt(f)
17+
}
18+
}
19+
20+
impl<I> Debug for SafeFormat<'_, I>
21+
where
22+
I: Iterator + Clone,
23+
<I as Iterator>::Item: Debug,
24+
{
25+
fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
26+
#[allow(clippy::disallowed_methods)]
27+
self.cloneable_iter.clone().format(self.sep).fmt(f)
28+
}
29+
}
30+
31+
pub(crate) trait IteratorSafeFormatExt: Iterator {
32+
fn safe_format(self, sep: &str) -> SafeFormat<'_, Self>
33+
where
34+
Self: Clone;
35+
}
36+
37+
impl<I: Iterator> IteratorSafeFormatExt for I {
38+
fn safe_format(self, sep: &str) -> SafeFormat<'_, Self>
39+
where
40+
Self: Clone,
41+
{
42+
SafeFormat {
43+
sep,
44+
cloneable_iter: self,
45+
}
46+
}
47+
}

‎scylla-cql/src/value.rs‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ use crate::deserialize::DeserializationError;
1515
use crate::deserialize::FrameSlice;
1616
use crate::frame::response::result::{CollectionType, ColumnType};
1717
use crate::frame::types;
18+
use crate::utils::safe_format::IteratorSafeFormatExt;
1819

1920
/// Error type indicating that the value is too large to fit in the destination type.
2021
///
@@ -1229,7 +1230,6 @@ impl std::fmt::Display for CqlValue {
12291230
use crate::pretty::{
12301231
CqlStringLiteralDisplayer, HexBytes, MaybeNullDisplayer, PairDisplayer,
12311232
};
1232-
use itertools::Itertools;
12331233

12341234
match self {
12351235
// Scalar types
@@ -1291,25 +1291,25 @@ impl std::fmt::Display for CqlValue {
12911291
f.write_str("(")?;
12921292
t.iter()
12931293
.map(|x| MaybeNullDisplayer(x.as_ref()))
1294-
.format(",")
1294+
.safe_format(",")
12951295
.fmt(f)?;
12961296
f.write_str(")")?;
12971297
}
12981298
CqlValue::List(v) | CqlValue::Vector(v) => {
12991299
f.write_str("[")?;
1300-
v.iter().format(",").fmt(f)?;
1300+
v.iter().safe_format(",").fmt(f)?;
13011301
f.write_str("]")?;
13021302
}
13031303
CqlValue::Set(v) => {
13041304
f.write_str("{")?;
1305-
v.iter().format(",").fmt(f)?;
1305+
v.iter().safe_format(",").fmt(f)?;
13061306
f.write_str("}")?;
13071307
}
13081308
CqlValue::Map(m) => {
13091309
f.write_str("{")?;
13101310
m.iter()
13111311
.map(|(k, v)| PairDisplayer(k, v))
1312-
.format(",")
1312+
.safe_format(",")
13131313
.fmt(f)?;
13141314
f.write_str("}")?;
13151315
}
@@ -1322,7 +1322,7 @@ impl std::fmt::Display for CqlValue {
13221322
fields
13231323
.iter()
13241324
.map(|(k, v)| PairDisplayer(k, MaybeNullDisplayer(v.as_ref())))
1325-
.format(",")
1325+
.safe_format(",")
13261326
.fmt(f)?;
13271327
f.write_str("}")?;
13281328
}

‎scylla-proxy/src/lib.rs‎

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,16 @@ pub use proxy::get_exclusive_local_address;
1717

1818
#[cfg(test)]
1919
pub(crate) fn setup_tracing() {
20-
let _ = tracing_subscriber::fmt::fmt()
21-
.with_env_filter(tracing_subscriber::EnvFilter::from_default_env())
22-
.with_writer(tracing_subscriber::fmt::TestWriter::new())
20+
use tracing_subscriber::layer::SubscriberExt;
21+
use tracing_subscriber::util::SubscriberInitExt;
22+
use tracing_subscriber::Layer;
23+
24+
let testing_layer = tracing_subscriber::fmt::layer()
25+
.with_test_writer()
26+
.with_filter(tracing_subscriber::EnvFilter::from_default_env());
27+
let noop_layer = tracing_subscriber::fmt::layer().with_writer(std::io::sink);
28+
let _ = tracing_subscriber::registry()
29+
.with(testing_layer)
30+
.with(noop_layer)
2331
.try_init();
2432
}

‎scylla/src/cluster/metadata.rs‎

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -29,13 +29,13 @@ use crate::observability::metrics::Metrics;
2929
use crate::policies::host_filter::HostFilter;
3030
use crate::routing::Token;
3131
use crate::statement::unprepared::Statement;
32+
use crate::utils::safe_format::IteratorSafeFormatExt;
3233
use crate::DeserializeRow;
3334
use scylla_cql::utils::parse::{ParseErrorCause, ParseResult, ParserState};
3435

3536
use futures::future::{self, FutureExt};
3637
use futures::stream::{self, StreamExt, TryStreamExt};
3738
use futures::Stream;
38-
use itertools::Itertools;
3939
use rand::seq::{IndexedRandom, SliceRandom};
4040
use rand::{rng, Rng};
4141
use scylla_cql::frame::response::result::{ColumnSpec, TableSpec};
@@ -514,7 +514,10 @@ impl MetadataReader {
514514

515515
// shuffle known_peers to iterate through them in random order later
516516
self.known_peers.shuffle(&mut rng());
517-
debug!("Known peers: {:?}", self.known_peers.iter().format(", "));
517+
debug!(
518+
"Known peers: {:?}",
519+
self.known_peers.iter().safe_format(", ")
520+
);
518521

519522
let address_of_failed_control_connection = self.control_connection_endpoint.address();
520523
let filtered_known_peers = self
@@ -649,7 +652,11 @@ impl MetadataReader {
649652
if !metadata.peers.is_empty() && self.known_peers.is_empty() {
650653
error!(
651654
node_ips = tracing::field::display(
652-
metadata.peers.iter().map(|peer| peer.address).format(", ")
655+
metadata
656+
.peers
657+
.iter()
658+
.map(|peer| peer.address)
659+
.safe_format(", ")
653660
),
654661
"The host filter rejected all nodes in the cluster, \
655662
no connections that can serve user queries have been \
@@ -671,7 +678,7 @@ impl MetadataReader {
671678
.iter()
672679
.filter(|peer| self.host_filter.as_ref().is_none_or(|p| p.accept(peer)))
673680
.map(|peer| peer.address)
674-
.format(", ")
681+
.safe_format(", ")
675682
),
676683
control_connection_address = ?self.control_connection_endpoint.address(),
677684
"The node that the control connection is established to \

‎scylla/src/cluster/state.rs‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ use crate::routing::locator::tablets::{RawTablet, Tablet, TabletsInfo};
77
use crate::routing::locator::ReplicaLocator;
88
use crate::routing::partitioner::{calculate_token_for_partition_key, PartitionerName};
99
use crate::routing::{Shard, Token};
10+
use crate::utils::safe_format::IteratorSafeFormatExt;
1011

1112
use itertools::Itertools;
1213
use scylla_cql::frame::response::result::TableSpec;
@@ -426,7 +427,7 @@ impl ClusterState {
426427
Err((t, f)) => {
427428
debug!("Nodes ({}) that are replicas for a tablet {{ks: {}, table: {}, range: [{}. {}]}} not present in current ClusterState.known_peers. \
428429
Skipping these replicas until topology refresh",
429-
f.iter().format(", "), table.ks_name(), table.table_name(), t.range().0.value(), t.range().1.value());
430+
f.iter().safe_format(", "), table.ks_name(), table.table_name(), t.range().0.value(), t.range().1.value());
430431
t
431432
}
432433
};

‎scylla/src/network/connection_pool.rs‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,10 +14,10 @@ use crate::cluster::metadata::{PeerEndpoint, UntranslatedEndpoint};
1414
use crate::observability::metrics::Metrics;
1515

1616
use crate::cluster::NodeAddr;
17+
use crate::utils::safe_format::IteratorSafeFormatExt;
1718

1819
use arc_swap::ArcSwap;
1920
use futures::{future::RemoteHandle, stream::FuturesUnordered, Future, FutureExt, StreamExt};
20-
use itertools::Itertools;
2121
use rand::Rng;
2222
use std::convert::TryInto;
2323
use std::num::NonZeroUsize;
@@ -397,7 +397,9 @@ impl NodeConnectionPool {
397397
fn choose_random_connection_from_slice(v: &[Arc<Connection>]) -> Option<Arc<Connection>> {
398398
trace!(
399399
connections = tracing::field::display(
400-
v.iter().map(|conn| conn.get_connect_address()).format(", ")
400+
v.iter()
401+
.map(|conn| conn.get_connect_address())
402+
.safe_format(", ")
401403
),
402404
"Available"
403405
);

‎scylla/src/observability/driver_tracing.rs‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,8 @@ use crate::cluster::node::Node;
22
use crate::network::Connection;
33
use crate::response::query_result::QueryResult;
44
use crate::routing::{Shard, Token};
5-
use itertools::{Either, Itertools};
5+
use crate::utils::safe_format::IteratorSafeFormatExt;
6+
use itertools::Either;
67
use scylla_cql::frame::response::result::ColumnSpec;
78
use scylla_cql::frame::response::result::RawMetadataAndRawRows;
89
use scylla_cql::value::deser_cql_value;
@@ -137,7 +138,7 @@ impl RequestSpan {
137138
tracing::field::display(
138139
replicas
139140
.map(|(node, shard)| Replica(node, shard))
140-
.format(", "),
141+
.safe_format(", "),
141142
),
142143
);
143144
}
@@ -176,5 +177,5 @@ fn partition_key_displayer<'ps, 'res, 'spec: 'ps>(
176177
Ok(c) => Either::Left(c),
177178
Err(_) => Either::Right("<decoding error>"),
178179
})
179-
.format(", ")
180+
.safe_format(", ")
180181
}

‎scylla/src/routing/locator/tablets.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,4 @@
11
use bytes::Bytes;
2-
use itertools::Itertools;
32
use scylla_cql::deserialize::value::{DeserializeValue, ListlikeIterator};
43
use scylla_cql::deserialize::{DeserializationError, FrameSlice, TypeCheckError};
54
use scylla_cql::frame::response::result::{CollectionType, ColumnType, NativeType, TableSpec};
@@ -9,6 +8,7 @@ use uuid::Uuid;
98

109
use crate::cluster::Node;
1110
use crate::routing::{Shard, Token};
11+
use crate::utils::safe_format::IteratorSafeFormatExt;
1212

1313
use std::collections::{HashMap, HashSet};
1414
use std::ops::Deref;
@@ -417,7 +417,7 @@ impl TableTablets {
417417
if let Err(failed) = &r {
418418
warn!("Nodes ({}) listed as replicas for a tablet {{ks: {}, table: {}, range: [{}. {}]}} are not present in ClusterState.known_peers, \
419419
despite topology refresh. Removing problematic tablet.",
420-
failed.iter().format(", "), self.table_spec.ks_name(), self.table_spec.table_name(), tablet.first_token.value(), tablet.last_token.value());
420+
failed.iter().safe_format(", "), self.table_spec.ks_name(), self.table_spec.table_name(), tablet.first_token.value(), tablet.last_token.value());
421421
}
422422

423423
r.is_ok()

0 commit comments

Comments
 (0)