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

Also give subshells the terminal #3922

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@faho
Member

faho commented Mar 29, 2017

Description

It turns out the reason fzf et al don't work in command substitutions (for some reason known as "subshells" in this part of the code) is that we don't give them the JOB_TERMINAL flag, which means we don't hand control of the terminal to their process group.

This is likely to need more testing.

Things I've tested so far:

  • echo -s x(git branch | fzf | string trim)x (which would previously hang)

  • echo (cat) (which would print a newline and leave a stopped cat)

Also sudo and percol.

Fixes issue #1362.

TODOs:

  • [N/A] Changes to fish usage are reflected in user documentation/manpages.
  • Tests have been added for regressions fixed
  • User-visible changes noted in CHANGELOG.md

@faho faho added the bug label Mar 29, 2017

@faho faho added this to the 2.6.0 milestone Mar 29, 2017

@faho

This comment has been minimized.

Show comment
Hide comment
@faho

faho Mar 29, 2017

Member

command substitutions (for some reason known as "subshells" in this part of the code)

The reason being that it's not just command substitutions - it's also e.g. the prompt functions or completions.

That means after this, complete -c something -a '(read -l something)' will actually read when completing something. That doesn't seem useful, but it also doesn't seem harmful, in an "if you want to shoot yourself in the foot" kind of way. Similarly, now the prompt could read stuff, where previously most likely whatever would try to read would get SIGTTOU and stop, leaving stray processes all over the place.

Member

faho commented Mar 29, 2017

command substitutions (for some reason known as "subshells" in this part of the code)

The reason being that it's not just command substitutions - it's also e.g. the prompt functions or completions.

That means after this, complete -c something -a '(read -l something)' will actually read when completing something. That doesn't seem useful, but it also doesn't seem harmful, in an "if you want to shoot yourself in the foot" kind of way. Similarly, now the prompt could read stuff, where previously most likely whatever would try to read would get SIGTTOU and stop, leaving stray processes all over the place.

@layus

This comment has been minimized.

Show comment
Hide comment
@layus

layus Mar 29, 2017

echo (cat) (which would print a newline and leave a stopped cat)

Do you mean that you leave behind a cat waiting on stdin ? Could this cat interact with the standard input after the end of the echo command ?

layus commented Mar 29, 2017

echo (cat) (which would print a newline and leave a stopped cat)

Do you mean that you leave behind a cat waiting on stdin ? Could this cat interact with the standard input after the end of the echo command ?

@faho

This comment has been minimized.

Show comment
Hide comment
@faho

faho Mar 29, 2017

Member

Do you mean that you leave behind a cat waiting on stdin ?

Not quite. The cat has received a signal (SIGTTIN or SIGTTOU) because it tried to read from or write to a terminal that it did not have control over. Now, a process could react to these signals and do something different, but the default action is to stop (as if it received SIGSTOP), which is also what cat specifically does.

Could this cat interact with the standard input after the end of the echo command ?

No, because we never give it control of the terminal. We could theoretically give it up later, but that would make $anycommand (cat) meaningless. What this does instead is give it control when we start it.

Member

faho commented Mar 29, 2017

Do you mean that you leave behind a cat waiting on stdin ?

Not quite. The cat has received a signal (SIGTTIN or SIGTTOU) because it tried to read from or write to a terminal that it did not have control over. Now, a process could react to these signals and do something different, but the default action is to stop (as if it received SIGSTOP), which is also what cat specifically does.

Could this cat interact with the standard input after the end of the echo command ?

No, because we never give it control of the terminal. We could theoretically give it up later, but that would make $anycommand (cat) meaningless. What this does instead is give it control when we start it.

@layus

layus approved these changes Mar 30, 2017

I took time to test your patch, and this works amazingly well. Please merge as soon as possible :-).

It misunderstood your comment (which would print a newline and leave a stopped cat) as the new behavior, while it describes the old, buggy behavior.

I tested it and it gives

gmaudoux@klatch ~/p/fish> echo (cat)
Hello, World![^D, not rendered]
Hello, World!
gmaudoux@klatch ~/p/fish> 

Simply amazing how this simple fix solves a longstanding pain.

@layus

This comment has been minimized.

Show comment
Hide comment
@layus

layus Mar 30, 2017

May I however add that it does not solve the issue with pipes, for example with sudo netstat -tnpa | grep sshd. Should I file a different issue ?

layus commented Mar 30, 2017

May I however add that it does not solve the issue with pipes, for example with sudo netstat -tnpa | grep sshd. Should I file a different issue ?

@faho

This comment has been minimized.

Show comment
Hide comment
@faho

faho Mar 30, 2017

Member

May I however add that it does not solve the issue with pipes, for example with sudo netstat -tnpa | grep sshd. Should I file a different issue ?

No, that's #206.

Edit: Note that this exposes the actual issue there - we don't redirect properly, the command ends up connected to the terminal. With this, it'll have permission to read/write. Without this, it'd get SIGTTIN/SIGTTOU, be stopped and hang around.

Member

faho commented Mar 30, 2017

