From 7a9ff485698894b49aebadbc6f66f0c14cedc934 Mon Sep 17 00:00:00 2001 From: Gary Bressler Date: Thu, 15 Apr 2021 00:15:37 +0000 Subject: [PATCH] [component_manager] Fix destroy/create race This fixes a bug in component manager that can cause a created component to be incorrectly deleted if it was created just after another component with the same name was deleted. To be precise, the race is as follows: 1) Client calls DestroyChild. 2) component_manager schedules a DeleteChild action 3) DestroyChild returns. 4) Client calls CreateChild, with the same instance name. 5) Instance is created. 6) The DeleteChild action from #2 executes. It starts by scheduling a MarkDeleted action, which incorrectly marks the new instance deleted. The solution is for DeleteChild not to call MarkDeleted. Instead, code that handles instance destruction must explicitly call MarkDeleted first. This bug was causing `ffx session restart` to sometimes error with CreateComponentFailed (which actually was an error of failing to bind to the session component). Tested: Manually verified the fix works with `session_control`. Added an integration test which should flake if the issue is present. Unit test checks that MarkDeleted is not rescheduled. Multiply: destruction_integration_test: 200 Fixed: 41967 Change-Id: I1f19811b0afff0533a2721b45fe7cc07bde072ce Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/511284 Commit-Queue: Gary Bressler Reviewed-by: Fady Samuel --- .../src/builtin_environment.rs | 51 ++++++-- src/sys/component_manager/src/framework.rs | 28 ++-- .../src/model/actions/delete_child.rs | 6 +- .../src/model/actions/destroy.rs | 56 ++++++-- .../src/model/actions/mark_deleted.rs | 35 +++-- .../src/model/actions/mod.rs | 21 ++- .../component_manager/src/model/component.rs | 10 +- .../src/model/testing/test_helpers.rs | 15 ++- .../src/model/tests/routing.rs | 7 +- .../tests/destruction/BUILD.gn | 28 ++++ .../tests/destruction/collection_realm.rs | 2 +- .../tests/destruction/destroy_and_recreate.rs | 122 ++++++++++++++++++ .../tests/destruction/integration_test.rs | 15 ++- .../destruction/meta/destroy_and_recreate.cml | 19 +++ .../tests/destruction/meta/trigger_realm.cml | 10 ++ 15 files changed, 345 insertions(+), 80 deletions(-) create mode 100644 src/sys/component_manager/tests/destruction/destroy_and_recreate.rs create mode 100644 src/sys/component_manager/tests/destruction/meta/destroy_and_recreate.cml diff --git a/src/sys/component_manager/src/builtin_environment.rs b/src/sys/component_manager/src/builtin_environment.rs index 95c92cb878dd..8dbb93697014 100644 --- a/src/sys/component_manager/src/builtin_environment.rs +++ b/src/sys/component_manager/src/builtin_environment.rs @@ -51,7 +51,7 @@ use { root_stop_notifier::RootStopNotifier, work_scheduler::WorkScheduler, }, - anyhow::{format_err, Context as _, Error}, + anyhow::{bail, format_err, Context as _, Error}, cm_rust::{CapabilityName, RunnerRegistration}, cm_types::Url, fidl::endpoints::{create_endpoints, create_proxy, ServerEnd, ServiceMarker}, @@ -70,6 +70,7 @@ use { futures::{channel::oneshot, prelude::*}, log::*, std::{ + default, path::PathBuf, sync::{Arc, Weak}, }, @@ -78,7 +79,6 @@ use { // Allow shutdown to take up to an hour. pub static SHUTDOWN_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(60 * 60); -#[derive(Default)] pub struct BuiltinEnvironmentBuilder { // TODO(60804): Make component manager's namespace injectable here. runtime_config: Option, @@ -87,6 +87,21 @@ pub struct BuiltinEnvironmentBuilder { utc_clock: Option>, add_environment_resolvers: bool, inspector: Option, + enable_hub: bool, +} + +impl default::Default for BuiltinEnvironmentBuilder { + fn default() -> Self { + Self { + runtime_config: None, + runners: vec![], + resolvers: ResolverRegistry::default(), + utc_clock: None, + add_environment_resolvers: false, + inspector: None, + enable_hub: true, + } + } } impl BuiltinEnvironmentBuilder { @@ -108,6 +123,11 @@ impl BuiltinEnvironmentBuilder { self } + pub fn enable_hub(mut self, val: bool) -> Self { + self.enable_hub = val; + self + } + /// Create a UTC clock if required. /// Not every instance of component_manager running on the system maintains a /// UTC clock. Only the root component_manager should have the `maintain-utc-clock` @@ -247,6 +267,7 @@ impl BuiltinEnvironmentBuilder { builtin_runners, self.utc_clock, self.inspector.unwrap_or(component::inspector().clone()), + self.enable_hub, ) .await?) } @@ -333,7 +354,7 @@ pub struct BuiltinEnvironment { pub work_scheduler: Arc, pub realm_capability_host: Arc, pub storage_admin_capability_host: Arc, - pub hub: Arc, + pub hub: Option>, pub builtin_runners: Vec>, pub event_registry: Arc, pub event_source_factory: Arc, @@ -358,6 +379,7 @@ impl BuiltinEnvironment { builtin_runners: Vec>, utc_clock: Option>, inspector: Inspector, + enable_hub: bool, ) -> Result { let execution_mode = match runtime_config.debug { true => ExecutionMode::Debug, @@ -617,8 +639,13 @@ impl BuiltinEnvironment { let stop_notifier = Arc::new(RootStopNotifier::new()); model.root.hooks.install(stop_notifier.hooks()).await; - let hub = Arc::new(Hub::new(root_component_url.as_str().to_owned())?); - model.root.hooks.install(hub.hooks()).await; + let hub = if enable_hub { + let hub = Arc::new(Hub::new(root_component_url.as_str().to_owned())?); + model.root.hooks.install(hub.hooks()).await; + Some(hub) + } else { + None + }; // Set up the Component Tree Diagnostics runtime statistics. let component_tree_stats = @@ -714,13 +741,19 @@ impl BuiltinEnvironment { } /// Setup a ServiceFs that contains the Hub and (optionally) the `EventSource` service. - async fn create_service_fs<'a>(&self) -> Result>, ModelError> { + async fn create_service_fs<'a>(&self) -> Result>, Error> { + if let None = self.hub { + bail!("Hub must be enabled if OutDirContents is not `None`"); + } + // Create the ServiceFs let mut service_fs = ServiceFs::new(); // Setup the hub let (hub_proxy, hub_server_end) = create_proxy::().unwrap(); self.hub + .as_ref() + .unwrap() .open_root(OPEN_RIGHT_READABLE | OPEN_RIGHT_WRITABLE, hub_server_end.into_channel()) .await?; service_fs.add_remote("hub", hub_proxy); @@ -750,7 +783,7 @@ impl BuiltinEnvironment { } /// Bind ServiceFs to a provided channel - async fn bind_service_fs(&mut self, channel: zx::Channel) -> Result<(), ModelError> { + async fn bind_service_fs(&mut self, channel: zx::Channel) -> Result<(), Error> { let mut service_fs = self.create_service_fs().await?; // Bind to the channel @@ -770,7 +803,7 @@ impl BuiltinEnvironment { } /// Bind ServiceFs to the outgoing directory of this component, if it exists. - pub async fn bind_service_fs_to_out(&mut self) -> Result<(), ModelError> { + pub async fn bind_service_fs_to_out(&mut self) -> Result<(), Error> { if let Some(handle) = fuchsia_runtime::take_startup_handle( fuchsia_runtime::HandleType::DirectoryRequest.into(), ) { @@ -786,7 +819,7 @@ impl BuiltinEnvironment { /// Bind ServiceFs to a new channel and return the Hub directory. /// Used mainly by integration tests. - pub async fn bind_service_fs_for_hub(&mut self) -> Result { + pub async fn bind_service_fs_for_hub(&mut self) -> Result { // Create a channel that ServiceFs will operate on let (service_fs_proxy, service_fs_server_end) = create_proxy::().unwrap(); diff --git a/src/sys/component_manager/src/framework.rs b/src/sys/component_manager/src/framework.rs index affe4ac76767..2d6420b2e90f 100644 --- a/src/sys/component_manager/src/framework.rs +++ b/src/sys/component_manager/src/framework.rs @@ -73,7 +73,7 @@ impl CapabilityProvider for RealmCapabilityProvider { Ok(fasync::Task::spawn(async move { if let Err(e) = host.serve(component, stream).await { // TODO: Set an epitaph to indicate this was an unexpected error. - warn!("serve failed: {:?}", e); + warn!("serve failed: {}", e); } }) .into()) @@ -110,7 +110,7 @@ impl RealmCapabilityHost { let method_name = request.method_name(); let res = self.handle_request(request, &component).await; if let Err(e) = &res { - error!("Error occurred sending Realm response for {}: {:?}", method_name, e); + error!("Error occurred sending Realm response for {}: {}", method_name, e); } res }) @@ -167,10 +167,7 @@ impl RealmCapabilityHost { ModelError::InstanceAlreadyExists { .. } => fcomponent::Error::InstanceAlreadyExists, ModelError::CollectionNotFound { .. } => fcomponent::Error::CollectionNotFound, ModelError::Unsupported { .. } => fcomponent::Error::Unsupported, - e => { - error!("add_dynamic_child() failed: {:?}", e); - fcomponent::Error::Internal - } + _ => fcomponent::Error::Internal, }) } @@ -183,8 +180,8 @@ impl RealmCapabilityHost { let partial_moniker = PartialMoniker::new(child.name, child.collection); let child = { let state = component.lock_resolved_state().await.map_err(|e| match e { - ComponentInstanceError::ResolveFailed { moniker, err } => { - debug!("failed to resolve instance with moniker {}: {:?}", moniker, err); + ComponentInstanceError::ResolveFailed { moniker, err, .. } => { + debug!("failed to resolve instance with moniker {}: {}", moniker, err); fcomponent::Error::InstanceCannotResolve } e => { @@ -201,15 +198,15 @@ impl RealmCapabilityHost { .await .map_err(|e| match e { ModelError::ResolverError { err, .. } => { - debug!("failed to resolve child: {:?}", err); + debug!("failed to resolve child: {}", err); fcomponent::Error::InstanceCannotResolve } ModelError::RunnerError { err } => { - debug!("failed to start child: {:?}", err); + debug!("failed to start child: {}", err); fcomponent::Error::InstanceCannotStart } e => { - error!("bind() failed: {:?}", e); + error!("bind() failed: {}", e); fcomponent::Error::Internal } })? @@ -231,11 +228,12 @@ impl RealmCapabilityHost { () } Err(e) => { - error!("open_exposed() failed: {:?}", e); + debug!("open_exposed() failed: {}", e); return Err(fcomponent::Error::Internal); } } } else { + debug!("bind_child() failed: instance not found {:?}", child); return Err(fcomponent::Error::InstanceNotFound); } Ok(()) @@ -253,7 +251,7 @@ impl RealmCapabilityHost { ModelError::InstanceNotFoundInRealm { .. } => fcomponent::Error::InstanceNotFound, ModelError::Unsupported { .. } => fcomponent::Error::Unsupported, e => { - error!("remove_dynamic_child() failed: {:?}", e); + error!("remove_dynamic_child() failed: {}", e); fcomponent::Error::Internal } })?; @@ -274,7 +272,7 @@ impl RealmCapabilityHost { ) -> Result<(), fcomponent::Error> { let component = component.upgrade().map_err(|_| fcomponent::Error::InstanceDied)?; let state = component.lock_resolved_state().await.map_err(|e| { - error!("failed to resolve InstanceState: {:?}", e); + error!("failed to resolve InstanceState: {}", e); fcomponent::Error::Internal })?; let decl = state.decl(); @@ -312,7 +310,7 @@ impl RealmCapabilityHost { fasync::Task::spawn(async move { if let Err(e) = Self::serve_child_iterator(children, stream, batch_size).await { // TODO: Set an epitaph to indicate this was an unexpected error. - warn!("serve_child_iterator failed: {:?}", e); + warn!("serve_child_iterator failed: {}", e); } }) .detach(); diff --git a/src/sys/component_manager/src/model/actions/delete_child.rs b/src/sys/component_manager/src/model/actions/delete_child.rs index fa9c8036fd8f..8b5d389095a9 100644 --- a/src/sys/component_manager/src/model/actions/delete_child.rs +++ b/src/sys/component_manager/src/model/actions/delete_child.rs @@ -4,7 +4,7 @@ use { crate::model::{ - actions::{Action, ActionKey, ActionSet, DestroyAction, MarkDeletedAction}, + actions::{Action, ActionKey, ActionSet, DestroyAction}, component::{ComponentInstance, InstanceState}, error::ModelError, hooks::{Event, EventPayload}, @@ -40,10 +40,6 @@ async fn do_delete_child( component: &Arc, moniker: ChildMoniker, ) -> Result<(), ModelError> { - // Some paths may have already marked the child deleting before scheduling the DeleteChild - // action, in which case this is a no-op. - ActionSet::register(component.clone(), MarkDeletedAction::new(moniker.clone())).await?; - // The child may not exist or may already be deleted by a previous DeleteChild action. let child = { let state = component.lock_state().await; diff --git a/src/sys/component_manager/src/model/actions/destroy.rs b/src/sys/component_manager/src/model/actions/destroy.rs index 01a2e2ff2f95..38b0f9934b70 100644 --- a/src/sys/component_manager/src/model/actions/destroy.rs +++ b/src/sys/component_manager/src/model/actions/destroy.rs @@ -5,8 +5,8 @@ use { crate::model::{ actions::{ - Action, ActionKey, ActionSet, DeleteChildAction, DiscoverAction, ResolveAction, - StartAction, + Action, ActionKey, ActionSet, DeleteChildAction, DiscoverAction, MarkDeletedAction, + ResolveAction, StartAction, }, component::{BindReason, ComponentInstance, InstanceState}, error::ModelError, @@ -49,13 +49,22 @@ async fn do_destroy(component: &Arc) -> Result<(), ModelError component.abs_moniker ); } + let nfs = { match *component.lock_state().await { InstanceState::Resolved(ref s) => { let mut nfs = vec![]; for (m, _) in s.all_children().iter() { let component = component.clone(); - let nf = ActionSet::register(component, DeleteChildAction::new(m.clone())); + let m = m.clone(); + let nf = async move { + ActionSet::register( + component.clone(), + MarkDeletedAction::new(m.to_partial()), + ) + .await?; + ActionSet::register(component, DeleteChildAction::new(m)).await + }; nfs.push(nf); } nfs @@ -109,7 +118,7 @@ pub mod tests { super::*, crate::model::{ actions::{ - test_utils::{is_child_deleted, is_destroyed, is_executing}, + test_utils::{is_child_deleted, is_destroyed, is_executing, is_marked_deleted}, ActionNotifier, ShutdownAction, }, binding::Binder, @@ -149,10 +158,17 @@ pub mod tests { .expect("could not bind to a"); assert!(is_executing(&component_a).await); + // Register shutdown first because DeleteChild requires the component to be shut down. + ActionSet::register(component_a.clone(), ShutdownAction::new()) + .await + .expect("shutdown failed"); // Register delete child action, and wait for it. Component should be destroyed. ActionSet::register(component_root.clone(), DeleteChildAction::new("a:0".into())) .await .expect("destroy failed"); + // DeleteChild should not mark the instance non-live. That's done by MarkDeleted which we + // don't call here. + assert!(!is_marked_deleted(&component_root, &"a:0".into()).await); assert!(is_child_deleted(&component_root, &component_a).await); { let events: Vec<_> = test @@ -180,6 +196,7 @@ pub mod tests { ActionSet::register(component_root.clone(), DeleteChildAction::new("a:0".into())) .await .expect("destroy failed"); + assert!(!is_marked_deleted(&component_root, &"a:0".into()).await); assert!(is_child_deleted(&component_root, &component_a).await); } @@ -221,6 +238,9 @@ pub mod tests { // Register delete child action, and wait for it. Components should be destroyed. let component_container = test.look_up(vec!["container:0"].into()).await; + ActionSet::register(component_container.clone(), ShutdownAction::new()) + .await + .expect("shutdown failed"); ActionSet::register(component_root.clone(), DeleteChildAction::new("container:0".into())) .await .expect("destroy failed"); @@ -323,9 +343,19 @@ pub mod tests { // Register delete child action, while `action` is stalled. let component_root = test.look_up(vec![].into()).await; + let component_a = match *component_root.lock_state().await { + InstanceState::Resolved(ref s) => { + s.get_live_child(&PartialMoniker::from("a")).expect("child a not found") + } + _ => panic!("not resolved"), + }; let (f, delete_handle) = { let component_root = component_root.clone(); + let component_a = component_a.clone(); async move { + ActionSet::register(component_a, ShutdownAction::new()) + .await + .expect("shutdown failed"); ActionSet::register(component_root, DeleteChildAction::new("a:0".into())) .await .expect("destroy failed"); @@ -335,12 +365,6 @@ pub mod tests { fasync::Task::spawn(f).detach(); // Check that `action` is being waited on. - let component_a = match *component_root.lock_state().await { - InstanceState::Resolved(ref s) => { - s.get_live_child(&PartialMoniker::from("a")).expect("child a not found") - } - _ => panic!("not resolved"), - }; let action_key = action.key(); loop { let actions = component_a.lock_actions().await; @@ -458,6 +482,9 @@ pub mod tests { }; // Register delete action on "a", and wait for it. + ActionSet::register(component_a.clone(), ShutdownAction::new()) + .await + .expect("shutdown failed"); ActionSet::register(component_root.clone(), DeleteChildAction::new("a:0".into())) .await .expect("destroy failed"); @@ -530,6 +557,9 @@ pub mod tests { // Register destroy action on "a", and wait for it. This should cause all components // in "a"'s component to be shut down and destroyed, in bottom-up order, but "x" is still // running. + ActionSet::register(component_a.clone(), ShutdownAction::new()) + .await + .expect("shutdown failed"); ActionSet::register(component_root.clone(), DeleteChildAction::new("a:0".into())) .await .expect("delete child failed"); @@ -644,6 +674,9 @@ pub mod tests { // Register destroy action on "a", and wait for it. This should cause all components // that were started to be destroyed, in bottom-up order. + ActionSet::register(component_a.clone(), ShutdownAction::new()) + .await + .expect("shutdown failed"); ActionSet::register(component_root.clone(), DeleteChildAction::new("a:0".into())) .await .expect("delete child failed"); @@ -770,6 +803,9 @@ pub mod tests { // Register delete action on "a", and wait for it. "b"'s component is deleted, but "b" // returns an error so the delete action on "a" does not succeed. + ActionSet::register(component_a.clone(), ShutdownAction::new()) + .await + .expect("shutdown failed"); ActionSet::register(component_root.clone(), DeleteChildAction::new("a:0".into())) .await .expect_err("destroy succeeded unexpectedly"); diff --git a/src/sys/component_manager/src/model/actions/mark_deleted.rs b/src/sys/component_manager/src/model/actions/mark_deleted.rs index e02e7dda5a51..e28fa66a059b 100644 --- a/src/sys/component_manager/src/model/actions/mark_deleted.rs +++ b/src/sys/component_manager/src/model/actions/mark_deleted.rs @@ -10,17 +10,17 @@ use { hooks::{Event, EventPayload}, }, async_trait::async_trait, - moniker::ChildMoniker, + moniker::PartialMoniker, std::sync::Arc, }; /// Marks a child of a component as deleted, after shutting it down. pub struct MarkDeletedAction { - moniker: ChildMoniker, + moniker: PartialMoniker, } impl MarkDeletedAction { - pub fn new(moniker: ChildMoniker) -> Self { + pub fn new(moniker: PartialMoniker) -> Self { Self { moniker } } } @@ -38,13 +38,12 @@ impl Action for MarkDeletedAction { async fn do_mark_deleted( component: &Arc, - moniker: ChildMoniker, + moniker: PartialMoniker, ) -> Result<(), ModelError> { - let partial_moniker = moniker.to_partial(); let child = { let state = component.lock_state().await; match *state { - InstanceState::Resolved(ref s) => s.get_live_child(&partial_moniker).map(|r| r.clone()), + InstanceState::Resolved(ref s) => s.get_live_child(&moniker).map(|r| r.clone()), InstanceState::Destroyed => None, InstanceState::New | InstanceState::Discovered => { panic!("do_mark_deleted: not resolved"); @@ -60,7 +59,7 @@ async fn do_mark_deleted( let mut state = component.lock_state().await; match *state { InstanceState::Resolved(ref mut s) => { - s.mark_child_deleted(&partial_moniker); + s.mark_child_deleted(&moniker); } InstanceState::Destroyed => {} InstanceState::New | InstanceState::Discovered => { @@ -78,7 +77,7 @@ pub mod tests { use { super::*, crate::model::{ - actions::{test_utils::is_deleting, ActionSet}, + actions::{test_utils::is_marked_deleted, ActionSet}, testing::{ test_helpers::{ component_decl_with_test_runner, ActionsTest, ComponentDeclBuilder, @@ -101,10 +100,10 @@ pub mod tests { // Register `mark_deleted` action, and wait for it. Component should be marked deleted. let component_root = test.look_up(vec![].into()).await; - ActionSet::register(component_root.clone(), MarkDeletedAction::new("a:0".into())) + ActionSet::register(component_root.clone(), MarkDeletedAction::new("a".into())) .await .expect("mark delete failed"); - assert!(is_deleting(&component_root, "a:0".into()).await); + assert!(is_marked_deleted(&component_root, &"a:0".into()).await); { let events: Vec<_> = test .test_hook @@ -125,10 +124,10 @@ pub mod tests { } // Execute action again, same state and no new events. - ActionSet::register(component_root.clone(), MarkDeletedAction::new("a:0".into())) + ActionSet::register(component_root.clone(), MarkDeletedAction::new("a".into())) .await .expect("mark delete failed"); - assert!(is_deleting(&component_root, "a:0".into()).await); + assert!(is_marked_deleted(&component_root, &"a:0".into()).await); { let events: Vec<_> = test .test_hook @@ -167,18 +166,18 @@ pub mod tests { // Register `mark_deleted` action for "a" only. let component_root = test.look_up(vec![].into()).await; - ActionSet::register(component_root.clone(), MarkDeletedAction::new("coll:a:1".into())) + ActionSet::register(component_root.clone(), MarkDeletedAction::new("coll:a".into())) .await .expect("mark delete failed"); - assert!(is_deleting(&component_root, "coll:a:1".into()).await); - assert!(!is_deleting(&component_root, "coll:b:2".into()).await); + assert!(is_marked_deleted(&component_root, &"coll:a:1".into()).await); + assert!(!is_marked_deleted(&component_root, &"coll:b:2".into()).await); // Register `mark_deleted` action for "b". - ActionSet::register(component_root.clone(), MarkDeletedAction::new("coll:b:1".into())) + ActionSet::register(component_root.clone(), MarkDeletedAction::new("coll:b".into())) .await .expect("mark delete failed"); - assert!(is_deleting(&component_root, "coll:a:1".into()).await); - assert!(is_deleting(&component_root, "coll:b:2".into()).await); + assert!(is_marked_deleted(&component_root, &"coll:a:1".into()).await); + assert!(is_marked_deleted(&component_root, &"coll:b:2".into()).await); { let events: Vec<_> = test .test_hook diff --git a/src/sys/component_manager/src/model/actions/mod.rs b/src/sys/component_manager/src/model/actions/mod.rs index a309aab2261f..610c5740e083 100644 --- a/src/sys/component_manager/src/model/actions/mod.rs +++ b/src/sys/component_manager/src/model/actions/mod.rs @@ -77,7 +77,7 @@ use { task::{Context, Poll}, Future, }, - moniker::ChildMoniker, + moniker::{ChildMoniker, PartialMoniker}, std::any::Any, std::collections::HashMap, std::fmt::Debug, @@ -105,7 +105,7 @@ pub enum ActionKey { Start, Stop, Shutdown, - MarkDeleted(ChildMoniker), + MarkDeleted(PartialMoniker), DeleteChild(ChildMoniker), Destroy, } @@ -409,10 +409,10 @@ pub(crate) mod test_utils { component.lock_execution().await.runtime.is_some() } - pub async fn is_deleting(component: &ComponentInstance, moniker: ChildMoniker) -> bool { + pub async fn is_marked_deleted(component: &ComponentInstance, moniker: &ChildMoniker) -> bool { let partial = moniker.to_partial(); match *component.lock_state().await { - InstanceState::Resolved(ref s) => match s.get_child(&moniker) { + InstanceState::Resolved(ref s) => match s.get_child(moniker) { Some(child) => { let child_execution = child.lock_execution().await; s.get_live_child(&partial).is_none() && child_execution.is_shut_down() @@ -426,11 +426,11 @@ pub(crate) mod test_utils { } } - /// Verifies that a child component is deleted by checking its InstanceState and verifying that it - /// does not exist in the InstanceState of its parent. Assumes the parent is not destroyed yet. + /// Verifies that a child component is deleted by checking its InstanceState and verifying that + /// it does not exist in the InstanceState of its parent. Assumes the parent is not destroyed + /// yet. pub async fn is_child_deleted(parent: &ComponentInstance, child: &ComponentInstance) -> bool { let child_moniker = child.abs_moniker.leaf().expect("Root component cannot be destroyed"); - let partial_moniker = child_moniker.to_partial(); // Verify the parent-child relationship assert_eq!(parent.abs_moniker.child(child_moniker.clone()), child.abs_moniker); @@ -443,14 +443,9 @@ pub(crate) mod test_utils { let child_state = child.lock_state().await; let child_execution = child.lock_execution().await; - - let found_partial_moniker = parent_resolved_state - .live_children() - .find(|(curr_partial_moniker, _)| **curr_partial_moniker == partial_moniker); let found_child_moniker = parent_resolved_state.all_children().get(child_moniker); - found_partial_moniker.is_none() - && found_child_moniker.is_none() + found_child_moniker.is_none() && matches!(*child_state, InstanceState::Destroyed) && child_execution.runtime.is_none() && child_execution.is_shut_down() diff --git a/src/sys/component_manager/src/model/component.rs b/src/sys/component_manager/src/model/component.rs index c3e63d4a4cbc..47b705c46daf 100644 --- a/src/sys/component_manager/src/model/component.rs +++ b/src/sys/component_manager/src/model/component.rs @@ -443,10 +443,11 @@ impl ComponentInstance { if let Some(tup) = tup { let (instance, _) = tup; let child_moniker = ChildMoniker::from_partial(partial_moniker, instance); - ActionSet::register(self.clone(), MarkDeletedAction::new(child_moniker.clone())) + ActionSet::register(self.clone(), MarkDeletedAction::new(partial_moniker.clone())) .await?; - let fut = ActionSet::register(self.clone(), DeleteChildAction::new(child_moniker)); - Ok(fut) + let mut actions = self.lock_actions().await; + let nf = actions.register_no_wait(self, DeleteChildAction::new(child_moniker)); + Ok(nf) } else { Err(ModelError::instance_not_found_in_realm( self.abs_moniker.clone(), @@ -562,7 +563,8 @@ impl ComponentInstance { // above. if let Some(coll) = m.collection() { if transient_colls.contains(coll) { - ActionSet::register(self.clone(), MarkDeletedAction::new(m.clone())).await?; + ActionSet::register(self.clone(), MarkDeletedAction::new(m.to_partial())) + .await?; let nf = ActionSet::register(self.clone(), DeleteChildAction::new(m)); futures.push(nf); } diff --git a/src/sys/component_manager/src/model/testing/test_helpers.rs b/src/sys/component_manager/src/model/testing/test_helpers.rs index 717797c2749a..18abbec23f34 100644 --- a/src/sys/component_manager/src/model/testing/test_helpers.rs +++ b/src/sys/component_manager/src/model/testing/test_helpers.rs @@ -698,6 +698,7 @@ pub struct TestEnvironmentBuilder { root_component: String, components: Vec<(&'static str, ComponentDecl)>, runtime_config: RuntimeConfig, + enable_hub: bool, } impl TestEnvironmentBuilder { @@ -706,6 +707,7 @@ impl TestEnvironmentBuilder { root_component: "root".to_owned(), components: vec![], runtime_config: Default::default(), + enable_hub: true, } } @@ -724,6 +726,11 @@ impl TestEnvironmentBuilder { self } + pub fn enable_hub(mut self, val: bool) -> Self { + self.enable_hub = val; + self + } + /// Returns a `Model` and `BuiltinEnvironment` suitable for most tests. pub async fn build(mut self) -> TestModelResult { let mock_runner = Arc::new(MockRunner::new()); @@ -741,6 +748,7 @@ impl TestEnvironmentBuilder { .add_resolver("test".to_string(), Box::new(mock_resolver)) .add_runner(TEST_RUNNER_NAME.into(), mock_runner.clone()) .set_runtime_config(self.runtime_config) + .enable_hub(self.enable_hub) .build() .await .expect("builtin environment setup failed"), @@ -778,12 +786,13 @@ impl ActionsTest { TestEnvironmentBuilder::new() .set_root_component(root_component) .set_components(components) + // Don't install the Hub's hooks because the Hub expects components + // to start and stop in a certain lifecycle ordering. In particular, some unit + // tests register individual actions in a way that confuses the hub's expectations. + .enable_hub(false) .build() .await; - // TODO(fsamuel): Don't install the Hub's hooks because the Hub expects components - // to start and stop in a certain lifecycle ordering. In particular, some unit - // tests will destroy component instances before binding to their parents. let test_hook = Arc::new(TestHook::new()); model.root.hooks.install(test_hook.hooks()).await; model.root.hooks.install(extra_hooks).await; diff --git a/src/sys/component_manager/src/model/tests/routing.rs b/src/sys/component_manager/src/model/tests/routing.rs index dff1ed5a3a64..adc4fb721fb0 100644 --- a/src/sys/component_manager/src/model/tests/routing.rs +++ b/src/sys/component_manager/src/model/tests/routing.rs @@ -12,7 +12,9 @@ use { config::{CapabilityAllowlistKey, CapabilityAllowlistSource}, framework::REALM_SERVICE, model::{ - actions::{ActionSet, DeleteChildAction, DestroyAction, ShutdownAction}, + actions::{ + ActionSet, DeleteChildAction, DestroyAction, MarkDeletedAction, ShutdownAction, + }, error::ModelError, events::registry::EventSubscription, hooks::{Event, EventPayload, EventType, Hook, HooksRegistration}, @@ -1826,6 +1828,9 @@ async fn destroying_instance_kills_framework_service_task() { // Destroy `b`. This should cause the task hosted for `Realm` to be cancelled. let root = test.model.look_up(&vec![].into()).await.unwrap(); + ActionSet::register(root.clone(), MarkDeletedAction::new("b".into())) + .await + .expect("mark deleted failed"); ActionSet::register(root.clone(), DeleteChildAction::new("b:0".into())) .await .expect("destroy failed"); diff --git a/src/sys/component_manager/tests/destruction/BUILD.gn b/src/sys/component_manager/tests/destruction/BUILD.gn index 38872008fa55..e3d20c243634 100644 --- a/src/sys/component_manager/tests/destruction/BUILD.gn +++ b/src/sys/component_manager/tests/destruction/BUILD.gn @@ -42,6 +42,27 @@ rustc_binary("destruction_collection_realm_bin") { sources = [ "collection_realm.rs" ] } +rustc_binary("destroy_and_recreate_bin") { + name = "destroy_and_recreate" + edition = "2018" + source_root = "destroy_and_recreate.rs" + deps = [ + "//garnet/lib/rust/io_util", + "//sdk/fidl/fuchsia.io:fuchsia.io-rustc", + "//sdk/fidl/fuchsia.sys2:fuchsia.sys2-rustc", + "//src/lib/fidl/rust/fidl", + "//src/lib/fuchsia-async", + "//src/lib/fuchsia-component", + "//src/lib/syslog/rust:syslog", + "//src/lib/zircon/rust:fuchsia-zircon", + "//src/sys/component_manager/tests/fidl:components-rustc", + "//third_party/rust_crates:anyhow", + "//third_party/rust_crates:log", + ] + + sources = [ "destroy_and_recreate.rs" ] +} + rustc_binary("destruction_trigger_bin") { name = "destruction_trigger" edition = "2018" @@ -75,10 +96,17 @@ fuchsia_component("collection_realm") { manifest = "meta/collection_realm.cml" } +fuchsia_component("destroy_and_recreate") { + testonly = true + deps = [ ":destroy_and_recreate_bin" ] + manifest = "meta/destroy_and_recreate.cml" +} + fuchsia_unittest_package("destruction_integration_test") { manifest = "meta/destruction_integration_test.cmx" deps = [ ":collection_realm", + ":destroy_and_recreate", ":destruction_integration_test_bin", ":trigger", ":trigger_realm", diff --git a/src/sys/component_manager/tests/destruction/collection_realm.rs b/src/sys/component_manager/tests/destruction/collection_realm.rs index 38f2fd3a8af9..7e5ec8ec7930 100644 --- a/src/sys/component_manager/tests/destruction/collection_realm.rs +++ b/src/sys/component_manager/tests/destruction/collection_realm.rs @@ -17,7 +17,7 @@ use { #[fasync::run_singlethreaded] async fn main() { - syslog::init_with_tags(&[]).expect("could not initialize logging"); + syslog::init().expect("could not initialize logging"); info!("Started collection realm"); let realm = client::connect_to_service::() .expect("could not connect to Realm service"); diff --git a/src/sys/component_manager/tests/destruction/destroy_and_recreate.rs b/src/sys/component_manager/tests/destruction/destroy_and_recreate.rs new file mode 100644 index 000000000000..e7fef4d70b0b --- /dev/null +++ b/src/sys/component_manager/tests/destruction/destroy_and_recreate.rs @@ -0,0 +1,122 @@ +// Copyright 2021 The Fuchsia Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +use { + anyhow::{Context as _, Error}, + fidl::endpoints::{self, Proxy}, + fidl_fidl_test_components as ftest, + fidl_fuchsia_io::{DirectoryMarker, DirectoryProxy, MODE_TYPE_SERVICE}, + fidl_fuchsia_sys2 as fsys, fuchsia_async as fasync, + fuchsia_component::client, + fuchsia_syslog as syslog, fuchsia_zircon as zx, + io_util::{self, OPEN_RIGHT_READABLE}, + log::*, + std::path::PathBuf, +}; + +#[fasync::run_singlethreaded] +async fn main() { + syslog::init().expect("could not initialize logging"); + info!("Started collection realm"); + let realm = client::connect_to_service::() + .expect("could not connect to Realm service"); + + // Create a "trigger realm" child component. + info!("Creating child"); + { + let mut collection_ref = fsys::CollectionRef { name: "coll".to_string() }; + let child_decl = fsys::ChildDecl { + name: Some("trigger".to_string()), + url: Some( + "fuchsia-pkg://fuchsia.com/destruction_integration_test#meta/trigger_realm.cm" + .to_string(), + ), + startup: Some(fsys::StartupMode::Lazy), + environment: None, + ..fsys::ChildDecl::EMPTY + }; + realm + .create_child(&mut collection_ref, child_decl) + .await + .expect(&format!("create_child failed")) + .expect(&format!("failed to create child")); + } + + // Bind to child, causing it to start (along with its eager children). + info!("Binding to child"); + { + let mut child_ref = + fsys::ChildRef { name: "trigger".to_string(), collection: Some("coll".to_string()) }; + let (dir, server_end) = endpoints::create_proxy::().unwrap(); + realm + .bind_child(&mut child_ref, server_end) + .await + .expect(&format!("bind_child failed")) + .expect(&format!("failed to bind to child")); + let trigger = open_trigger_svc(&dir).expect("failed to open trigger service"); + trigger.run().await.expect("trigger failed"); + } + + // Destroy the child. + info!("Destroying and recreating child"); + { + let mut child_ref = + fsys::ChildRef { name: "trigger".to_string(), collection: Some("coll".to_string()) }; + realm + .destroy_child(&mut child_ref) + .await + .expect("destroy_child failed") + .expect("failed to destroy child"); + } + + // Recreate the child immediately. + { + let mut collection_ref = fsys::CollectionRef { name: "coll".to_string() }; + let child_decl = fsys::ChildDecl { + name: Some("trigger".to_string()), + url: Some( + "fuchsia-pkg://fuchsia.com/destruction_integration_test#meta/trigger_realm.cm" + .to_string(), + ), + startup: Some(fsys::StartupMode::Lazy), + environment: None, + ..fsys::ChildDecl::EMPTY + }; + realm + .create_child(&mut collection_ref, child_decl) + .await + .expect(&format!("create_child failed")) + .expect(&format!("failed to create child")); + } + + // Sleep so that it's more likely that, if the new component is destroyed incorrectly, + // this test will fail. + fasync::Timer::new(fasync::Time::after(zx::Duration::from_seconds(5))).await; + + // Rebind to the child. + info!("Re-binding to child"); + { + let mut child_ref = + fsys::ChildRef { name: "trigger".to_string(), collection: Some("coll".to_string()) }; + let (dir, server_end) = endpoints::create_proxy::().unwrap(); + realm + .bind_child(&mut child_ref, server_end) + .await + .expect(&format!("bind_child failed")) + .expect(&format!("failed to bind to child")); + let trigger = open_trigger_svc(&dir).expect("failed to open trigger service"); + trigger.run().await.expect("trigger failed"); + } +} + +fn open_trigger_svc(dir: &DirectoryProxy) -> Result { + let node_proxy = io_util::open_node( + dir, + &PathBuf::from("fidl.test.components.Trigger"), + OPEN_RIGHT_READABLE, + MODE_TYPE_SERVICE, + ) + .context("failed to open trigger service")?; + Ok(ftest::TriggerProxy::new(node_proxy.into_channel().unwrap())) +} diff --git a/src/sys/component_manager/tests/destruction/integration_test.rs b/src/sys/component_manager/tests/destruction/integration_test.rs index ea049bafd1c7..886337fc13d1 100644 --- a/src/sys/component_manager/tests/destruction/integration_test.rs +++ b/src/sys/component_manager/tests/destruction/integration_test.rs @@ -14,7 +14,7 @@ use { }; #[fasync::run_singlethreaded(test)] -async fn destruction() { +async fn destroy() { let test = OpaqueTest::default( "fuchsia-pkg://fuchsia.com/destruction_integration_test#meta/collection_realm.cm", ) @@ -67,3 +67,16 @@ async fn destruction() { event.resume().await.unwrap(); expectation.await.unwrap(); } + +#[fasync::run_singlethreaded(test)] +async fn destroy_and_recreate() { + let mut test = OpaqueTest::default( + "fuchsia-pkg://fuchsia.com/destruction_integration_test#meta/destroy_and_recreate.cm", + ) + .await + .expect("failed to start test"); + let event_source = test.connect_to_event_source().await.unwrap(); + event_source.start_component_tree().await; + let status = test.component_manager_app.wait().await.expect("failed to wait for component"); + assert!(status.success(), "test failed"); +} diff --git a/src/sys/component_manager/tests/destruction/meta/destroy_and_recreate.cml b/src/sys/component_manager/tests/destruction/meta/destroy_and_recreate.cml new file mode 100644 index 000000000000..02cb5ada9b70 --- /dev/null +++ b/src/sys/component_manager/tests/destruction/meta/destroy_and_recreate.cml @@ -0,0 +1,19 @@ +{ + include: [ "sdk/lib/diagnostics/syslog/client.shard.cml" ], + program: { + runner: "elf", + binary: "bin/destroy_and_recreate", + }, + collections: [ + { + name: "coll", + durability: "transient", + }, + ], + use: [ + { + protocol: "fuchsia.sys2.Realm", + from: "framework", + }, + ], +} diff --git a/src/sys/component_manager/tests/destruction/meta/trigger_realm.cml b/src/sys/component_manager/tests/destruction/meta/trigger_realm.cml index ae6960b15de1..78ab897bdd57 100644 --- a/src/sys/component_manager/tests/destruction/meta/trigger_realm.cml +++ b/src/sys/component_manager/tests/destruction/meta/trigger_realm.cml @@ -22,6 +22,16 @@ capabilities: [ { protocol: "fidl.test.components.Trigger" }, ], + offer: [ + { + protocol: "fuchsia.logger.LogSink", + from: "parent", + to: [ + "#trigger_a", + "#trigger_b", + ], + }, + ], // Expose my Trigger service so the integration test can invoke it. expose: [