Summary
Cleanup / documentation. #5 fixed a deadlock caused by a builtin blocking on a descriptor while occupying a tokio worker. The invariant that prevents a recurrence is currently written down in only one place — the doc comment on openfiles::without_parking_worker — which is a file a builtin author has no particular reason to open.
Proposal: state it on builtins::Command::execute in brush-core/src/builtins.rs, since that is the thing someone reads when writing a new builtin.
The invariant
A builtin runs inline on the task that invoked it, and for the common case that task is a runtime worker:
interp.rs sets run_in_current_shell = pipeline_len == 1 || ...- a single-command pipeline therefore takes
ShellForCommand::ParentShell
- which dispatches to
execute_via_builtin_in_parent_shell, awaiting the builtin directly rather than handing it to a thread
So a builtin that performs a blocking read occupies a worker for the duration. That is fine when the data is already there, and deadlocks when the thing that would produce the data is a task sitting in that same worker's LIFO slot — which is exactly what mapfile -t x < <(cmd) inside a command substitution produces, because the process-substitution producer is spawned from that worker and the LIFO slot is not stealable by its peers.
Note this is the common path, not the exceptional one. execute_via_builtin_in_owned_shell (multi-command pipelines) does use spawn_blocking and is safe, but a plain foo < <(bar) is a single-command pipeline.
Not an instance: bundled coreutils
Worth recording so the next person does not have to re-derive it. The experimental-bundled-coreutils builtins look like they should be affected — with the feature on, type cat reports cat is a shell builtin — but they are not, because they do not run in-process. The shims re-execute the brush binary as a child process, as the crate docs describe:
$ brush -c 'echo "shell pid: $$"; cat /proc/self/status | grep -E "^(Name|Pid):"'
shell pid: 402965
Name: brush
Pid: 402983 # separate process
The shell awaits a child and yields, so no worker is held. Confirmed empirically that cat, head, sort and wc all complete normally in the shape that hung mapfile.
Suggested wording
Something to the effect of: a builtin implementation must not block indefinitely on a file descriptor; it runs on a runtime worker and blocking there can prevent a process-substitution producer from ever being polled. Use openfiles::without_parking_worker around any synchronous read that can wait on a pipe.
After #5 the two builtins that need it (mapfile/readarray and read) already use it, so this is purely about the next one.
Optionally
A debug assertion — or a #[cfg(debug_assertions)] warning — when a builtin returns after having blocked on the current worker for longer than some threshold would catch it at the point of introduction rather than as a hang in someone's script. Lower priority than the doc note and more intrusive, so mentioning rather than proposing.
Summary
Cleanup / documentation. #5 fixed a deadlock caused by a builtin blocking on a descriptor while occupying a tokio worker. The invariant that prevents a recurrence is currently written down in only one place — the doc comment on
openfiles::without_parking_worker— which is a file a builtin author has no particular reason to open.Proposal: state it on
builtins::Command::executeinbrush-core/src/builtins.rs, since that is the thing someone reads when writing a new builtin.The invariant
A builtin runs inline on the task that invoked it, and for the common case that task is a runtime worker:
interp.rssetsrun_in_current_shell = pipeline_len == 1 || ...ShellForCommand::ParentShellexecute_via_builtin_in_parent_shell, awaiting the builtin directly rather than handing it to a threadSo a builtin that performs a blocking read occupies a worker for the duration. That is fine when the data is already there, and deadlocks when the thing that would produce the data is a task sitting in that same worker's LIFO slot — which is exactly what
mapfile -t x < <(cmd)inside a command substitution produces, because the process-substitution producer is spawned from that worker and the LIFO slot is not stealable by its peers.Note this is the common path, not the exceptional one.
execute_via_builtin_in_owned_shell(multi-command pipelines) does usespawn_blockingand is safe, but a plainfoo < <(bar)is a single-command pipeline.Not an instance: bundled coreutils
Worth recording so the next person does not have to re-derive it. The
experimental-bundled-coreutilsbuiltins look like they should be affected — with the feature on,type catreportscat is a shell builtin— but they are not, because they do not run in-process. The shims re-execute the brush binary as a child process, as the crate docs describe:The shell awaits a child and yields, so no worker is held. Confirmed empirically that
cat,head,sortandwcall complete normally in the shape that hungmapfile.Suggested wording
Something to the effect of: a builtin implementation must not block indefinitely on a file descriptor; it runs on a runtime worker and blocking there can prevent a process-substitution producer from ever being polled. Use
openfiles::without_parking_workeraround any synchronous read that can wait on a pipe.After #5 the two builtins that need it (
mapfile/readarrayandread) already use it, so this is purely about the next one.Optionally
A debug assertion — or a
#[cfg(debug_assertions)]warning — when a builtin returns after having blocked on the current worker for longer than some threshold would catch it at the point of introduction rather than as a hang in someone's script. Lower priority than the doc note and more intrusive, so mentioning rather than proposing.