Mirrors/servo
1
0
Fork 0
mirror of https://github.com/servo/servo.git synced 2025-07-22 23:03:42 +01:00
Code Issues Projects Releases Packages Wiki Activity

fix(bhm): deliver exit signal reliably when component registration and signal submission coincide

Browse source 
> There's a race condition between the reception of
> `BackgroundHangMonitorControlMsg::Exit` and `MonitoredComponentMsg::
> Register`. When the worker receives `Exit`, it stops receiving
> messages, and any remaining messages (including the
> `MonitoredComponentMsg::Register` we sent) in the channel are dropped.
> Wrapping `exit_signal` with this RAII wrapper ensures the exit signal
> is delivered even in such cases.

This should (hopefully) eliminate the intermittent hang-ups in the test
case `test_hang_monitoring_exit_signal` for good.
This commit is contained in:
yvt 2021-06-20 10:44:40 +09:00
parent 2eec1e69ea
commit 18c79cafac
2 changed files with 119 additions and 30 deletions
Download patch file Download diff file Expand all files Collapse all files

56
components/background_hang_monitor/background_hang_monitor.rs
View file

@ -44,6 +44,11 @@ impl HangMonitorRegister {
while monitor.run() {
// Monitoring until all senders have been dropped...
}
// Suppress `signal_to_exit`
for (_, mut component) in monitor.monitored_components.drain() {
component.exit_signal.release();
}
})
.expect("Couldn't start BHM worker.");
Box::new(HangMonitorRegister {
@ -85,6 +90,15 @@ impl BackgroundHangMonitorRegister for HangMonitorRegister {
#[cfg(any(target_os = "android", target_arch = "arm", target_arch = "aarch64"))]
let sampler = crate::sampler::DummySampler::new();
// There's a race condition between the reception of
// `BackgroundHangMonitorControlMsg::Exit` and
// `MonitoredComponentMsg::Register`. When the worker receives `Exit`,
// it stops receiving messages, and any remaining messages (including
// the `MonitoredComponentMsg::Register` we sent) in the channel are
// dropped. Wrapping `exit_signal` with this RAII wrapper ensures the
// exit signal is delivered even in such cases.
let exit_signal = SignalToExitOnDrop(exit_signal);
bhm_chan.send(MonitoredComponentMsg::Register(
sampler,
thread::current().name().map(str::to_owned),
@ -110,7 +124,7 @@ enum MonitoredComponentMsg {
Option<String>,
Duration,
Duration,
Option<Box<dyn BackgroundHangMonitorExitSignal>>,
SignalToExitOnDrop,
),
/// Unregister component for monitoring.
Unregister,
@ -174,6 +188,33 @@ impl BackgroundHangMonitor for BackgroundHangMonitorChan {
}
}
/// Wraps [`BackgroundHangMonitorExitSignal`] and calls `signal_to_exit` when
/// dropped.
struct SignalToExitOnDrop(Option<Box<dyn BackgroundHangMonitorExitSignal>>);
impl SignalToExitOnDrop {
/// Call `BackgroundHangMonitorExitSignal::signal_to_exit` now.
fn signal_to_exit(&mut self) {
if let Some(signal) = self.0.take() {
signal.signal_to_exit();
}
}
/// Disassociate `BackgroundHangMonitorExitSignal` from itself, preventing
/// `BackgroundHangMonitorExitSignal::signal_to_exit` from being called in
/// the future.
fn release(&mut self) {
self.0 = None;
}
}
impl Drop for SignalToExitOnDrop {
#[inline]
fn drop(&mut self) {
self.signal_to_exit();
}
}
struct MonitoredComponent {
sampler: Box<dyn Sampler>,
last_activity: Instant,
@ -183,7 +224,7 @@ struct MonitoredComponent {
sent_transient_alert: bool,
sent_permanent_alert: bool,
is_waiting: bool,
exit_signal: Option<Box<dyn BackgroundHangMonitorExitSignal>>,
exit_signal: SignalToExitOnDrop,
}
struct Sample(MonitoredComponentId, Instant, NativeStack);
@ -306,10 +347,8 @@ impl BackgroundHangMonitorWorker {
return true;
},
Ok(BackgroundHangMonitorControlMsg::Exit(sender)) => {
for component in self.monitored_components.values() {
if let Some(signal) = component.exit_signal.as_ref() {
signal.signal_to_exit();
}
for component in self.monitored_components.values_mut() {
component.exit_signal.signal_to_exit();
}
// Confirm exit with to the constellation.
@ -379,10 +418,13 @@ impl BackgroundHangMonitorWorker {
);
},
(component_id, MonitoredComponentMsg::Unregister) => {
let _ = self
let (_, mut component) = self
.monitored_components
.remove_entry(&component_id)
.expect("Received Unregister for an unknown component");
// Prevent `signal_to_exit` from being called
component.exit_signal.release();
},
(component_id, MonitoredComponentMsg::NotifyActivity(annotation)) => {
let component = self

93
components/background_hang_monitor/tests/hang_monitor_tests.rs
View file

@ -164,10 +164,45 @@ fn test_hang_monitoring_unregister() {
assert!(background_hang_monitor_receiver.try_recv().is_err());
}
// Perform two certain steps in `test_hang_monitoring_exit_signal_inner` in
// different orders to check for the race condition that
// caused <https://github.com/servo/servo/issues/28270> and
// <https://github.com/servo/servo/issues/27191>.
#[test]
// https://github.com/servo/servo/issues/28270
#[cfg(not(any(target_os = "windows", target_os = "macos")))]
fn test_hang_monitoring_exit_signal() {
fn test_hang_monitoring_exit_signal1() {
test_hang_monitoring_exit_signal_inner(|e1, e2| {
e1();
thread::sleep(Duration::from_millis(100));
e2();
});
}
#[test]
fn test_hang_monitoring_exit_signal2() {
test_hang_monitoring_exit_signal_inner(|e1, e2| {
e1();
e2();
});
}
#[test]
fn test_hang_monitoring_exit_signal3() {
test_hang_monitoring_exit_signal_inner(|e1, e2| {
e2();
e1();
});
}
#[test]
fn test_hang_monitoring_exit_signal4() {
test_hang_monitoring_exit_signal_inner(|e1, e2| {
e2();
thread::sleep(Duration::from_millis(100));
e1();
});
}
fn test_hang_monitoring_exit_signal_inner(op_order: fn(&mut dyn FnMut(), &mut dyn FnMut())) {
let _lock = SERIAL.lock().unwrap();
let (background_hang_monitor_ipc_sender, _background_hang_monitor_receiver) =
@ -185,9 +220,9 @@ fn test_hang_monitoring_exit_signal() {
}
let closing = Arc::new(AtomicBool::new(false));
let signal = BHMExitSignal {
let mut signal = Some(Box::new(BHMExitSignal {
closing: closing.clone(),
};
}));
// Init a worker, without active monitoring.
let background_hang_monitor_register = HangMonitorRegister::init(
@ -195,26 +230,38 @@ fn test_hang_monitoring_exit_signal() {
control_receiver,
false,
);
let _background_hang_monitor = background_hang_monitor_register.register_component(
MonitoredComponentId(TEST_PIPELINE_ID, MonitoredComponentType::Script),
Duration::from_millis(10),
Duration::from_millis(1000),
Some(Box::new(signal)),
let mut background_hang_monitor = None;
let (exit_sender, exit_receiver) = ipc::channel().expect("Failed to create IPC channel!");
let mut exit_sender = Some(exit_sender);
// `op_order` determines the order in which these two closures are
// executed.
op_order(
&mut || {
// Register a component.
background_hang_monitor = Some(background_hang_monitor_register.register_component(
MonitoredComponentId(TEST_PIPELINE_ID, MonitoredComponentType::Script),
Duration::from_millis(10),
Duration::from_millis(1000),
Some(signal.take().unwrap()),
));
},
&mut || {
// Send the exit message.
control_sender
.send(BackgroundHangMonitorControlMsg::Exit(
exit_sender.take().unwrap(),
))
.unwrap();
},
);
let (exit_sender, exit_receiver) = ipc::channel().expect("Failed to create IPC channel!");
// Assert we receive a confirmation back.
assert!(exit_receiver.recv().is_ok());
// Send the exit message.
if control_sender
.send(BackgroundHangMonitorControlMsg::Exit(exit_sender))
.is_ok()
{
// Assert we receive a confirmation back.
assert!(exit_receiver.recv().is_ok());
// Assert we get the exit signal.
while !closing.load(Ordering::SeqCst) {
thread::sleep(Duration::from_millis(10));
}
// Assert we get the exit signal.
while !closing.load(Ordering::SeqCst) {
thread::sleep(Duration::from_millis(10));
}
}