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.

Sign up for GitHub

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

Jump to bottom

Fix bugs in async code #753

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix bugs in async code #753

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
104 changes: 59 additions & 45 deletions src/async.zsh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,49 +9,59 @@ _zsh_autosuggest_async_request() {
typeset -g _ZSH_AUTOSUGGEST_ASYNC_FD _ZSH_AUTOSUGGEST_CHILD_PID

# If we've got a pending request, cancel it
if [[ -n "$_ZSH_AUTOSUGGEST_ASYNC_FD" ]] && { true <&$_ZSH_AUTOSUGGEST_ASYNC_FD } 2>/dev/null; then
# Close the file descriptor and remove the handler
exec {_ZSH_AUTOSUGGEST_ASYNC_FD}<&-
zle -F $_ZSH_AUTOSUGGEST_ASYNC_FD

# We won't know the pid unless the user has zsh/system module installed
if [[ -n "$_ZSH_AUTOSUGGEST_CHILD_PID" ]]; then
# Zsh will make a new process group for the child process only if job
# control is enabled (MONITOR option)
if [[ -o MONITOR ]]; then
# Send the signal to the process group to kill any processes that may
# have been forked by the suggestion strategy
kill -TERM -$_ZSH_AUTOSUGGEST_CHILD_PID 2>/dev/null
if (( _ZSH_AUTOSUGGEST_CHILD_PID )); then
kill -TERM -- $_ZSH_AUTOSUGGEST_CHILD_PID 2>/dev/null
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see you've also removed the check of [[ -o MONITOR ]]. Was that intentional and if so can you give some reasoning for it? I had never fully tested the different cases there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The check for MONITOR is still there. It used to be performed before invoking kill, which isn't quite right. I moved it to the proper point (when forking). This makes a difference if the option is changed between forking and killing. I should've this change in the description.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Friendly ping.

_ZSH_AUTOSUGGEST_CHILD_PID=
fi

_ZSH_AUTOSUGGEST_ASYNC_FD=

{
# Fork a process to fetch a suggestion and open a pipe to read from it
exec {_ZSH_AUTOSUGGEST_ASYNC_FD}< <(
# Suppress error messages
exec 2>/dev/null

# Tell parent process our pid
if (( ${+sysparams} )); then
echo ${sysparams[pid]} || return
else
# Kill just the child process since it wasn't placed in a new process
# group. If the suggestion strategy forked any child processes they may
# be orphaned and left behind.
kill -TERM $_ZSH_AUTOSUGGEST_CHILD_PID 2>/dev/null
echo || return
fi
fi
fi

# Fork a process to fetch a suggestion and open a pipe to read from it
exec {_ZSH_AUTOSUGGEST_ASYNC_FD}< <(
# Tell parent process our pid
echo $sysparams[pid]
# Fetch and print the suggestion
local suggestion
_zsh_autosuggest_fetch_suggestion "$1"
echo -nE - "$suggestion"
) || return
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@romkatv Can you elaborate on the addition of the || return added here and after read and echo? In what situations can exec/read/echo have non-zero exit status?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose one could read the source code of the builtins exec/read/echo and find out which syscalls they rely on. Then read the source code or docs of all operating systems to figure out when those system calls can fail.

I think a better question is what the code should do when exec/read/echo fail. Which variant of this code do you think behaves better in case of errors?


# Fetch and print the suggestion
local suggestion
_zsh_autosuggest_fetch_suggestion "$1"
echo -nE "$suggestion"
)
# There's a weird bug here where ^C stops working unless we force a fork
# See https://github.com/zsh-users/zsh-autosuggestions/issues/364
autoload -Uz is-at-least
is-at-least 5.8 || command true

# There's a weird bug here where ^C stops working unless we force a fork
# See https://github.com/zsh-users/zsh-autosuggestions/issues/364
autoload -Uz is-at-least
is-at-least 5.8 || command true
# Read the pid from the child process
read _ZSH_AUTOSUGGEST_CHILD_PID <&$_ZSH_AUTOSUGGEST_ASYNC_FD || return

# Read the pid from the child process
read _ZSH_AUTOSUGGEST_CHILD_PID <&$_ZSH_AUTOSUGGEST_ASYNC_FD
# Zsh will make a new process group for the child process only if job
# control is enabled (MONITOR option)
if [[ -o MONITOR ]]; then
# If we need to kill the background process in the future, we'll send
# SIGTERM to the process group to kill any processes that may have
# been forked by the suggestion strategy
_ZSH_AUTOSUGGEST_CHILD_PID=-$_ZSH_AUTOSUGGEST_CHILD_PID
fi

# When the fd is readable, call the response handler
zle -F "$_ZSH_AUTOSUGGEST_ASYNC_FD" _zsh_autosuggest_async_response
# When the fd is readable, call the response handler
zle -F "$_ZSH_AUTOSUGGEST_ASYNC_FD" _zsh_autosuggest_async_response
} always {
# Clean things up if there was an error
if (( $? && _ZSH_AUTOSUGGEST_ASYNC_FD )); then
exec {_ZSH_AUTOSUGGEST_ASYNC_FD}<&-
_ZSH_AUTOSUGGEST_ASYNC_FD=
_ZSH_AUTOSUGGEST_CHILD_PID=
fi
}
}

