Skip to content
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

Unregister components while exiting #22474

Merged
merged 1 commit into from Dec 18, 2018
Merged
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file
Failed to load files.

Always

Just for now

@@ -72,6 +72,8 @@ impl BackgroundHangMonitorClone for HangMonitorRegister {
pub enum MonitoredComponentMsg {
/// Register component for monitoring,
Register(Box<Sampler>, Duration, Duration),
/// Unregister component for monitoring.
Unregister,
/// Notify start of new activity for a given component,
NotifyActivity(HangAnnotation),
/// Notify start of waiting for a new task to come-in for processing.
@@ -119,6 +121,10 @@ impl BackgroundHangMonitor for BackgroundHangMonitorChan {
let msg = MonitoredComponentMsg::NotifyWait;
self.send(msg);
}
fn unregister(&self) {
let msg = MonitoredComponentMsg::Unregister;
self.send(msg);
}
}

struct MonitoredComponent {
@@ -200,6 +206,12 @@ impl BackgroundHangMonitorWorker {
"This component was already registered for monitoring."
);
},
(component_id, MonitoredComponentMsg::Unregister) => {
let _ = self
.monitored_components
.remove_entry(&component_id)
.expect("Received Unregister for an unknown component");
},
(component_id, MonitoredComponentMsg::NotifyActivity(annotation)) => {
let component = self
.monitored_components
@@ -105,3 +105,30 @@ fn test_hang_monitoring() {
// Still no new alerts because the hang monitor has shut-down already.
assert!(background_hang_monitor_receiver.try_recv().is_err());
}

#[test]
fn test_hang_monitoring_unregister() {
let (background_hang_monitor_ipc_sender, background_hang_monitor_receiver) =
ipc::channel().expect("ipc channel failure");

let background_hang_monitor_register =
HangMonitorRegister::init(background_hang_monitor_ipc_sender.clone());
let background_hang_monitor = background_hang_monitor_register.register_component(
MonitoredComponentId(TEST_PIPELINE_ID, MonitoredComponentType::Script),
Duration::from_millis(10),
Duration::from_millis(1000),
);

// Start an activity.
let hang_annotation = HangAnnotation::Script(ScriptHangAnnotation::AttachLayout);
background_hang_monitor.notify_activity(hang_annotation);

// Unregister the component.
background_hang_monitor.unregister();
This conversation was marked as resolved by csmoe

This comment has been minimized.

Copy link
@gterzian

gterzian Dec 16, 2018

Member

Let's put the sleep after the call to unregister, I'm afraid that this might fail intermittently if the monitor sends an alert just before the call to unregister comes through.


// Sleep until the "transient" timeout has been reached.
thread::sleep(Duration::from_millis(10));

// No new alert yet
assert!(background_hang_monitor_receiver.try_recv().is_err());
}
@@ -926,6 +926,7 @@ impl LayoutThread {
self.root_flow.borrow_mut().take();
// Drop the rayon threadpool if present.
let _ = self.parallel_traversal.take();
self.background_hang_monitor.unregister();
}

fn handle_add_stylesheet(&self, stylesheet: &Stylesheet, guard: &SharedRwLockReadGuard) {
@@ -469,4 +469,6 @@ pub trait BackgroundHangMonitor {
fn notify_activity(&self, annotation: HangAnnotation);
/// Notify the start of waiting for a new event to come in.
fn notify_wait(&self);
/// Unregister the component from monitor.
fn unregister(&self);
}
@@ -2379,6 +2379,8 @@ impl ScriptThread {
self.handle_exit_pipeline_msg(pipeline_id, DiscardBrowsingContext::Yes);
}

self.background_hang_monitor.unregister();

debug!("Exited script thread.");
}

ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.