May I however add that it does not solve the issue with pipes, for example with sudo netstat -tnpa | grep sshd. Should I file a different issue ?

No, that's #206.

Edit: Note that this exposes the actual issue there - we don't redirect properly, the command ends up connected to the terminal. With this, it'll have permission to read/write. Without this, it'd get SIGTTIN/SIGTTOU, be stopped and hang around.

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Mar 31, 2017

Contributor

echo (cat) (which would print a newline and leave a stopped cat)

FYI, yes it does although that isn't immediately obvious since you are returned to your interactive shell. And you have to run the jobs command or something like ps waux | grep cat to notice those stopped jobs.

This simple fix seems almost too good to be correct. So it is going to need some testing and analysis before being merged. Having said that I am cautiously optimistic this is correct or at least more correct than the existing behavior based on my limited testing.

Contributor

krader1961 commented Mar 31, 2017

echo (cat) (which would print a newline and leave a stopped cat)

FYI, yes it does although that isn't immediately obvious since you are returned to your interactive shell. And you have to run the jobs command or something like ps waux | grep cat to notice those stopped jobs.

This simple fix seems almost too good to be correct. So it is going to need some testing and analysis before being merged. Having said that I am cautiously optimistic this is correct or at least more correct than the existing behavior based on my limited testing.

@faho

This comment has been minimized.

Show comment
Hide comment
@faho

faho Mar 31, 2017

Member

This simple fix seems almost too good to be correct.

Yes, it does. Though it also makes a certain degree of sense - this code explicitly stops any "subshell" from using the terminal, so removing it should allow it to.

Having said that I am cautiously optimistic this is correct or at least more correct than the existing behavior based on my limited testing.

Yes, "more correct" being the thing to shoot for. Maybe there is a case where something shouldn't receive access to the terminal, but that case should be handled specially then. Also it appears less common than command substitutions, given that I haven't found it so far.

Member

faho commented Mar 31, 2017

This simple fix seems almost too good to be correct.

Yes, it does. Though it also makes a certain degree of sense - this code explicitly stops any "subshell" from using the terminal, so removing it should allow it to.

Having said that I am cautiously optimistic this is correct or at least more correct than the existing behavior based on my limited testing.

Yes, "more correct" being the thing to shoot for. Maybe there is a case where something shouldn't receive access to the terminal, but that case should be handled specially then. Also it appears less common than command substitutions, given that I haven't found it so far.

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Apr 1, 2017

Contributor

I can't think of how this change could cause problems absent bugs elsewhere in the code and I haven't observed any problems. Too, it is important to get more people testing this sooner rather than later. So I've gone ahead and merged it.

Contributor

krader1961 commented Apr 1, 2017

I can't think of how this change could cause problems absent bugs elsewhere in the code and I haven't observed any problems. Too, it is important to get more people testing this sooner rather than later. So I've gone ahead and merged it.

@krader1961 krader1961 closed this Apr 1, 2017

faho added a commit that referenced this pull request Apr 1, 2017

@LindyBalboa

This comment has been minimized.

Show comment
Hide comment
@LindyBalboa

LindyBalboa Apr 12, 2017

I just want to say THANK YOU for fixing this. This was one of my biggest complaints about fish. I think now I might be sold for life.

LindyBalboa commented Apr 12, 2017

I just want to say THANK YOU for fixing this. This was one of my biggest complaints about fish. I think now I might be sold for life.

@layus layus referenced this pull request Apr 12, 2017

Open

Job handling is broken. #3952

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

@miquella

This comment has been minimized.

Show comment
Hide comment
@miquella

miquella Apr 27, 2017

Man, four hours of troubleshooting trying to figure out what was causing my issue, looks like this was it! Thank you for the fix!

miquella commented Apr 27, 2017

Man, four hours of troubleshooting trying to figure out what was causing my issue, looks like this was it! Thank you for the fix!

miquella added a commit to miquella/vaulted that referenced this pull request Apr 27, 2017

Fix suggested fish command to load variables
The previous command ended up running all of the lines together on a
single line and since the first line begins with a comment character,
the entire script was effectively commented out!

For additional information, see:
 * fish-shell/fish-shell#159
 * fish-shell/fish-shell#3993

Additionally, commands run in a fish command substitution do not have
access to the TTY, preventing vaulted from prompting for a password.
Using piping avoids this issue all together.

For additional information, see:
 * fish-shell/fish-shell#1362
 * fish-shell/fish-shell#3922

tpickett66 added a commit to miquella/vaulted that referenced this pull request Apr 27, 2017

Fix suggested fish command to load variables
The previous command ended up running all of the lines together on a
single line and since the first line begins with a comment character,
the entire script was effectively commented out!

For additional information, see:
 * fish-shell/fish-shell#159
 * fish-shell/fish-shell#3993

Additionally, commands run in a fish command substitution do not have
access to the TTY, preventing vaulted from prompting for a password.
Using piping avoids this issue all together.

For additional information, see:
 * fish-shell/fish-shell#1362
 * fish-shell/fish-shell#3922
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment