New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fish shell sends unexpected SIGPIPE to running processes #1926

Closed
Jean-Daniel opened this Issue Jan 31, 2015 · 5 comments

Comments

Projects
None yet
4 participants
@Jean-Daniel

Jean-Daniel commented Jan 31, 2015

When running some commands, the shell kills my jobs before they complete.
fish shell is the only shell that behave this way, so I consider it a bug.

For instance, I'm trying to run that command:
lld -arch x86_64 -r -print_atoms Inputs/dtrace-x86_64.yaml -o Output/dtrace-x86_64.yaml.tmp -flavor darwin | FileCheck dtrace-x86_64.yaml

The first command (lld) try to link the input object, print some internal status to stdout and then write the produced object to disk.
The second command (FileCheck) parses the output produced by lld and check if it contains some values.

It is perfectly valid and expected that FileCheck terminates before lld as it may be done looking for string in the output before lld is done writing the result to disk. When running that command in any shell (bash, tcsh, …) everything works fine, but when running it in fish, at some arbitrary point, lld receives a SIGPIPE signal that cause termination resulting in incomplete execution: the produced file is not valid (or not created at all).

Note: The function responsible to kill the process is in proc.cpp[462]: kill(prev->pid,SIGPIPE);

@zanchey zanchey added this to the next-major milestone Feb 1, 2015

@ChrisJefferson

This comment has been minimized.

Show comment
Hide comment
@ChrisJefferson

ChrisJefferson May 19, 2015

I think that perhaps fish is just a little more eager about killing the process, but is behaving basically the same as other shells.

For example, if I run: yes | head -n 5 in both bash, and fish, I get the output I expect:

y
y
y
y

And yes has been killed by SIGPIPE.

Therefore if a change was going to be made, we would have to decide exactly what it should be.

ChrisJefferson commented May 19, 2015

I think that perhaps fish is just a little more eager about killing the process, but is behaving basically the same as other shells.

For example, if I run: yes | head -n 5 in both bash, and fish, I get the output I expect:

y
y
y
y

And yes has been killed by SIGPIPE.

Therefore if a change was going to be made, we would have to decide exactly what it should be.

@ChrisJefferson

This comment has been minimized.

Show comment
Hide comment
@ChrisJefferson

ChrisJefferson May 19, 2015

On investigation, there is one difference between bash and fish (not sure if this is a "bug", but it is a measurable difference.

Given the following script (slow_print.sh)

#!/bin/sh
echo y; sleep 2; echo y; sleep 2; echo y; sleep 2

Then the command ./slow_print.sh | head -n 2 in bash quits after ~ 4 seconds, when the third y would be printed out. In fish it quits after ~ 2 seconds, as soon as the second y has been printed.

What I don't know is, should this be counted as a bug, or a feature :)

ChrisJefferson commented May 19, 2015

On investigation, there is one difference between bash and fish (not sure if this is a "bug", but it is a measurable difference.

Given the following script (slow_print.sh)

#!/bin/sh
echo y; sleep 2; echo y; sleep 2; echo y; sleep 2

Then the command ./slow_print.sh | head -n 2 in bash quits after ~ 4 seconds, when the third y would be printed out. In fish it quits after ~ 2 seconds, as soon as the second y has been printed.

What I don't know is, should this be counted as a bug, or a feature :)

@ChrisJefferson

This comment has been minimized.

Show comment
Hide comment
@ChrisJefferson

ChrisJefferson May 19, 2015

(sorry for repeated comments).

I can now sum this issue up compactly:

bash and zsh allow naturally occurring SIGPIPEs to cause piped together processes to exit, so given a | b, after b exits, a will exit when it next tries to write to to STDOUT.

fish on the other hand will eagerly kill a as soon as b exits. This behaviour is clearly on purpose, so I don't think it's a bug. It could be considered if it's a good feature. It's never annoyed me personally.

ChrisJefferson commented May 19, 2015

(sorry for repeated comments).

I can now sum this issue up compactly:

bash and zsh allow naturally occurring SIGPIPEs to cause piped together processes to exit, so given a | b, after b exits, a will exit when it next tries to write to to STDOUT.

fish on the other hand will eagerly kill a as soon as b exits. This behaviour is clearly on purpose, so I don't think it's a bug. It could be considered if it's a good feature. It's never annoyed me personally.

@Jean-Daniel

This comment has been minimized.

Show comment
Hide comment
@Jean-Daniel

Jean-Daniel May 19, 2015

If this is a feature, I would be curious to know what is the motivation to kill the parent process immediately when the child process exit and not when the parent try to write to the close pipe.

As some application expect the bash behavior but no application expect the fish one, I consider it as a bug more than a feature. In my case, a knows when it should stop to write in stdout and so never try to do it after b exit.

The command works perfectly with any shell but fish.

Jean-Daniel commented May 19, 2015

If this is a feature, I would be curious to know what is the motivation to kill the parent process immediately when the child process exit and not when the parent try to write to the close pipe.

As some application expect the bash behavior but no application expect the fish one, I consider it as a bug more than a feature. In my case, a knows when it should stop to write in stdout and so never try to do it after b exit.

The command works perfectly with any shell but fish.

@krader1961 krader1961 added the bug label Mar 14, 2017

krader1961 added a commit to krader1961/fish-shell that referenced this issue Mar 14, 2017

@krader1961 krader1961 self-assigned this Mar 14, 2017

@krader1961 krader1961 modified the milestones: 2.6.0, next-major Mar 14, 2017

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Mar 14, 2017

Contributor

Git blame shows that this behavior was already in place when the first git commit, 149594f, was made on 2005-09-20 and the code was still written in C. I don't think there is any question that this behavior is wrong. It appears to be a misguided effort to not "waste" time waiting for the left hand side of a pipe to notice that the right hand side has terminated. But that is just plain wrong since fish has no idea what that process might be doing and can not assume that preemptively killing it is safe.

The fix is trivial (see handle_child_status() in src/proc.cpp). Just remove this block of code:

if (p->completed && prev && !prev->completed && prev->pid) {
    kill(prev->pid, SIGPIPE);
}

If I do so the resulting shell passes all unit tests and works fine for me. The question is whether we dare to change it in a minor release. It's certainly possible people will notice the change in behavior; specifically, pipelines not completing as quickly. But I don't see how that could cause any problems or itself be construed a bug. So I think we should change it in the next minor release.

Contributor

krader1961 commented Mar 14, 2017

Git blame shows that this behavior was already in place when the first git commit, 149594f, was made on 2005-09-20 and the code was still written in C. I don't think there is any question that this behavior is wrong. It appears to be a misguided effort to not "waste" time waiting for the left hand side of a pipe to notice that the right hand side has terminated. But that is just plain wrong since fish has no idea what that process might be doing and can not assume that preemptively killing it is safe.

The fix is trivial (see handle_child_status() in src/proc.cpp). Just remove this block of code:

if (p->completed && prev && !prev->completed && prev->pid) {
    kill(prev->pid, SIGPIPE);
}

If I do so the resulting shell passes all unit tests and works fine for me. The question is whether we dare to change it in a minor release. It's certainly possible people will notice the change in behavior; specifically, pipelines not completing as quickly. But I don't see how that could cause any problems or itself be construed a bug. So I think we should change it in the next minor release.

develop7 added a commit to develop7/fish-shell that referenced this issue Apr 17, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment