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

`read` builtin doesn't respect ctrl-C #516

Closed
JanKanis opened this Issue Jan 13, 2013 · 11 comments

Comments

Projects
None yet
2 participants
@JanKanis
Member

JanKanis commented Jan 13, 2013

Like any command, read should die if it gets a ctrl-C

@JanKanis

This comment has been minimized.

Show comment
Hide comment
@JanKanis

JanKanis Jan 14, 2013

Member

example: try this to permanently lock your shell:

while test 1; read; end

All commands are builtins, so the window where your ctrl-C has to arrive to stop the loop is reduced to essentially zero.

Member

JanKanis commented Jan 14, 2013

example: try this to permanently lock your shell:

while test 1; read; end

All commands are builtins, so the window where your ctrl-C has to arrive to stop the loop is reduced to essentially zero.

@ridiculousfish

This comment has been minimized.

Show comment
Hide comment
@ridiculousfish

ridiculousfish Jan 19, 2013

Member

This looks to be mighty complicated to fix.

Member

ridiculousfish commented Jan 19, 2013

This looks to be mighty complicated to fix.

@JanKanis

This comment has been minimized.

Show comment
Hide comment
@JanKanis

JanKanis Jan 20, 2013

Member

I've created a fix for this on branch bug-read-ctrlC. I haven't merged it yet so you/everyone can have a look at it and verify I didn't miss anything important. read on the branch seems to show the same behavior as bash's read.

Member

JanKanis commented Jan 20, 2013

I've created a fix for this on branch bug-read-ctrlC. I haven't merged it yet so you/everyone can have a look at it and verify I didn't miss anything important. read on the branch seems to show the same behavior as bash's read.

@ridiculousfish

This comment has been minimized.

Show comment
Hide comment
@ridiculousfish

ridiculousfish Jan 21, 2013

Member

Reviewing...It seems simpler than my approach, which was to try to handle it in the big switch statement inside reader_readline...I could not work out exactly whether and how control-C was communicated back as a read character.

Cutting the big knot by tweaking the signal handler is attractive. However, there are a few other cases where we want to check for (and clear) the interrupt flag, without causing the reader to exit: wildcard expansions, history search, and possibly more in the future. Look for other calls to reader_interrupted. We should convince ourselves that these don't apply, or ensure that canceling a wildcard expansion doesn't anything else.

I'd also suggest calling this property something other than "interruptible." It's closer to reader_exits_when_interrupted.

Member

ridiculousfish commented Jan 21, 2013

Reviewing...It seems simpler than my approach, which was to try to handle it in the big switch statement inside reader_readline...I could not work out exactly whether and how control-C was communicated back as a read character.

Cutting the big knot by tweaking the signal handler is attractive. However, there are a few other cases where we want to check for (and clear) the interrupt flag, without causing the reader to exit: wildcard expansions, history search, and possibly more in the future. Look for other calls to reader_interrupted. We should convince ourselves that these don't apply, or ensure that canceling a wildcard expansion doesn't anything else.

I'd also suggest calling this property something other than "interruptible." It's closer to reader_exits_when_interrupted.

@JanKanis

This comment has been minimized.

Show comment
Hide comment
@JanKanis

JanKanis Jan 21, 2013

Member

I was initially thinking along the same lines as you as well, but studying how interrupts were handled brought me to this simpler approach.

Note that reader_readline is not called by the interrupt handler itself, it's called in normal context after a select returns EINTR. The only change to interrupt handler code is

-   if (!is_interactive_read)
+   if (!is_interactive_read || get_interruptible())

and the addition of get_interruptible(). This is necessary as we still want to skip all parser blocks if we're in an interruptible read.

AFAIK (i.e. according to grep) the only other client of reader_interrupted is in wildcard.cpp line 731. I haven't studied closely what it does as I'm not very familiar with the wildcard code. My impression was that the wildcard code gets called from the reader when running commands, and the reader_interrupted call does the same as when it is called from readb, when a select fails with EINTR. I checked with a breakpoint at the backtrace when wildcard_expand_internal is called, and my impression was correct. But it turned out that wildcard_expand_internal is also called on worker threads for (I guess) autocompletion. In that case the reader_interrupted call is not correct and not threadsafe. On the other hand, it wasn't correct or thread safe either without my modifications.

Member

JanKanis commented Jan 21, 2013

I was initially thinking along the same lines as you as well, but studying how interrupts were handled brought me to this simpler approach.

Note that reader_readline is not called by the interrupt handler itself, it's called in normal context after a select returns EINTR. The only change to interrupt handler code is

-   if (!is_interactive_read)
+   if (!is_interactive_read || get_interruptible())

and the addition of get_interruptible(). This is necessary as we still want to skip all parser blocks if we're in an interruptible read.

AFAIK (i.e. according to grep) the only other client of reader_interrupted is in wildcard.cpp line 731. I haven't studied closely what it does as I'm not very familiar with the wildcard code. My impression was that the wildcard code gets called from the reader when running commands, and the reader_interrupted call does the same as when it is called from readb, when a select fails with EINTR. I checked with a breakpoint at the backtrace when wildcard_expand_internal is called, and my impression was correct. But it turned out that wildcard_expand_internal is also called on worker threads for (I guess) autocompletion. In that case the reader_interrupted call is not correct and not threadsafe. On the other hand, it wasn't correct or thread safe either without my modifications.

@ridiculousfish

This comment has been minimized.

Show comment
Hide comment
@ridiculousfish

