diff --git a/brush-builtins/src/mapfile.rs b/brush-builtins/src/mapfile.rs index ba5505350..2d6f7f044 100644 --- a/brush-builtins/src/mapfile.rs +++ b/brush-builtins/src/mapfile.rs @@ -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. diff --git a/brush-builtins/src/read.rs b/brush-builtins/src/read.rs index 56809c60d..910354473 100644 --- a/brush-builtins/src/read.rs +++ b/brush-builtins/src/read.rs @@ -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(); diff --git a/brush-core/src/openfiles.rs b/brush-core/src/openfiles.rs index f51ece0de..292be668d 100644 --- a/brush-core/src/openfiles.rs +++ b/brush-core/src/openfiles.rs @@ -102,6 +102,44 @@ pub fn null() -> Result { 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(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(f: impl FnOnce() -> T) -> T { + f() +} + impl Clone for OpenFile { fn clone(&self) -> Self { match self { diff --git a/brush-shell/tests/cases/compat/builtins/mapfile.yaml b/brush-shell/tests/cases/compat/builtins/mapfile.yaml index 28a641d7e..d0584cfc1 100644 --- a/brush-shell/tests/cases/compat/builtins/mapfile.yaml +++ b/brush-shell/tests/cases/compat/builtins/mapfile.yaml @@ -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" diff --git a/brush-shell/tests/cases/compat/builtins/read.yaml b/brush-shell/tests/cases/compat/builtins/read.yaml index 8dc0e358a..5ed3d65a2 100644 --- a/brush-shell/tests/cases/compat/builtins/read.yaml +++ b/brush-shell/tests/cases/compat/builtins/read.yaml @@ -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" diff --git a/brush-shell/tests/cases/compat/word_expansion/command_substitution.yaml b/brush-shell/tests/cases/compat/word_expansion/command_substitution.yaml index 45951b9b4..ce3617689 100644 --- a/brush-shell/tests/cases/compat/word_expansion/command_substitution.yaml +++ b/brush-shell/tests/cases/compat/word_expansion/command_substitution.yaml @@ -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"