Skip to content
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

Aborting a completion attempt leaves set -o noglob in effect #786

Open
3 tasks done
abbeyj opened this issue Aug 8, 2022 · 6 comments
Open
3 tasks done

Aborting a completion attempt leaves set -o noglob in effect #786

abbeyj opened this issue Aug 8, 2022 · 6 comments

Comments

@abbeyj
Copy link

abbeyj commented Aug 8, 2022

Describe the bug

Certain completions turn off globbing with set -o noglob, perform some operation, and then attempt to reset globbing back to its original state. See for example

reset=$(shopt -po noglob)
set -o noglob
toks=($(compgen -d -- "${cur-}"))
IFS=' '
$reset
IFS=$'\n'
but this pattern is used in more than one place.

Unfortunately if the user presses Ctrl+C in the middle of this then the reset code is never run and the shell is left with set -o noglob in effect. This will likely be very disconcerting for the user. For instance, running ls * reports ls: cannot access *: No such file or directory. That's the message you'd normally expect to see if the directory is empty but now you'll see it reported for all directories, giving the impression that all of your files have been deleted.

A user could recover from this by running set +o noglob but they'd have to realize what was wrong first. I'm assuming in most cases people will just think their terminal is "broken" and be forced to close it and open a new one.

See also #691 .

To reproduce

You'll need a directory somewhere that's very slow to enumerate. One option would be to mount an NFS directory from some slow, distant machine. Another would be to create a local directory with an absurdly large number of files in it and have a cold cache (possibly something like echo 3 > /proc/sys/vm/drop_caches might help on Linux). Or maybe https://serverfault.com/a/954175. I can personally reproduce this with a large directory on NFS.

# Verify that globbing is turned on to start
$ shopt -po noglob
set +o noglob

$ cd /path/to/your/directory/<TAB><Ctrl+C>

# Globbing is now unexpectedly turned off
$ shopt -po noglob
set -o noglob

Expected behavior

The noglob setting should retain the previous value.

Versions (please complete the following information)

  • Operating system name/distribution and version: CentOS Linux release 7.9.2009 (Core)
  • bash version, echo "$BASH_VERSION": 4.2.46(2)-release
  • bash-completion version, (IFS=.; echo "${BASH_COMPLETION_VERSINFO[*]}"): This command outputs "2.11.0", but I'm actually using git commit 36ceb27.

Additional context

Maybe this could be handled by running the compgen in a subshell so that the setting would not have to be remembered and then explicitly restored?

Debug trace

I'm going to avoid including entire thing for now since it is large and I don't think it will help explain the problem. If you still need it, please tell me and I'll send the full one. It ends with this, the compgen having been interrupted by Ctrl+C:

+ local -a toks
+ local reset arg=-d
+ [[ -d == -d ]]
++ shopt -po noglob
+ reset='set +o noglob'
+ set -o noglob
+ toks=($(compgen -d -- "${cur-}"))
++ compgen -d -- /path/to/your/directory/
^C
$

I can't reproduce the problem on a different system that uses bash 5.1. I believe that's because of a recent change to bash, introduced in v5.1, that masks SIGINT during the execution of compgen: https://github.com/bminor/bash/blob/9439ce094c9aa7557a9d53ac7b412a23aa66e36b/subst.c#L6542. This makes the code in bash-completion work as expected but has the downside that you can't abort the completion and have to wait for it to finish no matter how long it takes.

@scop
Copy link
Owner

scop commented Aug 20, 2022

Hmph, yeah, I can see how that would happen.
I wonder if a simple trap "$reset" RETURN or a derivative would be a pattern we could use to avoid this.

Semi-related: #739

@akinomyoga
Copy link
Collaborator

I wonder if a simple trap "$reset" RETURN or a derivative would be a pattern we could use to avoid this.

I have thought the same thing after I first read this issue and tried it, but RETURN doesn't seem to be invoked when the completion is canceled by SIGINT. Currently, I don't come up with a good solution for this issue.

  • The subshells suggested by @abbeyj would have additional fork costs, and also it makes it hard to change the state of the parent shell e.g. when a completion function wants to cache the completion results, etc.
  • trap '$reset' INT might work, but the implementation would become complicated including saving/restoring of the original INT traps. Also, it only solves the problem for SIGINT; it doesn't solve the problem for the general signal.
  • In ble.sh, I turn off all the signals caused by user inputs by changing the terminal settings by stty.
  • Another solution might be to give up immediately restoring the state, but to restore the state on the next call of the completion. We record the state of whether we are currently adjusting the shell options or not in a global variable. We can check that variable at the start of the completion.
    We may also check the state on the execution of user commands by setting the DEBUG trap as bash-preexec does, but I'd say it's a fragile hack and hard to implement it in a transparent way to other configurations and user commands.

