Runtime diagnostics for leaked messages in unbounded channels (part 2) #13020
Conversation
dmitry-markin
added
A0-please_review
| @@ -62,7 +62,7 @@ pub fn channel(name: &'static str, queue_size_warning: i64) -> (Sender, Receiver | |||
| queue_size: queue_size.clone(), | |||
| queue_size_warning, | |||
| warning_fired: false, | |||
| creation_backtrace: Backtrace::capture(), | |||
| creation_backtrace: Backtrace::new_unresolved(), | |||
There was a problem hiding this comment.
Can we not use the channel from utils here?
The metrics field is always None or?
There was a problem hiding this comment.
Sender is not accessible on its own, but only via OutChannels. metrics is inserted when a new Sender is added with OutChannels::push():
substrate/client/network/src/service/out_events.rs
Lines 176 to 180 in 80082f1
There was a problem hiding this comment.
I've updated the comment to indicate that metrics will be initialized.
There was a problem hiding this comment.
Metrics in out_events.rs and mpsc.rs seem different, so I don't know how to reuse utils version of the channel.
There was a problem hiding this comment.
Okay ty. At some point we should probably merge both implementations. The metrics are probably not that different.
client/utils/src/mpsc.rs
Outdated
| self.name, self.queue_size_warning, | ||
| ), | ||
| } | ||
| let mut bt = self.creation_backtrace.lock().expect("another thread panicked."); |
There was a problem hiding this comment.
I'm not really sure about the mutex here. I mean I get why, but we could also just use Arc and then clone the backtrace here into some mutable value to resolve it.
let mut backtrace = (*self.creation_backtrace).clone();
backtrace.resolve();
But I don't know if not using a mutex is wort the clone 🤷
There was a problem hiding this comment.
Done. It's a rare one-time event anyway, not a big deal.
dmitry-markin
force-pushed
the
dm-unbounded-warning-folow-up
branch
from
e4394c0 to
f7276ed
Compare
| @@ -62,7 +62,7 @@ pub fn channel(name: &'static str, queue_size_warning: i64) -> (Sender, Receiver | |||
| queue_size: queue_size.clone(), | |||
| queue_size_warning, | |||
| warning_fired: false, | |||
| creation_backtrace: Backtrace::capture(), | |||
| creation_backtrace: Backtrace::new_unresolved(), | |||
There was a problem hiding this comment.
Okay ty. At some point we should probably merge both implementations. The metrics are probably not that different.
|
bot merge |
|
Waiting for commit status. |
#13020) * Fix code review issues * Clarify doc * Get rid of backtrace mutex * kick CI
paritytech#13020) * Fix code review issues * Clarify doc * Get rid of backtrace mutex * kick CI
paritytech#13020) * Fix code review issues * Clarify doc * Get rid of backtrace mutex * kick CI
This is a follow-up PR to #12971 addressing code review issues.
polkadot companion: paritytech/polkadot#6481