# Called when new data is ready to be read from the pipe
Expand All @@ -61,16 +71,20 @@ _zsh_autosuggest_async_response() {
emulate -L zsh

local suggestion
if (( $1 == _ZSH_AUTOSUGGEST_ASYNC_FD )); then
_ZSH_AUTOSUGGEST_ASYNC_FD=
_ZSH_AUTOSUGGEST_CHILD_PID=
if [[ $# == 1 || $2 == "hup" ]]; then
# Read everything from the fd
IFS='' read -rd '' -u $1 suggestion
fi
fi

if [[ -z "$2" || "$2" == "hup" ]]; then
# Read everything from the fd and give it as a suggestion
IFS='' read -rd '' -u $1 suggestion
zle autosuggest-suggest -- "$suggestion"
# Always remove the handler and close the fd
zle -F $1
exec {1}<&-

# Close the fd
exec {1}<&-
if [[ -n $suggestion ]]; then
zle autosuggest-suggest -- "$suggestion"
fi

# Always remove the handler
zle -F "$1"
}
108 changes: 61 additions & 47 deletions zsh-autosuggestions.zsh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -764,49 +764,59 @@ _zsh_autosuggest_async_request() {
typeset -g _ZSH_AUTOSUGGEST_ASYNC_FD _ZSH_AUTOSUGGEST_CHILD_PID

# If we've got a pending request, cancel it
if [[ -n "$_ZSH_AUTOSUGGEST_ASYNC_FD" ]] && { true <&$_ZSH_AUTOSUGGEST_ASYNC_FD } 2>/dev/null; then
# Close the file descriptor and remove the handler
exec {_ZSH_AUTOSUGGEST_ASYNC_FD}<&-
zle -F $_ZSH_AUTOSUGGEST_ASYNC_FD

# We won't know the pid unless the user has zsh/system module installed
if [[ -n "$_ZSH_AUTOSUGGEST_CHILD_PID" ]]; then
# Zsh will make a new process group for the child process only if job
# control is enabled (MONITOR option)
if [[ -o MONITOR ]]; then
# Send the signal to the process group to kill any processes that may
# have been forked by the suggestion strategy
kill -TERM -$_ZSH_AUTOSUGGEST_CHILD_PID 2>/dev/null
else
# Kill just the child process since it wasn't placed in a new process
# group. If the suggestion strategy forked any child processes they may
# be orphaned and left behind.
kill -TERM $_ZSH_AUTOSUGGEST_CHILD_PID 2>/dev/null
fi
fi
if (( _ZSH_AUTOSUGGEST_CHILD_PID )); then
kill -TERM -- $_ZSH_AUTOSUGGEST_CHILD_PID 2>/dev/null
_ZSH_AUTOSUGGEST_CHILD_PID=
fi

# Fork a process to fetch a suggestion and open a pipe to read from it
exec {_ZSH_AUTOSUGGEST_ASYNC_FD}< <(
# Tell parent process our pid
echo $sysparams[pid]
_ZSH_AUTOSUGGEST_ASYNC_FD=

# Fetch and print the suggestion
local suggestion
_zsh_autosuggest_fetch_suggestion "$1"
echo -nE "$suggestion"
)

# There's a weird bug here where ^C stops working unless we force a fork
# See https://github.com/zsh-users/zsh-autosuggestions/issues/364
autoload -Uz is-at-least
is-at-least 5.8 || command true
{
# Fork a process to fetch a suggestion and open a pipe to read from it
exec {_ZSH_AUTOSUGGEST_ASYNC_FD}< <(
# Suppress error messages
exec 2>/dev/null

# Tell parent process our pid
if (( ${+sysparams} )); then
echo ${sysparams[pid]} || return
else
echo || return
fi

# Read the pid from the child process
read _ZSH_AUTOSUGGEST_CHILD_PID <&$_ZSH_AUTOSUGGEST_ASYNC_FD
# Fetch and print the suggestion
local suggestion
_zsh_autosuggest_fetch_suggestion "$1"
echo -nE - "$suggestion"
) || return

# There's a weird bug here where ^C stops working unless we force a fork
# See https://github.com/zsh-users/zsh-autosuggestions/issues/364
autoload -Uz is-at-least
is-at-least 5.8 || command true

# Read the pid from the child process
read _ZSH_AUTOSUGGEST_CHILD_PID <&$_ZSH_AUTOSUGGEST_ASYNC_FD || return

# Zsh will make a new process group for the child process only if job
# control is enabled (MONITOR option)
if [[ -o MONITOR ]]; then
# If we need to kill the background process in the future, we'll send
# SIGTERM to the process group to kill any processes that may have
# been forked by the suggestion strategy
_ZSH_AUTOSUGGEST_CHILD_PID=-$_ZSH_AUTOSUGGEST_CHILD_PID
fi

# When the fd is readable, call the response handler
zle -F "$_ZSH_AUTOSUGGEST_ASYNC_FD" _zsh_autosuggest_async_response
# When the fd is readable, call the response handler
zle -F "$_ZSH_AUTOSUGGEST_ASYNC_FD" _zsh_autosuggest_async_response
} always {
# Clean things up if there was an error
if (( $? && _ZSH_AUTOSUGGEST_ASYNC_FD )); then
exec {_ZSH_AUTOSUGGEST_ASYNC_FD}<&-
_ZSH_AUTOSUGGEST_ASYNC_FD=
_ZSH_AUTOSUGGEST_CHILD_PID=
fi
}
}

# Called when new data is ready to be read from the pipe
Expand All @@ -816,18 +826,22 @@ _zsh_autosuggest_async_response() {
emulate -L zsh

local suggestion
if (( $1 == _ZSH_AUTOSUGGEST_ASYNC_FD )); then
_ZSH_AUTOSUGGEST_ASYNC_FD=
_ZSH_AUTOSUGGEST_CHILD_PID=
if [[ $# == 1 || $2 == "hup" ]]; then
# Read everything from the fd
IFS='' read -rd '' -u $1 suggestion
fi
fi

if [[ -z "$2" || "$2" == "hup" ]]; then
# Read everything from the fd and give it as a suggestion
IFS='' read -rd '' -u $1 suggestion
zle autosuggest-suggest -- "$suggestion"
# Always remove the handler and close the fd
zle -F $1
exec {1}<&-

# Close the fd
exec {1}<&-
if [[ -n $suggestion ]]; then
zle autosuggest-suggest -- "$suggestion"
fi

# Always remove the handler
zle -F "$1"
}

#--------------------------------------------------------------------#
Expand Down