ridiculousfish Jan 21, 2013

Member

echo /** is an example of a wildcard expansion that would take a very long time, and that you can cancel with control-C. There's also one in handle_token_history - I guess if history search were slow. We might want to add more cases like this in the future (completions especially).

The only call to reader_interrupted that could cause the reader to exit is in input.cpp. I think we introduce a new function, something like reader_interrupted_while_awaiting_input, which would be the one that calls reader_exit. input.cpp would call the new one to possibly trigger exit. Other clients could call reader_interrupted if they want to support canceling a long-running operation (echo /**), but without triggering exit.

Other thoughts - we want to make sure that read returns failure if exited via control-C. funced is an example. If the user control-Cs out, funced should not save its edits.

The fact that wildcard expansion in background threads calls reader_interrupted is a serious problem and I'm really glad you noticed it. Please open a new issue for that - thanks!

Member

ridiculousfish commented Jan 21, 2013

echo /** is an example of a wildcard expansion that would take a very long time, and that you can cancel with control-C. There's also one in handle_token_history - I guess if history search were slow. We might want to add more cases like this in the future (completions especially).

The only call to reader_interrupted that could cause the reader to exit is in input.cpp. I think we introduce a new function, something like reader_interrupted_while_awaiting_input, which would be the one that calls reader_exit. input.cpp would call the new one to possibly trigger exit. Other clients could call reader_interrupted if they want to support canceling a long-running operation (echo /**), but without triggering exit.

Other thoughts - we want to make sure that read returns failure if exited via control-C. funced is an example. If the user control-Cs out, funced should not save its edits.

The fact that wildcard expansion in background threads calls reader_interrupted is a serious problem and I'm really glad you noticed it. Please open a new issue for that - thanks!

@JanKanis

This comment has been minimized.

Show comment
Hide comment
@JanKanis

JanKanis Jan 21, 2013

Member

Ok, I think I understand the desired behavior better now. As I see it, there are three cases:

  • The standard case where the reader is reading input. In this case, if the reader is interruptible we want to fail the read and also cancel any active shellscript stack frames. If the reader is not interruptible we want the old behavior of only clearing the commandline.
  • A user pressed a key to search for a history item or search for a token in history. If the search takes e.g. too long and a ^C arrives, we cancel the search but do not alter reader state.
  • A command with some kind of wildcard is executed, but an interrupt arrives while searching directories for wildcard expansion. Current behavior is to print a Could not expand string “<yourcommand>” error, as the interrupt makes the wildcard expansion return a generic error. More desirable behavior would probably be to print a specific error message, e.g. "interrupted" when in interactive mode, and to cancel the running script immediately when in noninteractive mode (currently in noninteractive mode interrupts are not processed during wildcard expansion).
Member

JanKanis commented Jan 21, 2013

Ok, I think I understand the desired behavior better now. As I see it, there are three cases:

  • The standard case where the reader is reading input. In this case, if the reader is interruptible we want to fail the read and also cancel any active shellscript stack frames. If the reader is not interruptible we want the old behavior of only clearing the commandline.
  • A user pressed a key to search for a history item or search for a token in history. If the search takes e.g. too long and a ^C arrives, we cancel the search but do not alter reader state.
  • A command with some kind of wildcard is executed, but an interrupt arrives while searching directories for wildcard expansion. Current behavior is to print a Could not expand string “<yourcommand>” error, as the interrupt makes the wildcard expansion return a generic error. More desirable behavior would probably be to print a specific error message, e.g. "interrupted" when in interactive mode, and to cancel the running script immediately when in noninteractive mode (currently in noninteractive mode interrupts are not processed during wildcard expansion).
@ridiculousfish

This comment has been minimized.

Show comment
Hide comment
@ridiculousfish

ridiculousfish Jan 22, 2013

Member

That matches my understanding too!

Member

ridiculousfish commented Jan 22, 2013

That matches my understanding too!

@JanKanis

This comment has been minimized.

Show comment
Hide comment
@JanKanis

JanKanis Jan 22, 2013

Member

Thanks for the comments. I've changed things a bit and I think it handles the different cases of interruption now. Please review. I've named the new function reader_reading_interrupted because "interrupted while awaiting input" or "interrupted while reading" could also sound like questions to me. I couldn't find a short enough name that fully describes what the function does ("tell reader to process interrupts and return whether there were any and b.t.w. the reader was currently waiting for input").

Member

JanKanis commented Jan 22, 2013

Thanks for the comments. I've changed things a bit and I think it handles the different cases of interruption now. Please review. I've named the new function reader_reading_interrupted because "interrupted while awaiting input" or "interrupted while reading" could also sound like questions to me. I couldn't find a short enough name that fully describes what the function does ("tell reader to process interrupts and return whether there were any and b.t.w. the reader was currently waiting for input").

@ridiculousfish

This comment has been minimized.

Show comment
Hide comment
@ridiculousfish

ridiculousfish Jan 22, 2013

Member

Reviewed, looks great! Thanks!

Member

ridiculousfish commented Jan 22, 2013

Reviewed, looks great! Thanks!

@JanKanis

This comment has been minimized.

Show comment
Hide comment
@JanKanis

JanKanis Feb 5, 2013

Member

(please ignore d9d8bfa, the correct one is 9a89da3)

Member

JanKanis commented Feb 5, 2013

(please ignore d9d8bfa, the correct one is 9a89da3)

hauleth added a commit to hauleth/fish-shell that referenced this issue Apr 13, 2013

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