Clear async global variables on successful response

If _ZSH_AUTOSUGGEST_ASYNC_FD is not cleared, something else may open the file descriptor pointed to by it. And then the next call into _zsh_autosuggest_async_request will close it causing trouble. It seems like good practice to clean up _ZSH_AUTOSUGGEST_CHILD_PID as well, though it's not directly causing any known problems at the moment. I was able to produce errors with ZSH_AUTOSUGGEST_MANUAL_REBIND active and sourcing a file async-widget-setup.zsh after the first precmd with the plugin active. If manual rebind is not active or if the widgets are created before the first precmd, then zsh-autosuggestions wraps the widgets and for some reason we don't seem to get any fd conflicts. The .zshrc: ``` ZSH_AUTOSUGGEST_MANUAL_REBIND=true source zsh-autosuggestions.zsh ``` and async-widget-setup.zsh: ``` function async-widget-fork() { exec {fd}< <(echo foo) zle -M "opened: $fd, autosuggest fd: $_ZSH_AUTOSUGGEST_ASYNC_FD" } function async-widget-read() { zle -M "reading from $fd: $(cat <&$fd)" } zle -N async-widget-fork zle -N async-widget-read bindkey ^A async-widget-fork bindkey ^B async-widget-read ``` Then run `ZDOTDIR=$PWD zsh` and run `source async-widget-setup.zsh`. At the next prompt, type one character e.g. "a" to trigger an async request/response cycle. This leaves _ZSH_AUTOSUGGEST_ASYNC_FD set to the stale file descriptor number. Then press ^A to activate the fork. This will set the fd parameter to the same number as _ZSH_AUTOSUGGEST_ASYNC_FD. Then type another character e.g. "a" to trigger an async request. This will print a "No handler installed" error and close the file descriptor pointed to by both _ZSH_AUTOSUGGEST_ASYNC_FD and fd. Pressing ^B at this point will fail to read with a "bad file descriptor" error.
Avoid accessing unset param when zsh/system is not available
2025-12-06 23:30:39 +01:00 · 2023-08-25 22:05:57 -06:00 · 2023-08-25 16:29:02 -06:00 · 2023-08-25 16:29:02 -06:00 · 2023-08-25 16:28:18 -06:00 · 2023-08-25 16:27:29 -06:00
8 changed files with 114 additions and 83 deletions
--- a/.circleci/config.yml
+++ b/.circleci/config.yml
@@ -1,15 +0,0 @@
-version: 2
-jobs:
-  build:
-    parallelism: 4
-    shell: /bin/bash --login
-    docker:
-    - image: ericfreese/zsh-autosuggestions-test:latest
-    steps:
-    - checkout
-    - run:
-        name: Running tests
-        command: |
-          for v in $(grep "^[^#]" ZSH_VERSIONS | awk "(NR + $CIRCLE_NODE_INDEX) % $CIRCLE_NODE_TOTAL == 0"); do
-            TEST_ZSH_BIN=zsh-$v make test || exit 1
-          done
--- a/.github/workflows/integration.yml
+++ b/.github/workflows/integration.yml
@@ -0,0 +1,51 @@
+on: [push, pull_request]
+concurrency:
+  group: ${{ github.workflow }}-${{ github.ref }}
+  cancel-in-progress: true
+env:
+  IMAGE_CACHE_PATH: /tmp/.image-cache
+  IMAGE_CACHE_NAME: zsh-autosuggestions-test
+jobs:
+  determine-versions:
+    runs-on: ubuntu-22.04
+    outputs:
+      versions: ${{ steps.set-versions.outputs.versions }}
+    steps:
+    - uses: actions/checkout@v3
+    - id: set-versions
+      run: |
+        echo "versions=$(
+          grep "^[^#]" ZSH_VERSIONS \
+          | sed -E 's/(^|$)/"/g' \
+          | paste -sd ',' - \
+          | sed -e 's/^/[/' -e 's/$/]/'
+        )" >> $GITHUB_OUTPUT
+  test:
+    needs: determine-versions
+    runs-on: ubuntu-22.04
+    strategy:
+      matrix:
+        version: ${{ fromJson(needs.determine-versions.outputs.versions) }}
+    steps:
+    - uses: actions/checkout@v3
+    - name: Docker image cache
+      id: image-cache
+      uses: actions/cache@v3
+      with:
+        path: ${{ env.IMAGE_CACHE_PATH }}
+        key: image-cache-${{ matrix.version }}-${{ hashFiles('Dockerfile', 'install_test_zsh.sh', 'Gemfile.lock') }}
+    - name: Load cached docker image if available
+      if: ${{ steps.image-cache.outputs.cache-hit }}
+      run: gunzip < $IMAGE_CACHE_PATH/$IMAGE_CACHE_NAME.tar.gz | docker load
+    - name: Build the docker image if necessary
+      if: ${{ !steps.image-cache.outputs.cache-hit }}
+      run: |
+        docker build --build-arg TEST_ZSH_VERSION=${{ matrix.version }} -t $IMAGE_CACHE_NAME .
+        mkdir -p $IMAGE_CACHE_PATH
+        docker save $IMAGE_CACHE_NAME | gzip > $IMAGE_CACHE_PATH/$IMAGE_CACHE_NAME.tar.gz
+    - name: Run the tests
+      run: |
+        docker run --rm \
+          -v $PWD:/zsh-autosuggestions \
+          $IMAGE_CACHE_NAME \
+          make test
--- a/9
+++ b/9
@@ -1,5 +1,8 @@
 FROM ruby:2.5.3-alpine

