Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions docs/notes/2.34.x.md
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ Conditioned a previously unconditional 'chmod' of a file in the unpacked pants c

Fixed a crash when computing a process's cache key. `get_digest` returned a tuple and unwrapped the fallible `make_execute_request`, so any error inside it aborted the process rather than surfacing as an error. Both callers are on the cache-key path, so a transient failure while storing a process's wrapper script could take down an entire run with a panic and a "Please set RUST_BACKTRACE=1 ... and then file a bug" message.

Fixed a bug where Pants could spawn more concurrent LMDB reader threads than LMDB's default limit of 126 on machines with many CPU cores, causing runs to fail with `MDB_READERS_FULL`. The blocking-thread pool, sized as `--rule-threads-max` minus `--rule-threads-core`, is now capped below that limit, and a warning is logged when a run's configured thread counts get capped this way ([#23652](https://github.com/pantsbuild/pants/issues/23652)).

Fixed `system_binary` / PATH lookup crashing with `IntrinsicError: Operation not permitted` when a search-path entry (or a file under it) returns `EPERM`/`EACCES`. Those entries are now skipped so later PATH directories can still match.

### Goals
Expand Down
1 change: 1 addition & 0 deletions src/python/pants/engine/internals/native_engine.pyi
Original file line number Diff line number Diff line change
Expand Up @@ -408,6 +408,7 @@ class PyExecutor:
def __init__(self, core_threads: int, max_threads: int) -> None: ...
def to_borrowed(self) -> PyExecutor: ...
def shutdown(self, duration_secs: float) -> None: ...
def max_blocking_threads(self) -> int | None: ...

# ------------------------------------------------------------------------------
# Target
Expand Down
27 changes: 23 additions & 4 deletions src/python/pants/option/global_options.py
Original file line number Diff line number Diff line change
Expand Up @@ -276,14 +276,33 @@ class GlobalOptions(BootstrapOptions, Subsystem):

@staticmethod
def create_py_executor(bootstrap_options: OptionValueContainer) -> PyExecutor:
rule_threads_core = bootstrap_options.rule_threads_core
rule_threads_max = (
bootstrap_options.rule_threads_max
if bootstrap_options.rule_threads_max
else 4 * bootstrap_options.rule_threads_core
)
return PyExecutor(
core_threads=bootstrap_options.rule_threads_core, max_threads=rule_threads_max
else 4 * rule_threads_core
)
executor = PyExecutor(core_threads=rule_threads_core, max_threads=rule_threads_max)

requested_blocking_threads = rule_threads_max - rule_threads_core
effective_blocking_threads = executor.max_blocking_threads()
if (
effective_blocking_threads is not None
and effective_blocking_threads < requested_blocking_threads
):
logger.warning(
softwrap(
f"""
The configured `--rule-threads-core`/`--rule-threads-max` would have used
{requested_blocking_threads} blocking threads, but Pants capped this run to
{effective_blocking_threads} to stay under LMDB's reader limit and avoid
`MDB_READERS_FULL` errors (see
https://github.com/pantsbuild/pants/issues/23652). Lower `--rule-threads-max`
to silence this warning.
"""
)
)
return executor

@staticmethod
def resolve_keep_sandboxes(
Expand Down
34 changes: 33 additions & 1 deletion src/python/pants/option/global_options_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

from __future__ import annotations

import logging
import sys
from pathlib import Path
from textwrap import dedent
Expand All @@ -22,7 +23,7 @@
from pants.option.global_options import GlobalOptions
from pants.option.options_bootstrapper import OptionsBootstrapper
from pants.option.scope import GLOBAL_SCOPE
from pants.testutil.option_util import create_dynamic_remote_options
from pants.testutil.option_util import create_dynamic_remote_options, create_options_bootstrapper
from pants.testutil.pytest_util import no_exception
from pants.util.dirutil import safe_mkdir_for
from pants.version import VERSION
Expand Down Expand Up @@ -327,3 +328,34 @@ def test_free_threaded_advisory(
else:
assert advisory is not None
assert expected in advisory


def test_create_py_executor_respects_small_thread_counts() -> None:
ob = create_options_bootstrapper(
args=["--rule-threads-core=2", "--rule-threads-max=8"],
)
bootstrap_options = ob.bootstrap_options.for_global_scope()
executor = GlobalOptions.create_py_executor(bootstrap_options)
try:
assert executor.max_blocking_threads() == 6
finally:
executor.shutdown(5.0)


def test_create_py_executor_clamps_and_warns_for_large_thread_counts(caplog) -> None:
# rule_threads_core=64 means rule_threads_max defaults to 256, so the requested blocking pool
# (max - core = 192) is past the 120 cap (#23652).
ob = create_options_bootstrapper(
args=["--rule-threads-core=64"],
)
bootstrap_options = ob.bootstrap_options.for_global_scope()
executor = GlobalOptions.create_py_executor(bootstrap_options)
try:
assert executor.max_blocking_threads() == 120
# Pants renames the WARNING level to WARN process-wide, so check levelno, not levelname.
assert any(
record.levelno == logging.WARNING and "capped this run to 120" in record.getMessage()
for record in caplog.records
)
finally:
executor.shutdown(5.0)
6 changes: 6 additions & 0 deletions src/rust/engine/src/externs/scheduler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,12 @@ impl PyExecutor {
PyExecutor(self.0.to_borrowed())
}

/// The actual blocking-pool thread count, possibly lower than the `max_threads` passed to
/// `__new__` (see `task_executor::MAX_BLOCKING_THREADS_LMDB_SAFE`).
fn max_blocking_threads(&self) -> Option<usize> {
self.0.max_blocking_threads()
}

/// Shut down this executor, waiting for all tasks to exit. Any tasks which have not exited at
/// the end of the timeout will be leaked.
fn shutdown(&self, py: Python, duration_secs: f64) {
Expand Down
63 changes: 62 additions & 1 deletion src/rust/task_executor/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,11 @@ use tokio::task::{Id, JoinError, JoinHandle, JoinSet};
/// Tokio's own default. A conservative cap would stall clients rather than bound anything useful.
const MAX_REQUEST_THREADS: usize = 512;

/// Keeps the blocking pool under LMDB's 126-reader default so a run can't hit
/// `MDB_READERS_FULL`, no matter how large `--rule-threads-core`/`--rule-threads-max` or the
/// core count get. See https://github.com/pantsbuild/pants/issues/23652.
const MAX_BLOCKING_THREADS_LMDB_SAFE: usize = 120;

/// Copy our (thread-local or task-local) stdio destination and current workunit parent into
/// the task. The former ensures that when a pantsd thread kicks off a future, any stdio done
/// by it ends up in the pantsd log as we expect. The latter ensures that when a new workunit
Expand Down Expand Up @@ -52,6 +57,8 @@ pub struct Executor {
handle: Handle,
request_pool: Arc<Mutex<Option<Runtime>>>,
request_pool_handle: Option<Handle>,
/// Set by `new_owned`; `None` for `new()`, which borrows an ambient Runtime we don't size.
max_blocking_threads: Option<usize>,
}

impl Executor {
Expand All @@ -70,6 +77,7 @@ impl Executor {
handle: Handle::current(),
request_pool: Arc::new(Mutex::new(None)),
request_pool_handle: None,
max_blocking_threads: None,
}
}

Expand All @@ -94,11 +102,21 @@ impl Executor {
let on_thread_start = Arc::new(on_thread_start);
let on_thread_stop = Arc::new(on_thread_stop);

let requested_blocking_threads = match max_threads.checked_sub(num_worker_threads) {
Some(n) if n > 0 => n,
_ => {
return Err(format!(
"Invalid thread configuration: max_threads ({max_threads}) must be greater than num_worker_threads ({num_worker_threads})."
));
}
};
let max_blocking_threads = requested_blocking_threads.min(MAX_BLOCKING_THREADS_LMDB_SAFE);

let mut runtime_builder = Builder::new_multi_thread();

runtime_builder
.worker_threads(num_worker_threads)
.max_blocking_threads(max_threads - num_worker_threads)
.max_blocking_threads(max_blocking_threads)
.enable_all();

// NB: These run on every runtime-managed thread, including blocking-pool threads, which
Expand Down Expand Up @@ -144,6 +162,7 @@ impl Executor {
handle,
request_pool: Arc::new(Mutex::new(Some(request_pool))),
request_pool_handle: Some(request_pool_handle),
max_blocking_threads: Some(max_blocking_threads),
})
}

Expand All @@ -157,9 +176,17 @@ impl Executor {
handle: self.handle.clone(),
request_pool: Arc::new(Mutex::new(None)),
request_pool_handle: self.request_pool_handle.clone(),
max_blocking_threads: self.max_blocking_threads,
}
}

/// The actual blocking-pool thread count, possibly capped below what was requested (see
/// `MAX_BLOCKING_THREADS_LMDB_SAFE`). `None` if this Executor wraps an ambient Runtime we
/// didn't size ourselves (see `new()`).
pub fn max_blocking_threads(&self) -> Option<usize> {
self.max_blocking_threads
}

///
/// Enter the runtime context associated with this Executor. This should be used in situations
/// where threads not started by the runtime need access to it via task-local variables.
Expand Down Expand Up @@ -403,3 +430,37 @@ impl TailTasks {
}
}
}

#[cfg(test)]
mod tests {
use super::{Executor, MAX_BLOCKING_THREADS_LMDB_SAFE};

#[test]
fn new_owned_caps_blocking_threads_at_lmdb_safe_limit() {
// `rule_threads_max` can exceed this on large-core machines (#23652).
let executor =
Executor::new_owned(4, 4 + 10 * MAX_BLOCKING_THREADS_LMDB_SAFE, || {}, || {}).unwrap();
assert_eq!(
executor.max_blocking_threads(),
Some(MAX_BLOCKING_THREADS_LMDB_SAFE)
);
}

#[test]
fn new_owned_does_not_cap_small_requests() {
let requested_blocking_threads = MAX_BLOCKING_THREADS_LMDB_SAFE - 1;
let executor =
Executor::new_owned(2, 2 + requested_blocking_threads, || {}, || {}).unwrap();
assert_eq!(
executor.max_blocking_threads(),
Some(requested_blocking_threads)
);
}

#[test]
fn new_owned_errors_rather_than_underflows() {
// Regression: this subtraction used to underflow instead of returning an error.
assert!(Executor::new_owned(4, 4, || {}, || {}).is_err());
assert!(Executor::new_owned(4, 3, || {}, || {}).is_err());
}
}
Loading