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
6 changes: 4 additions & 2 deletions brush-builtins/src/mapfile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -82,8 +82,10 @@ impl builtins::Command for MapFileCommand {
.try_fd(self.fd)
.ok_or_else(|| ErrorKind::BadFileDescriptor(self.fd))?;

// Read!
let results = self.read_entries(input_file)?;
// Read! The read is synchronous and can block until the write end is closed,
// so it must not hold onto the runtime worker while it waits.
let results =
brush_core::openfiles::without_parking_worker(|| self.read_entries(input_file))?;

if let Some(origin) = self.origin {
// -O: preserve existing array, assign at offset.
Expand Down
8 changes: 6 additions & 2 deletions brush-builtins/src/read.rs
Original file line number Diff line number Diff line change
Expand Up @@ -114,8 +114,12 @@ impl builtins::Command for ReadCommand {
// Convert timeout to Duration.
let timeout = self.timeout_in_seconds.map(Duration::from_secs_f64);

// Perform the read operation (potentially with timeout).
let read_result = self.read_line(input_stream, context.stderr(), timeout)?;
// Perform the read operation (potentially with timeout). Without -t this blocks
// until the write end is closed, so it must not hold onto the runtime worker
// while it waits.
let read_result = brush_core::openfiles::without_parking_worker(|| {
self.read_line(input_stream, context.stderr(), timeout)
})?;

// Determine whether to skip IFS splitting (for -N option).
let skip_ifs_splitting = self.return_after_n_chars_no_delimiter.is_some();
Expand Down
38 changes: 38 additions & 0 deletions brush-core/src/openfiles.rs
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,44 @@ pub fn null() -> Result<OpenFile, error::Error> {
Ok(file.into())
}

/// Runs a closure that may block indefinitely on a descriptor, without parking the
/// runtime worker it is running on.
///
/// Builtins that consume a descriptor (`mapfile`, `read`) read it synchronously, and
/// they run inline on the task that invoked them. On the multi-threaded runtime that
/// task is a worker thread, and a worker sitting in `read(2)` cannot poll the tasks
/// held in its own queues. That deadlocks whenever one of those tasks is the producer
/// responsible for closing the write end being read from.
///
/// `mapfile -t lines < <(cmd)` inside a command substitution is exactly that shape: the
/// producer for `<(cmd)` is spawned from the very worker that then blocks, so it lands
/// in that worker's LIFO slot, which other workers cannot steal. Nothing ever writes the
/// data or drops the write end, so EOF never arrives and the read never returns.
///
/// [`tokio::task::block_in_place`] hands the current worker's queues to another worker
/// before blocking, which keeps the producer runnable.
#[cfg(any(unix, windows))]
pub fn without_parking_worker<T>(f: impl FnOnce() -> T) -> T {
// `block_in_place` is only meaningful on the multi-threaded runtime, and panics on
// the current-thread one. Outside a runtime there is no worker to park.
if tokio::runtime::Handle::try_current()
.is_ok_and(|h| h.runtime_flavor() == tokio::runtime::RuntimeFlavor::MultiThread)
{
tokio::task::block_in_place(f)
} else {
f()
}
}

/// Runs a closure that may block indefinitely on a descriptor.
///
/// This target only ever runs a current-thread runtime, so there is no worker to hand
/// off to and the closure is run inline.
#[cfg(not(any(unix, windows)))]
pub fn without_parking_worker<T>(f: impl FnOnce() -> T) -> T {
f()
}

impl Clone for OpenFile {
fn clone(&self) -> Self {
match self {
Expand Down
45 changes: 45 additions & 0 deletions brush-shell/tests/cases/compat/builtins/mapfile.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -200,3 +200,48 @@ cases:
declare -n ref=target
mapfile -t -O 2 ref < <(echo -e "a\nb")
declare -p target

#
# Process substitution inside command substitution.
#
# These deadlocked before the read was moved off the runtime worker: the
# producer for `<(...)` is spawned from the same worker that mapfile then
# blocked on, so it could never run to close the write end.
#
- name: "mapfile from process substitution inside command substitution"
timeout_in_seconds: 10
stdin: |
x=$(mapfile -t arr < <(echo -e "a\nb"); echo "${arr[*]}")
echo "$x"

- name: "mapfile from process substitution in function in command substitution"
timeout_in_seconds: 10
stdin: |
f() {
mapfile -t arr < <(echo -e "a\nb")
echo "${arr[*]}"
}
x=$(f)
echo "$x"

- name: "repeated mapfile from process substitution in command substitution"
timeout_in_seconds: 10
stdin: |
f() {
mapfile -t a < <(echo one)
mapfile -t b < <(echo two)
echo "${a[*]} ${b[*]}"
}
x=$(f)
echo "$x"

- name: "mapfile from process substitution nested two command substitutions deep"
timeout_in_seconds: 10
stdin: |
g() {
mapfile -t arr < <(echo deep)
echo "${arr[*]}"
}
f() { echo "$(g)"; }
x=$(f)
echo "$x"
30 changes: 30 additions & 0 deletions brush-shell/tests/cases/compat/builtins/read.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -390,3 +390,33 @@ cases:
echo "arr[0]: ${arr[0]}"
echo "arr[1]: ${arr[1]}"
echo "arr[2]: ${arr[2]}"

#
# Process substitution inside command substitution.
#
# These deadlocked before the read was moved off the runtime worker: the
# producer for `<(...)` is spawned from the same worker that read then
# blocked on, so it could never run to close the write end.
#
- name: "while read loop fed by process substitution inside command substitution"
timeout_in_seconds: 10
stdin: |
x=$(while read -r line; do echo "got $line"; done < <(echo -e "a\nb"))
echo "$x"

- name: "while read loop fed by process substitution in function in command substitution"
timeout_in_seconds: 10
stdin: |
f() {
while read -r line; do
echo "got $line"
done < <(echo -e "a\nb")
}
x=$(f)
echo "$x"

- name: "read from process substitution inside command substitution"
timeout_in_seconds: 10
stdin: |
x=$(read -r line < <(echo hello); echo "$line")
echo "$x"
Original file line number Diff line number Diff line change
Expand Up @@ -220,3 +220,21 @@ cases:

$($(:))
echo "exit code: $?"

#
# A command substitution whose body redirects a builtin from a process
# substitution. The producer for `<(...)` is spawned from the worker running
# the substitution, so a builtin that reads it synchronously must not hold
# that worker while it waits.
#
- name: "Command substitution containing a builtin fed by process substitution"
timeout_in_seconds: 10
stdin: |
x=$(mapfile -t arr < <(echo -e "one\ntwo"); echo "${arr[*]}")
echo "$x"

- name: "Command substitution containing an external command fed by process substitution"
timeout_in_seconds: 10
stdin: |
x=$(cat < <(echo hello))
echo "$x"
Loading