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

[ctrl-C] fails to terminate job when `read` is involved #4238

Closed
silvio opened this Issue Jul 24, 2017 · 9 comments

Comments

Projects
None yet
4 participants
@silvio
Copy link

silvio commented Jul 24, 2017

fish version: 2.6.0
uname: Linux xxx 4.11.7-041107-generic #201706240231 SMP Sat Jun 24 06:33:20 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
TERM: screen-256color (tmux)

The problem occurs with a plain shell too.

I use a inotifywait-while-end call for cyclic compiling (no differences between the used build system here cargo but same problem with make):

inotifywait -mrq -e close_write Cargo.toml src/ libgerrit/ --format "%w/%f" | while read CHANGEDFILE
    clear
    cargo check; and cargo build
    echo "DONE"
end

I cannot stop the program via STRG+C.

@silvio silvio changed the title Problem to ckill inotifywait with fish 2.6.0 Problem to sigkill via STRG+C inotifywait with fish 2.6.0 Jul 24, 2017

@faho

This comment has been minimized.

Copy link
Member

faho commented Jul 24, 2017

The "inotifywait" and cargo are unnecessary here. Any external command piped to read seems to do.

cat | while read line; echo $line; end

exhibits the same issue (well, after you enter something - I think cat quits if it immediately gets a ctrl-c).

I'm unsure of the root cause.

@faho faho added this to the fish-future milestone Jul 24, 2017

@faho faho added the bug label Jul 24, 2017

@faho

This comment has been minimized.

Copy link
Member

faho commented Jul 24, 2017

I think cat quits if it immediately gets a ctrl-c

It actually doesn't.

Which leads me to a theory...

@silvio: Can you try your command and quit before the inotifywait prints the first line?

I'm guessing the issue is that the first read quits, and then the second starts and some of our process group nonsense occurs - see #3952 and related issues.

@faho

This comment has been minimized.

Copy link
Member

faho commented Jul 24, 2017

I'm guessing the issue is that the first read quits, and then the second starts and some of our process group nonsense occurs

Yep, that's it.

The issue is, as soon as the first read quits, fish returns control of the terminal to itself.

This is easy enough to work around (so trivial in fact that I can write the code), but I'm unsure if that is something we want to keep in the code. There's numerous problems with our mapping of processes to jobs, which should be fixed properly.

@krader1961 krader1961 changed the title Problem to sigkill via STRG+C inotifywait with fish 2.6.0 [ctrl-C] fails to terminate job when `read` is involved Jul 24, 2017

@krader1961

This comment has been minimized.

Copy link
Contributor

krader1961 commented Jul 24, 2017

Pretty close to 100% certain this is a duplicate of #3805.

@krader1961 krader1961 marked this as a duplicate of #3805 Jul 24, 2017

@krader1961 krader1961 closed this Jul 24, 2017

@krader1961 krader1961 removed this from the fish-future milestone Jul 24, 2017

@krader1961 krader1961 added duplicate and removed bug labels Jul 24, 2017

@faho

This comment has been minimized.

Copy link
Member

faho commented Jul 24, 2017

I'm pretty sure it's not. It's another piece of the job control/process group pile of REDACTED, but not quite the same.

As I said, I have something that fixes this:

commit 8c02fc42f5189883ee83ef26bf908d9b1da0eda0
Author: Fabian Homborg <FHomborg@gmail.com>
Date:   Mon Jul 24 18:50:00 2017 +0200

    Only return control of term if all foreground jobs quit
    
    Fixes #4238.

diff --git a/src/proc.cpp b/src/proc.cpp
index 063f01fe..3e24d3d8 100644
--- a/src/proc.cpp
+++ b/src/proc.cpp
@@ -970,7 +970,17 @@ void job_continue(job_t *j, bool cont) {
 
         // Put the shell back in the foreground.
         if (j->get_flag(JOB_TERMINAL) && j->get_flag(JOB_FOREGROUND)) {
-            terminal_return_from_job(j);
+            // HACK: Only return if this is the last foreground job
+            job_iterator_t jobs;
+            bool have_job = false;
+            for (auto job = jobs.next(); job; job = jobs.next()) {
+                if (job != j && job->get_flag(JOB_TERMINAL) && job->get_flag(JOB_FOREGROUND)) {
+                    have_job = true;
+                }
+            }
+            if (have_job == false) {
+                terminal_return_from_job(j);
+            }
         }
     }
 }

The problem is that, when fish starts cat | while read, it starts one job with two processes - the cat, and the read. These are in the same process group, while the parent fish is in another. This would be the case even if fish didn't insist (for some reason) on acquiring its own pgroup on start.

Now, when the cat sends over the first line, the read is done and quits. This causes fish to execute its weirdly named job_continue function, and acquire control over the terminal again (terminal_return_from_job()). This causes the cat to lose control, so it won't receive any input anymore.

As you see above, what my patch does is only do the returning if all jobs that want control over the terminal have finished. The proper solution would probably not consider the job finished in the first place. The second read would take the place of the first (in the same job) and it would just continue.

