Merge #1071

1071: Terminate child containers on program termination. r=Emilgardis a=Alexhuszagh

Upon receiving a termination signal, ensuring any spawned containers are also terminated. This is required since we intentional do not attach stdint to the container. This means, however, that interrupts such as Ctrl+C did not stop the build process. Likewise, termination signals such as SIGTERM merely exited cross, and did not terminate the container.

This also refactors the global container state to store whether the container exists and current logic to terminate the container if it exists. By coupling the two, it simplifies safe handling and modification of this global state. Since we have a signal handler, these functions may be called asynchronously, therefore any global state may be modified, rendering the current state invalid if not appropriately guarded. Coupling the two ensures that our logic is sound.

Fixes the regression in #964.
Closes #1041.

Co-authored-by: Alex Huszagh <ahuszagh@gmail.com>

Author: bors[bot]

src/bin/commands/containers.rs

@@ -319,7 +319,7 @@ fn get_cross_volumes(
     engine: &docker::Engine,
     msg_info: &mut MessageInfo,
 ) -> cross::Result<Vec<String>> {
-    use cross::docker::remote::VOLUME_PREFIX;
+    use cross::docker::VOLUME_PREFIX;
     let stdout = docker::subcommand(engine, "volume")
         .arg("list")
         .args(["--format", "{{.Name}}"])
@@ -400,7 +400,7 @@ pub fn create_persistent_volume(
     let container = dirs.unique_container_identifier(&toolchain.host().target)?;
     let volume = dirs.unique_toolchain_identifier()?;
 
-    if docker::remote::volume_exists(engine, &volume, msg_info)? {
+    if docker::volume_exists(engine, &volume, msg_info)? {
         eyre::bail!("Error: volume {volume} already exists.");
     }
 
@@ -409,18 +409,18 @@ pub fn create_persistent_volume(
         .run_and_get_status(msg_info, false)?;
 
     // stop the container if it's already running
-    let state = docker::remote::container_state(engine, &container, msg_info)?;
+    let state = docker::container_state(engine, &container, msg_info)?;
     if !state.is_stopped() {
         msg_info.warn(format_args!("container {container} was running."))?;
-        docker::remote::container_stop_default(engine, &container, msg_info)?;
+        docker::container_stop_default(engine, &container, msg_info)?;
     }
     if state.exists() {
         msg_info.warn(format_args!("container {container} was exited."))?;
-        docker::remote::container_rm(engine, &container, msg_info)?;
+        docker::container_rm(engine, &container, msg_info)?;
     }
 
     // create a dummy running container to copy data over
-    let mount_prefix = docker::remote::MOUNT_PREFIX;
+    let mount_prefix = docker::MOUNT_PREFIX;
     let mut docker = docker::subcommand(engine, "run");
     docker.args(["--name", &container]);
     docker.arg("--rm");
@@ -440,7 +440,7 @@ pub fn create_persistent_volume(
         docker.args(["sh", "-c", "sleep infinity"]);
     }
     // store first, since failing to non-existing container is fine
-    docker::remote::create_container_deleter(engine.clone(), container.clone());
+    docker::Container::create(engine.clone(), container.clone())?;
     docker.run_and_get_status(msg_info, false)?;
 
     docker::remote::copy_volume_container_xargo(
@@ -467,7 +467,7 @@ pub fn create_persistent_volume(
         msg_info,
     )?;
 
-    docker::remote::drop_container(is_tty, msg_info);
+    docker::Container::finish_static(is_tty, msg_info);
 
     Ok(())
 }
@@ -486,11 +486,11 @@ pub fn remove_persistent_volume(
     let dirs = docker::ToolchainDirectories::assemble(&mount_finder, toolchain)?;
     let volume = dirs.unique_toolchain_identifier()?;
 
-    if !docker::remote::volume_exists(engine, &volume, msg_info)? {
+    if !docker::volume_exists(engine, &volume, msg_info)? {
         eyre::bail!("Error: volume {volume} does not exist.");
     }
 
-    docker::remote::volume_rm(engine, &volume, msg_info)?;
+    docker::volume_rm(engine, &volume, msg_info)?;
 
     Ok(())
 }
@@ -499,7 +499,7 @@ fn get_cross_containers(
     engine: &docker::Engine,
     msg_info: &mut MessageInfo,
 ) -> cross::Result<Vec<String>> {
-    use cross::docker::remote::VOLUME_PREFIX;
+    use cross::docker::VOLUME_PREFIX;
     let stdout = docker::subcommand(engine, "ps")
         .arg("-a")
         .args(["--format", "{{.Names}}: {{.State}}"])
@@ -533,7 +533,7 @@ pub fn remove_all_containers(
         // cannot fail, formatted as {{.Names}}: {{.State}}
         let (name, state) = container.split_once(':').unwrap();
         let name = name.trim();
-        let state = docker::remote::ContainerState::new(state.trim())?;
+        let state = docker::ContainerState::new(state.trim())?;
         if state.is_stopped() {
             stopped.push(name);
         } else {

src/docker/local.rs

@@ -1,6 +1,7 @@
 use std::io;
 use std::path::Path;
 use std::process::{Command, ExitStatus};
+use std::sync::atomic::Ordering;
 
 use super::shared::*;
 use crate::errors::Result;
@@ -50,6 +51,8 @@ pub(crate) fn run(
         msg_info,
     )?;
 
+    let container = toolchain_dirs.unique_container_identifier(options.target.target())?;
+    docker.args(["--name", &container]);
     docker.arg("--rm");
 
     docker_seccomp(&mut docker, engine.kind, &options.target, &paths.metadata)
@@ -121,9 +124,22 @@ pub(crate) fn run(
             .wrap_err("when building custom image")?;
     }
 
-    docker
+    Container::create(engine.clone(), container)?;
+    let status = docker
         .arg(&image_name)
         .args(["sh", "-c", &build_command(toolchain_dirs, &cmd)])
         .run_and_get_status(msg_info, false)
-        .map_err(Into::into)
+        .map_err(Into::into);
+
+    // `cargo` generally returns 0 or 101 on completion, but isn't guaranteed
+    // to. `ExitStatus::code()` may be None if a signal caused the process to
+    // terminate or it may be a known interrupt return status (130, 137, 143).
+    // simpler: just test if the program termination handler was called.
+    // SAFETY: an atomic load.
+    let is_terminated = unsafe { crate::errors::TERMINATED.load(Ordering::SeqCst) };
+    if !is_terminated {
+        Container::exit_static();
+    }
+
+    status
 }

src/docker/mod.rs

@@ -1,4 +1,4 @@
-pub mod custom;
+pub(crate) mod custom;
 mod engine;
 mod image;
 mod local;