+ARG TEST_ZSH_VERSION
+RUN : "${TEST_ZSH_VERSION:?}"
+
 RUN apk add --no-cache autoconf
 RUN apk add --no-cache libtool
 RUN apk add --no-cache libcap-dev
@@ -11,10 +14,8 @@ RUN apk add --no-cache tmux

 WORKDIR /zsh-autosuggestions

-ADD ZSH_VERSIONS /zsh-autosuggestions/ZSH_VERSIONS
-ADD install_test_zsh.sh /zsh-autosuggestions/install_test_zsh.sh
+ADD install_test_zsh.sh ./
 RUN ./install_test_zsh.sh

-ADD Gemfile /zsh-autosuggestions/Gemfile
-ADD Gemfile.lock /zsh-autosuggestions/Gemfile.lock
+ADD Gemfile Gemfile.lock ./
 RUN bundle install
--- a/README.md
+++ b/README.md
@@ -170,18 +170,16 @@ Tests are written in ruby using the [`rspec`](http://rspec.info/) framework. The

 Test files live in `spec/`. To run the tests, run `make test`. To run a specific test, run `TESTS=spec/some_spec.rb make test`. You can also specify a `zsh` binary to use by setting the `TEST_ZSH_BIN` environment variable (ex: `TEST_ZSH_BIN=/bin/zsh make test`).

-A docker image for testing is available [on docker hub](https://hub.docker.com/r/ericfreese/zsh-autosuggestions-test). It comes with ruby, the bundler dependencies, and all supported versions of zsh installed.
-
-Pull the docker image with:
+It's possible to run the tests for any supported version of zsh in a Docker image by building an image from the provided Dockerfile. To build the docker image for a specific version of zsh (where `<version>` below is substituted with the contents of a line from the [`ZSH_VERSIONS`](ZSH_VERSIONS) file), run:

 ```sh
-docker pull ericfreese/zsh-autosuggestions-test
+docker build --build-arg TEST_ZSH_VERSION=<version> -t zsh-autosuggestions-test .
 ```

-To run the tests for a specific version of zsh (where `<version>` below is substituted with the contents of a line from the [`ZSH_VERSIONS`](ZSH_VERSIONS) file):
+After building the image, run the tests via:

 ```sh
-docker run -it -e TEST_ZSH_BIN=zsh-<version> -v $PWD:/zsh-autosuggestions zsh-autosuggestions-test make test
+docker run -it -v $PWD:/zsh-autosuggestions zsh-autosuggestions-test make test
 ```


--- a/7
+++ b/7
@@ -1,9 +1,5 @@
 # Zsh releases to run tests against
 # See https://github.com/zsh-users/zsh/releases
-#
-# When modifying this file, rebuild and push docker image:
-#   $ docker build -t ericfreese/zsh-autosuggestions-test .
-#   $ docker push ericfreese/zsh-autosuggestions-test
 4.3.11
 5.0.2
 5.0.8
@@ -14,4 +10,5 @@
 5.5.1
 5.6.2
 5.7.1
-5.8
+5.8.1
+5.9
--- a/install_test_zsh.sh
+++ b/install_test_zsh.sh
@@ -2,19 +2,17 @@

 set -ex

-for v in $(grep "^[^#]" ZSH_VERSIONS); do
-  mkdir zsh-$v
-  cd zsh-$v
+mkdir zsh-build
+cd zsh-build

-  curl -L https://api.github.com/repos/zsh-users/zsh/tarball/zsh-$v | tar xz --strip=1
+curl -L https://api.github.com/repos/zsh-users/zsh/tarball/zsh-$TEST_ZSH_VERSION | tar xz --strip=1

 ./Util/preconfig
 ./configure --enable-pcre \
            --enable-cap \
            --enable-multibyte \
            --with-term-lib='ncursesw tinfo' \
-              --with-tcsetpgrp \
-              --program-suffix="-$v"
+            --with-tcsetpgrp

 make install.bin
 make install.modules
@@ -22,5 +20,4 @@ for v in $(grep "^[^#]" ZSH_VERSIONS); do

 cd ..

-  rm -rf zsh-$v
-done
+rm -rf zsh-build
--- a/src/async.zsh
+++ b/src/async.zsh
@@ -15,31 +15,20 @@ _zsh_autosuggest_async_request() {
 		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
+		if (( _ZSH_AUTOSUGGEST_CHILD_PID )); then
+			kill -TERM -- $_ZSH_AUTOSUGGEST_CHILD_PID 2>/dev/null
 		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]
+		# Tell parent process our pid if we can
+		echo ${sysparams[pid]:-}

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

 	# There's a weird bug here where ^C stops working unless we force a fork
@@ -50,6 +39,15 @@ _zsh_autosuggest_async_request() {
 	# 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:+-$_ZSH_AUTOSUGGEST_CHILD_PID}
+	fi
+
 	# When the fd is readable, call the response handler
 	zle -F "$_ZSH_AUTOSUGGEST_ASYNC_FD" _zsh_autosuggest_async_response
 }
@@ -60,17 +58,20 @@ _zsh_autosuggest_async_request() {
 _zsh_autosuggest_async_response() {
 	emulate -L zsh

+	typeset -g _ZSH_AUTOSUGGEST_ASYNC_FD _ZSH_AUTOSUGGEST_CHILD_PID
 	local suggestion

-	if [[ -z "$2" || "$2" == "hup" ]]; then
+	if [[ $# == 1 || "$2" == "hup" ]]; then
 		# Read everything from the fd and give it as a suggestion
 		IFS='' read -rd '' -u $1 suggestion
 		zle autosuggest-suggest -- "$suggestion"

 		# Close the fd
 		exec {1}<&-
+		_ZSH_AUTOSUGGEST_ASYNC_FD=
+		_ZSH_AUTOSUGGEST_ASYNC_PID=
 	fi

 	# Always remove the handler
-	zle -F "$1"
+	zle -F $1
 }
--- a/zsh-autosuggestions.zsh
+++ b/zsh-autosuggestions.zsh
@@ -770,31 +770,20 @@ _zsh_autosuggest_async_request() {
 		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
+		if (( _ZSH_AUTOSUGGEST_CHILD_PID )); then
+			kill -TERM -- $_ZSH_AUTOSUGGEST_CHILD_PID 2>/dev/null
 		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]
+		# Tell parent process our pid if we can
+		echo ${sysparams[pid]:-}

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

 	# There's a weird bug here where ^C stops working unless we force a fork
@@ -805,6 +794,15 @@ _zsh_autosuggest_async_request() {
 	# 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:+-$_ZSH_AUTOSUGGEST_CHILD_PID}
+	fi
+
 	# When the fd is readable, call the response handler
 	zle -F "$_ZSH_AUTOSUGGEST_ASYNC_FD" _zsh_autosuggest_async_response
 }
@@ -815,19 +813,22 @@ _zsh_autosuggest_async_request() {
 _zsh_autosuggest_async_response() {
 	emulate -L zsh

+	typeset -g _ZSH_AUTOSUGGEST_ASYNC_FD _ZSH_AUTOSUGGEST_CHILD_PID
 	local suggestion

-	if [[ -z "$2" || "$2" == "hup" ]]; then
+	if [[ $# == 1 || "$2" == "hup" ]]; then
 		# Read everything from the fd and give it as a suggestion
 		IFS='' read -rd '' -u $1 suggestion
 		zle autosuggest-suggest -- "$suggestion"

 		# Close the fd
 		exec {1}<&-
+		_ZSH_AUTOSUGGEST_ASYNC_FD=
+		_ZSH_AUTOSUGGEST_ASYNC_PID=
 	fi

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

 #--------------------------------------------------------------------#
Author	SHA1	Message	Date
Eric Freese	3391962a15	Clear async global variables on successful response If _ZSH_AUTOSUGGEST_ASYNC_FD is not cleared, something else may open the file descriptor pointed to by it. And then the next call into _zsh_autosuggest_async_request will close it causing trouble. It seems like good practice to clean up _ZSH_AUTOSUGGEST_CHILD_PID as well, though it's not directly causing any known problems at the moment. I was able to produce errors with ZSH_AUTOSUGGEST_MANUAL_REBIND active and sourcing a file async-widget-setup.zsh after the first precmd with the plugin active. If manual rebind is not active or if the widgets are created before the first precmd, then zsh-autosuggestions wraps the widgets and for some reason we don't seem to get any fd conflicts. The .zshrc: ``` ZSH_AUTOSUGGEST_MANUAL_REBIND=true source zsh-autosuggestions.zsh ``` and async-widget-setup.zsh: ``` function async-widget-fork() { exec {fd}< <(echo foo) zle -M "opened: $fd, autosuggest fd: $_ZSH_AUTOSUGGEST_ASYNC_FD" } function async-widget-read() { zle -M "reading from $fd: $(cat <&$fd)" } zle -N async-widget-fork zle -N async-widget-read bindkey ^A async-widget-fork bindkey ^B async-widget-read ``` Then run `ZDOTDIR=$PWD zsh` and run `source async-widget-setup.zsh`. At the next prompt, type one character e.g. "a" to trigger an async request/response cycle. This leaves _ZSH_AUTOSUGGEST_ASYNC_FD set to the stale file descriptor number. Then press ^A to activate the fork. This will set the fd parameter to the same number as _ZSH_AUTOSUGGEST_ASYNC_FD. Then type another character e.g. "a" to trigger an async request. This will print a "No handler installed" error and close the file descriptor pointed to by both _ZSH_AUTOSUGGEST_ASYNC_FD and fd. Pressing ^B at this point will fail to read with a "bad file descriptor" error.	2023-08-25 22:05:57 -06:00
Eric Freese	61257de667	Avoid accessing unset param when zsh/system is not available	2023-08-25 16:29:02 -06:00
Eric Freese	84c7cfaa89	Prevent parameter value being interpreted as kill flag	2023-08-25 16:29:02 -06:00
Eric Freese	6259d46d36	Move check for MONITOR directly after fork This should properly handle the edge case where the MONITOR option is changed between forking and killing.	2023-08-25 16:28:18 -06:00
Eric Freese	b350b900ce	Prevent suggestion being treated as an option for echo	2023-08-25 16:27:29 -06:00
Eric Freese	e386de0247	cleanup: Remove unnecessary/inconsistent quoting from fd arg	2023-08-25 16:27:29 -06:00
Eric Freese	2b730c9634	cleanup: More precisely check number of args instead of second arg value	2023-08-25 16:27:29 -06:00
Eric Freese	35ed23efe8	cleanup: Use simpler arithmetic expression	2023-08-25 16:27:28 -06:00
Eric Freese	9b0272944f	Add support for 5.9	2023-05-26 19:57:30 -06:00
Eric Freese	c5044edd48	Support latest minor version of 5.8	2023-05-26 19:57:14 -06:00
Eric Freese	2cc34c015e	Switch from Circle CI to GitHub Actions The testing docker image has been split up. Instead of having one image with all supported versions of zsh installed, we now have a separate image for each supported zsh version. We use GitHub Action matrices to run jobs in parallel for all of the supported versions. We no longer need to publish images to Docker Hub. The images are just built by CI (or developers) as needed from the Dockerfile in the repo.	2023-05-26 19:44:39 -06:00