Hi Chet, thanks for taking the time to review this :D
[My apologies again upfront for another lengthy (comprehensive?) email.]
On Wed, 3 Apr 2024, Chet Ramey wrote:
On 4/2/24 12:22 PM, Carl Edquist wrote:
the forked coproc has to close its fds to/from _all_ other existing
coprocs (as there can be several).
And there is the issue. Without multi-coproc support, the shell only
keeps track of one coproc at a time, so there's only one set of pipe
file descriptors to close.
Right, exactly. The example with the default build (showing the essential
case that causes deadlock) was to highlight that your multi-coproc support
code apparently does indeed correctly track and close all these fds, and
thus prevents the deadlock issue.
On Wed, 3 Apr 2024, Chet Ramey wrote:
It's straightforward: the coproc process terminates, the shell reaps it,
marks it as dead, notifies the user that the process exited, and reaps
it before printing the next prompt. I don't observe any different
behavior between the default and when compiled for multiple coprocs.
It depends on when the process terminates as to whether you get a prompt
back and need to run an additional command before reaping the coproc
(macOS, RHEL), which gives you the opportunity to run the `read'
command:
Ah, my mistake then - thanks for explaining. I must have been thrown off
by the timing, running it with and without an intervening interactive
prompt before the read command. When run interactively, an extra 'Enter'
(or not) before the read command changes the behavior.
So in that case, this issue (that the shell closes its read-end of the
pipe from a reaped coproc, potentially before being able to read the final
output) was already there and is not specific to the multi-coproc code.
But in any case, it seems like this is a race then? That is, whether the
child process terminates before or after the prompt in question.
$ coproc WC { wc; }
[1] 48057
$ exec {WC[1]}>&-
$ read -u ${WC[0]} X
[1]+ Done coproc WC { wc; }
bash: DEBUG warning: cpl_reap: deleting 48057
$ echo $X
0 0 0
(I put in a trace statement to show exactly when the coproc gets reaped and
deallocated.)
Thanks! (for taking the time to play with this)
Though apparently it's still a race here. If you diagram the shell and
coproc (child) processes, I think you'll see that your DEBUG statement can
also happen _before_ the read command, which would then fail. You can
contrive this by adding a small sleep (eg, 0.1s) at the end of
execute_builtin_or_function (in execute_cmd.c), just before it returns.
Eg:
diff --git a/execute_cmd.c b/execute_cmd.c
index ed1063e..c72f322 100644
--- a/execute_cmd.c
+++ b/execute_cmd.c
@@ -5535,6 +5535,7 @@ execute_builtin_or_function (words, builtin, var,
redirects,
discard_unwind_frame ("saved_fifos");
#endif
+ usleep(100000);
return (result);
}
If I do this, I consistently see "read: X: invalid file descriptor
specification" running the above 4-line "coproc WC" example in a script,
demonstrating that there is no guarantee that the read command will start
before the WC coproc is reaped and {WC[0]} is closed, even though it's the
next statement after 'exec {WC[1]}>&-'.
But (as I'll try to show) you can trip up on this race even without
slowing down bash itself artificially.
I can't reproduce your results with non-interactive shells, either, with
job control enabled or disabled.
That's fair; let's try it with a script:
$ cat cope.sh
#!/bin/bash
coproc WC { wc; }
jobs
exec {WC[1]}>&-
[[ $1 ]] && sleep "$1"
jobs
read -u ${WC[0]} X
echo $X
Run without sleep, the wc output is seen:
$ ./cope.sh
[1]+ Running coproc WC { wc; } &
[1]+ Running coproc WC { wc; } &
0 0 0
Run with a brief sleep after closing the write end, and it breaks:
$ ./cope.sh .1
[1]+ Running coproc WC { wc; } &
[1]+ Done coproc WC { wc; }
./cope.sh: line 8: read: X: invalid file descriptor specification
And, if I run with "0" for a sleep time, it intermittently behaves like
either of the above. Racy!
This is a bug. The shell should not automatically close its read pipe
to a coprocess that has terminated -- it should stay open to read the
final output, and the user should be responsible for closing the read
end explicitly.
How long should the shell defer deallocating the coproc after the
process terminates?
I only offer my opinion here, but it strikes me that it definitely should
_not_ be based on an amount of _time_. That's inherently racy.
In the above example, there is only a single line to read; but the general
case there may be many 'records' sitting in the pipe waiting to be
processed, and processing each record may take an arbitrary amount of
time. (Consider a coproc containing a sort command, for example, that
produces all its output lines at once after it sees EOF, and then
terminates.)
Zack illustrated basically the same point with his example:
exec {fd}< <( some command )
while IFS='' read -r line <&"${fd}"; do
# do stuff
done
{fd}<&-
A process-substitution open to the shell like this is effectively a
one-ended coproc (though not in the jobs list), and it behaves reliably
here because the user can count on {fd} to remain open even after the
child process terminates.
So, the user can determine when the coproc fds are no longer needed,
whether that's when EOF is hit trying to read from the coproc, or whatever
other condition.
Personally I like the idea of 'closing' a coproc explicitly, but if it's a
bother to add options to the coproc keyword, then I would say just let the
user be responsible for closing the fds. Once the coproc has terminated
_and_ the coproc's fds are closed, then the coproc can be deallocated.
Apparently there is already some detection in there for when the coproc
fds get closed, as the {NAME[@]} fd array members get set to -1
automatically when when you do, eg, 'exec {NAME[0]}<&-'. So perhaps this
won't be a radical change.
Alternatively (or, additionally), you could interpret 'unset NAME' for a
coproc to mean "deallocate the coproc." That is, close the {NAME[@]} fds,
unset the NAME variable, and remove any coproc bookkeeping for NAME.
(Though if the coproc child process hasn't terminated on its own yet,
still it shouldn't be killed, and perhaps it should remain in the jobs
list as a background process until it's done.)
...
[And if you _really_ don't want to defer deallocating a coproc after it
terminates, I suppose you can go ahead and deallocate it in terms of
removing it from the jobs list and dropping any bookkeeping for it - as
long as you leave the fds and fd variables intact for the user.
It's a little dicey, but in theory it should not lead to deadlock, even if
copies of these (now untracked) reaped-coproc pipe fds end up in other
coprocs. Why not? Because (1) this coproc is dead and thus won't be
trying to read anymore from its (now closed) end, and (2) reads from the
shell's end will not block since there are no copies of the coproc's write
end open anymore.
Still, it'd be cleaner to defer deallocation, to avoid these stray (albeit
harmless) copies of fds making their way into new coprocs.]
What should it do to make sure that the variables don't hang around with
invalid file descriptors?
First, just to be clear, the fds to/from the coproc pipes are not invalid
when the coproc terminates (you can still read from them); they are only
invalid after they are closed.
The surprising bit is when they become invalid unexpectedly (from the
point of view of the user) because the shell closes them automatically,
at the somewhat arbitrary timing when the coproc is reaped.
Second, why is it a problem if the variables keep their (invalid) fds
after closing them, if the user is the one that closed them anyway?
Isn't this how it works with the auto-assigned fd redirections?
Eg:
$ exec {d}<.
$ echo $d
10
$ exec {d}<&-
$ echo $d
10
But, as noted, bash apparently already ensures that the variables don't
hang around with invalid file descriptors, as once you close them the
corresponding variable gets updated to "-1".
Or should the user be responsible for unsetting the array variable too?
(That's never been a requirement, obviously.)
On the one hand, bash is already messing with the coproc array variables
(setting the values to -1 when the user closes the fds), so it's not
really a stretch in my mind for bash to unset the whole variable when the
coproc is deallocated. On the other hand, as mentioned above, bash leaves
automatically allocated fd variables intact after the user explicitly
closes them.
So I guess either way seems reasonable.
If the user has explicitly closed both fd ends for a coproc, it should not
be a surprise to the user either way - whether the variable gets unset
automatically, or whether it remains with (-1 -1).
Since you are already unsetting the variable when the coproc is
deallocated though, I'd say it's fine to keep doing that -- just don't
deallocate the coproc before the user has closed both fds.
It also invites trouble if the shell variable that holds the fds gets
removed unexpectedly when the coprocess terminates. (Suddenly the
variable expands to an empty string.) It seems to me that the proper
time to clear the coproc variable (if at all) is after the user has
explicitly closed both of the fds.
That requires adding more plumbing than I want to,
Your project your call :D
especially since the user can always save the file descriptors of
interest into another variable if they want to use them after the coproc
terminates.
*Except* that it's inherently a race condition whether the original
variables will still be intact to save them.
Even if you attempt to save them immediately:
coproc X { exit; }
X_BACKUP=( ${X[@]} )
it's not guaranteed that X_BACKUP=(...) will run before coproc X has been
deallocated, and the X variable cleared.
No doubt this hasn't escaped you, but in any case you can see it for
yourself if you introduce a small delay in execute_coproc, in the parent,
just after the call to make_child:
diff --git a/execute_cmd.c b/execute_cmd.c
index ed1063e..5949e3e 100644
--- a/execute_cmd.c
+++ b/execute_cmd.c
@@ -2440,6 +2440,8 @@ execute_coproc (command, pipe_in, pipe_out,
fds_to_close)
exit (estat);
}
+ else
+ usleep(100000);
close (rpipe[1]);
close (wpipe[0]);
When I try this, X_BACKUP is consistently empty. Though if you add, say,
a call to "sleep 0.2" to coproc X, then X_BACKUP consistently gets a copy
of X's fds in time.
I am sorry if this sounds contrived, but I hope it demonstrates that
closing fds and and unsetting the variable for a coproc automatically when
it terminates is fundamentally flawed, because it depends on the arbitrary
race timing between the two processes.
*Or* else add an option to the coproc keyword to explicitly close the
coproc - which will close both fds and clear the variable.
Not going to add any more options to reserved words; that does more
violence to the grammar than I want.
Not sure how you'd feel about using 'unset' on the coproc variable
instead. (Though as discussed, I think the coproc terminated + fds
manually closed condition is also sufficient.)
.............
Anyway, as far as I'm concerned there's nothing urgent about all this, but
(along with the multi-coproc support that you implemented), avoiding the
current automatic deallocation behavior would seem to go a long way toward
making coproc a correct and generally useful feature.
Thanks for your time!
Carl
PS Zack you're welcome :)