Does that make any sense?

@krader1961

This comment has been minimized.

Copy link
Contributor

krader1961 commented Jul 24, 2017

It's another piece of the job control/process group pile of REDACTED, but not quite the same.

Right. When I said it's a duplicate of #3805 I didn't mean to imply the broken behavior (i.e., the symptom) was the same only that the underlying reason for both problems likely has the same root cause: our weird job control management.

I'm nervous about making piecemeal changes. I took a quick glance at your change and it looks okay as does your reasoning about the problem. So it's probably worth turning into a PR and asking people to build and test it on their systems.

@krader1961 krader1961 reopened this Jul 24, 2017

@krader1961 krader1961 added bug and removed duplicate labels Jul 24, 2017

@krader1961 krader1961 added this to the fish-future milestone Jul 24, 2017

faho added a commit to faho/fish-shell that referenced this issue Jul 25, 2017

@faho

This comment has been minimized.

Copy link
Member

faho commented Jul 25, 2017

@silvio: Please try #4242.

faho added a commit to faho/fish-shell that referenced this issue Jul 25, 2017

@silvio

This comment has been minimized.

Copy link

silvio commented Jul 25, 2017

hi @faho , I have tested your #4242 with commit "586166b75512339b9a5366fc80477db3b0cddabf". The behaviour has changed but has not repaired the problem.

I have slightly adopted my testcase to give you a test with out cargo tools in your hand. Its now usable to work in the fish-shell repository. Start it and do a touch somewhere in src/.

inotifywait -mrq -e close_write src/ --format "%w/%f" | while read CHANGEDFILE
   clear
   make
   echo "DONE"
end

The new behavior is:

  • before the first inotifywait is over, a STRG+C kills the inotifywait without problems
  • after inotifywait has ended and the while block starts one STRG+C doesn't break the execution, but two of them will stop the current process/while block execution
  • if the called while block is over and we are again in the inotifywait spin. No STRG+C will break anything. But when we run again the while loop it stops immediately
  • When I execute inotifywait -mrq -e close_write src/ --format "%w/%f" without the while part the STRG+C works. (has not changed)
@faho

This comment has been minimized.

Copy link
Member

faho commented Jul 25, 2017

after inotifywait has ended

Note: Technically, it does not end. inotifywait continues running.

Okay, so the issue there is that make is run in a separate job that also gets control of the terminal. After it has ended, we still return control to fish, not the (still running) inotifywait.

This is going to need some major reworking - my gut still tells me that the entire thing should be one job (including make) and hence one process group.

mqudsi added a commit to mqudsi/fish-shell that referenced this issue Aug 21, 2017

Fixes job handling involving functions and terminal control
While the idiomatic fix to fish' myriad of job control issues would be
to parse all functions prior to beginning the job pipeline so that
everything in the command line can be executed in the context of a
single job, that would require a huge effort to rewrite the core job
flow in fish and does not make sense at this time.

Instead, this patch fixes fish-shell#3952 and fish-shell#206 (but notably not fish-shell#4238) by
having jobs that are part of a single command pipeline, including those
that are functions executing external commands, use the same process
group. This prevents a (parent|child) from crashing with SIGTTIN or
hanging at SIGTTOU because it has a different process group than the
process currently in control of the terminal.

Additionally, since this fix involves removing the code that forces fish
to run in its own process group (which IMHO never made sense, as job
control is the job of the shell, not the process being launched), it
also fixes fish-shell#3805 and works around BashOnWindows#1653.

mqudsi added a commit to mqudsi/fish-shell that referenced this issue Sep 8, 2017

Fixes job handling involving functions and terminal control
While the idiomatic fix to fish' myriad of job control issues would be
to parse all functions prior to beginning the job pipeline so that
everything in the command line can be executed in the context of a
single job, that would require a huge effort to rewrite the core job
flow in fish and does not make sense at this time.

Instead, this patch fixes fish-shell#3952 and fish-shell#206 (but notably not fish-shell#4238) by
having jobs that are part of a single command pipeline, including those
that are functions executing external commands, use the same process
group. This prevents a (parent|child) from crashing with SIGTTIN or
hanging at SIGTTOU because it has a different process group than the
process currently in control of the terminal.

Additionally, since this fix involves removing the code that forces fish
to run in its own process group (which IMHO never made sense, as job
control is the job of the shell, not the process being launched), it
also fixes fish-shell#3805 and works around BashOnWindows#1653.

mqudsi added a commit to mqudsi/fish-shell that referenced this issue Sep 8, 2017

Assign terminal to unfinished foreground jobs over shell
When a foreground job that had control of the terminal has completed,
assign control of the terminal to the next foreground job in the
pipeline (if any) prior to giving control of the terminal back to the
shell.

This is similar to @faho's fix for fish-shell#4238 but will handle situations
where the next job in the chain is not in the same process group (so
commands executed by functions, etc can take control of the terminal,
too).

Fixes fish-shell#4238. Testing obviously needed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment