There is no more race condition between parent and child with
regards to setting the process groups. Each child sets it for themselves
and then blocks indefinitely until the parent does what it needs to for
them (having waited for them to set their process groups). They are not
SIGCONT'd until the next process in the chain (if any) starts so that
that process can join their process group and open the pipes.
In the last commit, we introduced an indiscriminate if !EXTERNAL check
that unblocks a previously SIGSTOP'd command (if any) to allow the main
loop in exec_job to read from it without deadlocking (since builtins and
functions read directly from input as an optimization, sometimes).
Now only unblocking where a fork will not happen to ensure that if a
builtin ends up forking, that fork'd process is guaranteed to be able to
join the previous process' process group and access its output pipes.
We were having child processes SIGSTOP themselves immediately after
setting their process group and before launching their intended targets,
but they were not necessarily stopped by the time the next command was
being executed (so the opposite of the original race condition where
they might have finished executing by the time the next command came
around), and as a result when we sent them SIGCONT, that could never
reach. Now using waitpid to synchronize the SIGSTOP/SIGCONT between the
two.
If we had a good, unnamed inter-process event/semaphore, we could use
that to have a child process conditionally stop itself if the next
command in the job chain hadn't yet been started / setup, but this is
probably a lot more straightforward and less-confusing, which isn't a
bad thing.
Additionally, there was a bug caused by the fact that the main exec_job
loop actually blocks to read from previous commands in the job if the
current command is a built-in that doesn't need to fork.
With this waitpid code, I was able to finally add the SIGSTOP code to
all the fork'd processes in the main exec_job loop without introducing
deadlocks; it turns out that they should be treated just like the main
EXTERNAL fork, but they tend to execute faster causing the same deadlock
described above to occur more readily.
The only thing I'm not sure about is whether we should execute
unblock_pid undconditionally for all !EXTERNAL commands. It makes more
sense to *only* do that if a blocking read were about to be done in the
main loop, otherwise the original race condition could still appear
(though it is probably mitigated by whatever duration the SIGSTOP lasted
for, even if it is SIGCONT'd before the next command tries to join the
process group).
I hadn't realized that the for loop is called multiple times for a given
"single input" (anything that doesn't include semicolons, etc) to fish,
and so processes were being blocked but blocked_pid was lost by the time
that the next job (which was reading from the last process in the
previous job) came around.
Now using a static variable to store the last blocked PID. AFAICT, this
main job control loop is always executed from the same process and
thread, so this shouldn't need to be wrapped in atomics/mutexes, etc.
This code should be more portable, and certainly cleaner. We are
currently always sending SIGCONT to the last process (if it was part of
a job chain) regardless of whether it called SIGSTOP on itself or not,
which should be fine.
Need to explore whether or not the other forks in src/exec.cpp need to
be SIGSTOP'd on run or only the one that we included in this patch.
I'm not sure if this happens on all platforms, but under WSL with the
existing codebase, processes in the job chain that pipe their
stdout/stderr to the next process in the job could terminate before the
next job started (on fast enough machines for quick enough jobs).
This caused issues like #4235 and possibly #3952, at least for external
commands. What was happening is that the first process was finishing
before the second process was fully set up. fish would then try to
assign (in both the child and the parent) the process group id belonging
to the process group leader to the new process; and if the first process
had already terminated, it would have ended its process group with it as
well before that happened.
I'm not sure if there was already a mechanism in place for ensuring that
a process remains running at least as long as it takes for the next
process in the chain to join its group, etc., but if that code was
there, it wasn't working in my test setup (WSL).
This patch definitely needs some review; I'm not sure how I should
handle non-external commands (and external commands executed via
posix_spawn). I don't know if they are affected by the race condition in
the first place, but when I tried to add the same "wait for next command
in chain to run before unblocking" that would cause black screens
requiring ctrl+c to bypass.
The "unblock previous command" code was originally run by the next child
to be forked, but was then moved to the shell code instead, making it
more-centrally located and less error-prone.
Note that additional headers may be required for the mmap system call on
other platforms.
This is the first step to implementing issue #4200 is to stop subclassing
env_var_t from wcstring. Not too surprisingly doing this identified
several places that were incorrectly treating env_var_t and wcstring as
interchangeable types. I'm not talking about those places that passed
an env_var_t instance to a function that takes a wcstring. I'm talking
about doing things like assigning the former to the latter type, relying
on the implicit conversion, and thus losing information.
We also rename `env_get_string()` to `env_get()` for symmetry with
`env_set()` and to make it clear the function does not return a string.
This makes command substitutions impose the same limit on the amount
of data they accept as the `read` builtin. It does not limit output of
external commands or builtins in other contexts.
Fixes#3822
PR #3691 made most calls to `signal_block()` and `signal_unblock()`
no-ops unless a magic env var is set when fish starts running. It's
been seven months since that change was made and no problems have been
reported. This finishes that work by removing those no-op function calls
and support for the magic env var in our next major release (which won't
happen till at least six months from now).
This implements `status is-breakpoint` that returns true if the current
shell prompt is displayed in the context of a `breakpoint` command.
This also fixes several bugs. Most notably making `breakpoint` a no-op if
the shell isn't interactive. Also, typing `breakpoint` at an interactive
prompt should be an error rather than creating a new nested debugging
context.
Partial fix for #1310
This is the next step in determining whether we can disable blocking
signals without a good reason to do so. This makes not blocking signals
the default behavior. If someone finds a problem they can add this to
their ~/config/fish/config.fish file:
set FISH_NO_SIGNAL_BLOCK 0
Alternatively set that env var before starting fish. I won't be surprised
if people report problems. Till now we have relied on people opting in
to this behavior to tell us whether it causes problems. This makes the
experimental behavior the default that has to be opted out of. This will
give us a lot more confidence this change doesn't cause problems before
the next minor release.
Note that there are still a few places where we force blocking of
signals. Primarily to keep SIGTSTP from interfering with the shell in
response to manipulating the controlling tty. Bash is more selective
in the signals it blocks around the problematic syscalls (c.f., its
`git_terminal_to()` function). However, I don't see any value in that
refinement.
I recently upgraded the software on my macOS server and was dismayed to
see that cppcheck reported a huge number of format string errors due to
mismatches between the format string and its arguments from calls to
`assert()`. It turns out they are due to the macOS header using `%lu`
for the line number which is obviously wrong since it is using the C
preprocessor `__LINE__` symbol which evaluates to a signed int.
I also noticed that the macOS implementation writes to stdout, rather
than stderr. It also uses `printf()` which can be a problem on some
platforms if the stream is already in wide mode which is the normal case
for fish.
So implement our own `assert()` implementation. This also eliminates
double-negative warnings that we get from some of our calls to
`assert()` on some platforms by oclint.
Also reimplement the `DIE()` macro in terms of our internal
implementation.
Rewrite `assert(0 && msg)` statements to `DIE(msg)` for clarity and to
eliminate oclint warnings about constant expressions.
Fixes#3276, albeit not in the fashion I originally envisioned.
Currently the block stack is just a vector of pointers.
Clients must manually use new() to allocate a block, and then
transfer ownership to the stack (so must NOT delete it).
Give the parser itself responsibility for allocating blocks too,
so that it takes over both allocation and deletion. Use unique_ptr
to make deletion less error-prone.
The shell was doing a log of signal blocking/unblocking that hurts
performance and can be avoided. This reduced the elapsed time for a
simple benchmark by 25%.
Partial fix for #2007
On some platforms, notably GNU libc, you cannot mix narrow and wide
stdio functions on a stream like stdout or stderr. Doing so will drop
the output of one or the other. This change makes all output to the
stderr stream consistently use the wide forms.
This change also converts some fprintf(stderr,...) calls to debug()
calls where appropriate.
Fixes#3692
The existing code is inconsistent, and in a couple of cases wrong, about
dealing with strings that are not valid ints. For example, there are
locations that call wcstol() and check errno without first setting errno
to zero. Normalize the code to a consistent pattern. This is mostly to
deal with inconsistencies between BSD, GNU, and other UNIXes.
This does make some syntax more liberal. For example `echo $PATH[1 .. 3]`
is now valid due to uniformly allowing leading and trailing whitespace
around numbers. Whereas prior to this change you would get a "Invalid
index value" error. Contrast this with `echo $PATH[ 1.. 3 ]` which was
valid and still is.
This fixes some of the IWYU and cppcheck lint warnings. And only on
macOS (formerly OS X). Fixing these types of warnings on a broader set
of platforms should be done but this is a baby step to making `make
lint-all` have few, if any, warnings. This reduces the number of lines
in the `make lint-all` output on macOS by over 500 lines.
Implementing the --shadow-builtin flag has proven to be highly controversial.
Revert the introduction of that flag to the `function` command. If someone
shoots themselves in the foot by redefining a builtin as a function that's
their problem and not our responsibility to protect them from doing so.
Fixes#3319
Just use static_cast directly instead of inscrutible "shortcut"
macro.
It was not always used and doesn't seem to do much besides scramble
things up; encountering CAST_INIT() in the code seems likely to lead
to head scratching due to the transformation taking place.
It was added to save folks typing the type twice, now with 100
columns available, let's roll that convenience macro back.
sockaddr_dl:
Perform reinterpret_cast<sockaddr_dl> conversion. The cast affected
alignment and looks fishy to a compiler (but it's fine). Ditch
C-style cast and communicate we're doing that on purpose.
Where we already manage to cover an enum entirely in a switch
statement such that default: cannot be reached, help ensure
it stays that way by condemning that route.
Also adjust a 'const' I came across that is ignored.
This change does several things. First, and most important, it allows
dumping the "n" most recent stack frames on each debug() call. Second,
it demangles the C++ symbols. Third, it prepends each debug() message
with the debug level.
Unrelated to the above I've replaced all `assert(!is_forked_child());`
statements with `ASSERT_IS_NOT_FORKED_CHILD()` for consistency.
It's currently too easy for someone to bork their shell by doing something
like `function test; return 0; end`. That's obviously a silly, contrived,
example but the point is that novice users who learn about functions are
prone to do something like that without realizing it will bork the shell. Even
expert users who know about the `test` builtin might forget that, say, `pwd`
is a builtin.
This change adds a `--shadow-builtin` flag that must be specified to
indicate you know what you're doing.
Fixes#3000
The fork (create new process) related debugging messages rely on an
undocumented env var and use `printf()` rather than `debug()`. There are
also errors in how the fork count is tracked that this fixes.
Fixes#2995
I missed restyling a few "switch" blocks to make them consistent with the rest
of the code base. This fixes that oversight. This should be the final step in
restyling the C++ code to have a consistent style. This also includes a few
trivial cleanups elsewhere.
I also missed restyling the "complete" module when working my way from a to z
so this final change includes restyling that module.
Total lint errors decreased 36%. Cppcheck errors went from 47 to 24. Oclint P2
errors went from 819 to 778. Oclint P3 errors went from 3252 to 1842.
Resolves#2902.
Remove the "make iwyu" build target. Move the functionality into the
recently introduced lint.fish script. Fix a lot, but not all, of the
include-what-you-use errors. Specifically, it fixes all of the IWYU errors
on my OS X server but only removes some of them on my Ubuntu 14.04 server.
Fixes#2957
When replacing the existing fish process with a new process it is
important to restore the temrinal modes to what they were when fish
started running. We don't want any tweaks done for the benefit of fish
(e.g., disabling ICRNL mode) to bleed thru to an "exec"ed command.
Resolves#2609
If stdio is dead due to EPIPE, there's no great reason to spew a stack dump.
This will still write an error to stderr if stdout dies. This might be
undesirable, but changing that should be considered separately.
This change eliminates global variables like stdout_buffer. Instead we wrap up
the IO information into a new struct io_streams_t, and thread that through
every builtin. This makes the intent clearer, gives us a place to hang new IO
data, and eliminates the ugly global state management like builtin_push_io.
This change moves source files into a src/ directory,
and puts object files into an obj/ directory. The Makefile
and xcode project are updated accordingly.
Fixes#1866
