Skip to content

Commit d0df9ec

Browse files
feat(process): distinguish between signals used to kill children (#10049)
### Description This is a redo of #10027 with some additional refactoring to reduce the complexity of this code. These changes should lessens the possibilities of deadlock or race conditions. I highly recommend viewing each commit individually. From original PR: > This is primarily a code move of the `ProcessManager` out of the `turborepo-lib` crate. We also now differ between a child shutting down in response to a `SIGINT` vs us killing the child. > Future PR will be changing how we treat each of these exit outcomes. The additional commits in this PR: - Fixed a race condition between between the 2 `wait`s performed by the `ChildStateManager` where one was called by the task the sent a shutdown to the child and the other was the default `wait`. If the latter won, then it would appear another entity killed the child even when it was really us. - Changed the return value of `ShutdownStyle::process` to avoid returning an impossible state - Remove unused `ChildState::Exited` field which was never read - Removed shared state between the child handle and the child state manager - Simplified child methods The removal of the shared state is possible since we already have the exit channel shared between the handles and the manager. Using both a channel and state was error prone as it lead to the following possibilities: - What does it mean if the manager is still listening for commands, but the state shows the child as exited - What does it mean if the manager channel is closed, but the state shows the child is still running Instead we only use the command channel being open/closed as an indication of if the child is running. Once the manager sees the child exit, it will send the exit status via the channel and exit resulting in the channel being closed. ### Testing Instructions Existing test suite, ran with `hyperfine 'cargo test' -r 500` to attempt to flush out any races.
1 parent 89abd4a commit d0df9ec

20 files changed

+165
-177
lines changed

Cargo.lock

+20-3
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

Cargo.toml

+1
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,7 @@ turborepo-fs = { path = "crates/turborepo-fs" }
6262
turborepo-lib = { path = "crates/turborepo-lib", default-features = false }
6363
turborepo-lockfiles = { path = "crates/turborepo-lockfiles" }
6464
turborepo-microfrontends = { path = "crates/turborepo-microfrontends" }
65+
turborepo-process = { path = "crates/turborepo-process" }
6566
turborepo-repository = { path = "crates/turborepo-repository" }
6667
turborepo-ui = { path = "crates/turborepo-ui" }
6768
turborepo-unescape = { path = "crates/turborepo-unescape" }

crates/turborepo-lib/Cargo.toml

+1-5
Original file line numberDiff line numberDiff line change
@@ -75,15 +75,13 @@ lazy_static = { workspace = true }
7575
libc = "0.2.140"
7676
merge = { workspace = true }
7777
miette = { workspace = true, features = ["fancy"] }
78-
nix = "0.26.2"
7978
notify = { workspace = true }
8079
num_cpus = { workspace = true }
8180
owo-colors = { workspace = true }
8281
oxc_resolver = { workspace = true }
8382
path-clean = "1.0.1"
8483
petgraph = { workspace = true }
8584
pidlock = { path = "../turborepo-pidlock" }
86-
portable-pty = "0.8.1"
8785
pprof = { version = "0.12.1", features = [
8886
"prost-codec",
8987
"frame-pointer",
@@ -138,6 +136,7 @@ turborepo-fs = { path = "../turborepo-fs" }
138136
turborepo-graph-utils = { path = "../turborepo-graph-utils" }
139137
turborepo-lockfiles = { workspace = true }
140138
turborepo-microfrontends = { workspace = true }
139+
turborepo-process = { workspace = true }
141140
turborepo-repository = { path = "../turborepo-repository" }
142141
turborepo-scm = { workspace = true, features = ["git2"] }
143142
turborepo-signals = { workspace = true }
@@ -156,9 +155,6 @@ which = { workspace = true }
156155
uds_windows = "1.0.2"
157156
async-io = "1.12.0"
158157

159-
[target.'cfg(target_os = "windows")'.dev-dependencies]
160-
windows-sys = { version = "0.59", features = ["Win32_System_Threading"] }
161-
162158
[build-dependencies]
163159
capnpc = "0.18.0"
164160
tonic-build = "0.8.4"

crates/turborepo-lib/src/lib.rs

-1
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,6 @@ mod microfrontends;
2828
mod opts;
2929
mod package_changes_watcher;
3030
mod panic_handler;
31-
mod process;
3231
mod query;
3332
mod rewrite_json;
3433
mod run;

crates/turborepo-lib/src/run/builder.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ use turborepo_api_client::{APIAuth, APIClient};
1313
use turborepo_cache::AsyncCache;
1414
use turborepo_env::EnvironmentVariableMap;
1515
use turborepo_errors::Spanned;
16+
use turborepo_process::ProcessManager;
1617
use turborepo_repository::{
1718
change_mapper::PackageInclusionReason,
1819
package_graph::{PackageGraph, PackageName},
@@ -44,7 +45,6 @@ use crate::{
4445
engine::{Engine, EngineBuilder},
4546
microfrontends::MicrofrontendsConfigs,
4647
opts::Opts,
47-
process::ProcessManager,
4848
run::{scope, task_access::TaskAccess, task_id::TaskName, Error, Run, RunCache},
4949
shim::TurboState,
5050
turbo_json::{TurboJson, TurboJsonLoader, UIMode},

crates/turborepo-lib/src/run/mod.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ use turbopath::{AbsoluteSystemPath, AbsoluteSystemPathBuf};
3030
use turborepo_api_client::{APIAuth, APIClient};
3131
use turborepo_ci::Vendor;
3232
use turborepo_env::EnvironmentVariableMap;
33+
use turborepo_process::ProcessManager;
3334
use turborepo_repository::package_graph::{PackageGraph, PackageName, PackageNode};
3435
use turborepo_scm::SCM;
3536
use turborepo_signals::{listeners::get_signal, SignalHandler};
@@ -45,7 +46,6 @@ use crate::{
4546
engine::Engine,
4647
microfrontends::MicrofrontendsConfigs,
4748
opts::Opts,
48-
process::ProcessManager,
4949
run::{global_hash::get_global_hash_inputs, summary::RunTracker, task_access::TaskAccess},
5050
task_graph::Visitor,
5151
task_hash::{get_external_deps_hash, get_internal_deps_hash, PackageInputsHashes},

crates/turborepo-lib/src/task_graph/visitor/command.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -4,12 +4,12 @@ use tracing::debug;
44
use turbopath::AbsoluteSystemPath;
55
use turborepo_env::EnvironmentVariableMap;
66
use turborepo_microfrontends::MICROFRONTENDS_PACKAGE;
7+
use turborepo_process::Command;
78
use turborepo_repository::package_graph::{PackageGraph, PackageInfo, PackageName};
89

910
use super::Error;
1011
use crate::{
11-
engine::Engine, microfrontends::MicrofrontendsConfigs, opts::TaskArgs, process::Command,
12-
run::task_id::TaskId,
12+
engine::Engine, microfrontends::MicrofrontendsConfigs, opts::TaskArgs, run::task_id::TaskId,
1313
};
1414

1515
pub trait CommandProvider {

crates/turborepo-lib/src/task_graph/visitor/exec.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ use console::StyledObject;
88
use tokio::sync::oneshot;
99
use tracing::{error, Instrument};
1010
use turborepo_env::{platform::PlatformEnv, EnvironmentVariableMap};
11+
use turborepo_process::{ChildExit, Command, ProcessManager};
1112
use turborepo_repository::package_manager::PackageManager;
1213
use turborepo_telemetry::events::{task::PackageTaskEventBuilder, TrackedErrors};
1314
use turborepo_ui::{ColorConfig, OutputWriter};
@@ -22,7 +23,6 @@ use crate::{
2223
cli::ContinueMode,
2324
config::UIMode,
2425
engine::{Engine, StopExecution},
25-
process::{ChildExit, Command, ProcessManager},
2626
run::{summary::TaskTracker, task_access::TaskAccess, task_id::TaskId, CacheOutput, TaskCache},
2727
task_hash::TaskHashTracker,
2828
};
@@ -454,7 +454,7 @@ impl ExecContext {
454454
// Something else killed the child
455455
ChildExit::KilledExternal => Err(InternalError::ExternalKill),
456456
// The child was killed by turbo indicating a shutdown
457-
ChildExit::Killed => Ok(ExecOutcome::Shutdown),
457+
ChildExit::Killed | ChildExit::Interrupted => Ok(ExecOutcome::Shutdown),
458458
}
459459
}
460460
}

crates/turborepo-lib/src/task_graph/visitor/mod.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ use turbopath::{AbsoluteSystemPath, AnchoredSystemPath};
2424
use turborepo_ci::{Vendor, VendorBehavior};
2525
use turborepo_env::{platform::PlatformEnv, EnvironmentVariableMap};
2626
use turborepo_errors::TURBO_SITE;
27+
use turborepo_process::ProcessManager;
2728
use turborepo_repository::package_graph::{PackageGraph, PackageName, ROOT_PKG_NAME};
2829
use turborepo_telemetry::events::{
2930
generic::GenericEventBuilder, task::PackageTaskEventBuilder, EventBuilder, TrackedErrors,
@@ -37,7 +38,6 @@ use crate::{
3738
engine::{Engine, ExecutionOptions},
3839
microfrontends::MicrofrontendsConfigs,
3940
opts::RunOpts,
40-
process::ProcessManager,
4141
run::{
4242
global_hash::GlobalHashableInputs,
4343
summary::{self, GlobalHashSummary, RunTracker},

crates/turborepo-process/Cargo.toml

+27
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
[package]
2+
name = "turborepo-process"
3+
version = "0.1.0"
4+
edition = "2021"
5+
license = "MIT"
6+
7+
[dev-dependencies]
8+
test-case = { workspace = true }
9+
tracing-test = { version = "0.2.4", features = ["no-env-filter"] }
10+
11+
[dependencies]
12+
atty = { workspace = true }
13+
console = { workspace = true }
14+
futures = { workspace = true }
15+
itertools = { workspace = true }
16+
libc = "0.2.140"
17+
nix = "0.26.2"
18+
portable-pty = "0.8.1"
19+
tokio = { workspace = true, features = ["full", "time"] }
20+
tracing.workspace = true
21+
turbopath = { workspace = true }
22+
23+
[lints]
24+
workspace = true
25+
26+
[target.'cfg(target_os = "windows")'.dev-dependencies]
27+
windows-sys = { version = "0.59", features = ["Win32_System_Threading"] }

0 commit comments

Comments
 (0)