@scop
Copy link
Owner

scop commented Aug 20, 2022

RETURN doesn't seem to be invoked when the completion is canceled by SIGINT

Would trap '$reset' INT RETURN "fix" the general signal part of this problem? Managing original traps to those would still be needed I guess.

give up immediately restoring the state, but to restore the state on the next call of the completion

I'm not sure I follow, but wouldn't this leave the altered state in effect for the user's shell until the next call of the completion?

@akinomyoga
Copy link
Collaborator

RETURN doesn't seem to be invoked when the completion is canceled by SIGINT

Would trap '$reset' INT RETURN "fix" the general signal part of this problem? Managing original traps to those would still be needed I guess.

I think the problem with recovering the options could be solved, but then there arises another problem of the completion that can take a long time not being able to be canceled. Specifically, OP describes the case where the filename completion is attempted in a directory with many files on remote filesystems.

sidenote: implementation in ble.sh

I now remembered that I have implemented in ble.sh more complicated processing for this issue of slow filename completions. I attempt the pathname expansions (aka glob expansions) in a background subshell, and wait for its termination or user inputs in the parent shell. The subshell is killed when there are any user inputs before its termination. In this case, the result needs to be passed to the parent shell after the completion of the pathname expansions. Here, the next problem is how to effectively pass the expansion results to the parent shell. It turned out that result=($very_long_list) can be as slow as the expansion itself and this cannot be canceled when we suppress the signals. I instead parse the result of declare -p result by awk. write a NUL-separated list of result elements to file, and read it using mapfile in the latest version of Bash. Adjustments are needed for older versions of Bash where mapfile doesn't support an option -d '' for the NUL delimiter.

Other completions that might take a long time are handled in a similar way separately for each case.

give up immediately restoring the state, but to restore the state on the next call of the completion

I'm not sure I follow, but wouldn't this leave the altered state in effect for the user's shell until the next call of the completion?

Right. So this is merely a partial solution in case all the other solutions are unavailable.

@akinomyoga
Copy link
Collaborator

akinomyoga commented Aug 21, 2022

It turned out that the ERR trap seems to be reliably called in Bash 4.4+, though it doesn't work in Bash 4.2 and 4.3.

$ bash-dev --norc
$ function f1 { trap 'echo f1/ERR:$FUNCNAME' ERR; f2; echo never; false; }; function f2 { sleep 1; }; f1
^Cf1/ERR:f1

$ exit
$ bash-4.4 --norc
$ function f1 { trap 'echo f1/ERR:$FUNCNAME' ERR; f2; echo never; false; }; function f2 { sleep 1; }; f1
^Cf1/ERR:f1

$ exit
$ bash-4.3 --norc
$ function f1 { trap 'echo f1/ERR:$FUNCNAME' ERR; f2; echo never; false; }; function f2 { sleep 1; }; f1
^C
$

edit: Anyway, the user's ERR trap needs to be saved and restored.

@akinomyoga
Copy link
Collaborator

I think another way to make sure to recover the original settings is to recheck them before running the user commands.

  • For example, this can be done by the DEBUG trap as bash-preexec.sh does. A possible problem with this approach is that a robust implementation could be very complicated as we observe in bash-preexec.sh. Also, it can conflict or interfere with the other DEBUG trap set by bash-preexec.sh, Starship, etc. Another thing is that it may slow down the execution of the user commands because the DEBUG trap is invoked for every user command at the top level.
  • Another approach is to send a signal from the subshell in PS0 to the main shell as suggested in bash 4.4: using $PS0 rcaloras/bash-preexec#28 (comment). I'm not sure how much this is robust. tycho-kirchner/shournal seems to use PS0 for an approximate preexec, but it doesn't use a signal to do works in the main shell. This PS0 approach can be used in Bash 4.4+ but is not available in Bash 4.2 and 4.3. A possible conflict could occur in the PS0 value and the choice of the signal.
  • Another approach is to hook into the readline keybindings to accept-line, operate-and-next, and edit-and-execute as I described in bash-completion pollutes $_ #901 (comment). This could also conflict with the user's settings, but we can list the user's keybindings to accept-line, etc. by bind -p and modify them.

Note: All of these approaches might be broken by the user's other settings overwriting DEBUG, PS0, and keybindings.

We might combine one of the above with #739 _comp_finalize. We might also handle $_ in #901 with the same approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants