Asynchronous suggestions

[ericfreese]

Uses zpty to fetch suggestions asynchronously.

Work in progress. Based on mafredri's zsh-async.

See issue #170 and previous PR #134.

TODO

• Support older zsh versions (5.0.x) where zpty doesn't give a file descriptor.
• Support both sync and async modes via config option for environments where zpty doesn't work (see zpty errors sindresorhus/pure#117)
• Update/rewrite broken tests.
• Fix suggestion appearing intermittently in mc
• Fix suggestions failing to become invalidated
• Fix stderr output showing up in suggestions.
• Look into processes running in zpty receiving SIGHUP when any zpty is deleted.
• Find workaround for zpty -d killing all previously created zpty commands (or if no workaround, default to synchronous suggestions for now).

[balta2ar]

@ericfreese ping?

[ericfreese]

@balta2ar Still alive, but I've been spending a lot of time on other projects lately. Still intending to get to this when I have more time.

[z0rc]

Ping?

[sindresorhus]

// @mafredri

[mafredri]

I was summoned so I added some comments, hope that's cool 😛.

Is there any particular reason for not using zsh-async here, btw? You would get the older zsh support for free. Either way, if there's any improvements I can make to zsh-async that would make you consider it here, let me know.

Also, this project looks interesting. Going to have to try it out 👍!

[mafredri]

It would be safer here to kill -KILL %1 &>/dev/null. There isn't a big risk for $last_pid colliding with a new, existing process, but a tiny chance nonetheless. With the above command you guarantee that no other process than the child gets killed since zpty has job control, just like a regular shell.

[mafredri]

Might I suggest feature detection? Takes care of supporting both sync/async, keeps the code simple and doesn't require a config option.

[mafredri]

Using zpty -r to read the FD here would allow supporting older versions of zsh e.g. through kill-signals. Doesn't support delimiter, but you can use a PATTERN as the last parameter, e.g. zpty -rt $ZSH_AUTOSUGGEST_PTY_NAME suggestion '*'$'\0'. I've not used pattern before, though, so I could be wrong on the syntax.

[ericfreese]

So I'm playing with zpty -r over here, and I'm seeing some non-printable characters showing up in my output.

You ever seen this? I can probably remove them with a regex or something, but I don't quite understand where these chars are coming from.

[ericfreese]

Was able to get rid of the carriage return by running stty -onlcr in the pty

[z0rc]

Is there any particular reason for not using zsh-async here, btw?

See #134 (comment)
Personally I believe it's okay to depend on zsh-async though.

[mafredri]

Thanks @z0rc. I think it makes sense seeing as z-asug has quite simple requirements. Benefits of zsh-async only start to show when you require more advanced job control (or need to support older versions of zsh).

[mafredri]

Support older zsh versions (5.0.x) where zpty doesn't give a file descriptor.

@ericfreese: Remembered you had this goal here, so I thought I'd inform you I've figured out a way to do this in mafredri/zsh-async#13. So far it has worked reliably for me on a bunch of different systems and versions of zsh.

[ericfreese]

Thanks for all the notes here @mafredri! I have been busy with holidays and other projects for a while but I'm hoping to get back to this soon. I've just started implementing high-level integration tests using tmux and RSpec (see tmux_integration_testing branch) to hopefully have a more robust test framework in place before continuing this async work. Will definitely dig into your comments more as I get back into this 😄

[mafredri]

@ericfreese you're welcome! Just a FYI, zpty also works very well for integration testing and allows you to stay completely within shell (e.g. no need for ruby dependency), for an example you can look at the zsh-async zle test, inspired by zsh comptest. Not telling you how to write your tests though, so keep on doing it as best fits you 😄 .

[ericfreese]

@mafredri

Maybe the kill could be changed to kill -KILL -$$ so that the entire process group is informed, or perhaps a slightly more graceful version: kill -HUP -$$; kill -KILL $$.

I don't think this is necessary. A little testing on my system shows sending SIGKILL to the process group leader effectively terminates all the processes in the group. I think I'll leave this out for now, but keep it in the back of my mind if we run into issues where this might help in the future.

Out of curiosity, what problems did you encounter with this approach?

Honestly... I don't remember now that it's been a couple weeks since I made that comment. Everything seems to work fine now.

I've also addressed silencing stderr output from the zpty server process. Mind taking another look and verifying that it is indeed fixed?

---

@z0rc

I wasn't able to reproduce the mc issue you were seeing, but I've pushed up a lot of changes since then. Would you mind trying the newest version to see if the problem persists?

---

@ivan

The suggestions should work much better now. Please try the newest version and let me know if it solves the problems you were seeing earlier.

---

I think we're getting pretty close here! Would love another round of feedback 🙏

@PythonNut, you might also be interested in checking this out.

[z0rc]

Well, with this branch the git status in pure prompt is broken. It uses zsh-async for fetching all git information and puts it in placeholder. When I switched to this branch the placeholder is always empty. Sorry, but I think I'll be sticking with z-asug async fork for a while.

I'm on macOS 10.12 and zsh 5.3.1 installed via brew if this matters.

I don't want to be rude, but given amount of code duplication and hacks in place I believe it would be more viable to use zsh-async rather than reimplementing the wheel here.

[mafredri]

@z0rc that pure fork has an old version of zsh-async tho, have you tried using a more recent version?

[z0rc]

@mafredri I'm not using async.zsh from that fork and load it directly from https://github.com/mafredri/zsh-async.

[ericfreese]

@z0rc Thank you for the feedback, but I do think we're very close here. I've fixed the issue you reported with the pure prompt. I'd ask that you bear with me and try it again, but I understand if you're no longer interested, and I can stop leaning on you for testing if you'd like.

@mafredri In case you're curious, I had to also add the zshexit hack to the zpty created during feature detection c4bfd8e. The tests were also not catching it because they were not testing zptys created before sourcing the plugin.

Edit: Was also able to reproduce and fix the issue with mc.

[ericfreese]

Went ahead and merged this into develop. We can continue testing and adding fixes on that branch.

[PythonNut]

@ericfreese this seems to break my own usage of zsh-async. Any idea what's going on?

[ericfreese]

@PythonNut Can you verify the commit sha that you're using? There is a bug in zpty that we're trying to work around, but the workaround may not be working right. Would be helpful to have more details on what exactly is breaking for you if possible.

[mafredri]

@ericfreese interestingly, I was fine on 06fca77 but after testing develop (4321fc0), it's not behaving well with Pure / zsh-async.

Also ran into this after a while:

_zsh_autosuggest_async_pty_create:zpty:12: pty command name already used: zsh_autosuggest_pty
_zsh_autosuggest_async_pty_create:zle:22: Bad file descriptor number for -F: _zsh_autosuggest_async_response
_zsh_autosuggest_async_pty_create:zpty:12: pty command name already used: zsh_autosuggest_pty
_zsh_autosuggest_async_pty_create:zle:22: Bad file descriptor number for -F: _zsh_autosuggest_async_response

[mafredri]

Actually, a bisect shows 4321fc0 to be the offending commit, which seems weird.

[ericfreese]

@mafredi You rock, thanks for the bisect. I was also intermittently seeing the pty command name already used errors and pushed a few changes up to develop to reduce the amount of stuff we were doing on every precmd. For example, we don't need to run feature detection every precmd, and it was probably a mistake to run async_start every precmd too. Hope that helps.

[balta2ar]

I've experienced the same issue with z. It's definitely a bug/race-condition in z

@mafredri Are you still using z? Did you manage to come up with something about that? I ended up making regular backups of my ~/.z and adding functions to manually restore it. Of course, this is silly and it's not solving the root cause, but at least I can quickly restore ~/.z.

[mafredri]

@ericfreese I noticed another issue where zsh-asug still can interfere with other zptys, even with the zshexit hook.

What I think can happen is that zsh-asug creates and deletes it's zpty in rapid succession, causing the HUP to go through to other zptys.

Example:

diff --git a/zsh-autosuggestions.zsh b/zsh-autosuggestions.zsh
index 34ca080..dc5f7c2 100644
--- a/zsh-autosuggestions.zsh
+++ b/zsh-autosuggestions.zsh
@@ -529,6 +529,7 @@ _zsh_autosuggest_async_server() {
 		(
 			local suggestion
 			_zsh_autosuggest_strategy_$ZSH_AUTOSUGGEST_STRATEGY "$query"
+			sleep 0.5
 			echo -n -E "$suggestion"$'\0'
 		) &
 
@@ -589,6 +590,7 @@ _zsh_autosuggest_async_pty_destroy() {
 }
 
 _zsh_autosuggest_async_pty_recreate() {
+	sleep 0.5
 	_zsh_autosuggest_async_pty_destroy
 	_zsh_autosuggest_async_pty_create
 }

Here, the sleep inside _zsh_autosuggest_async_server allows me to easily trigger the issue with Pure/zsh-async.

The second sleep in _zsh_autosuggest_async_pty_recreate essentially fixes the issue, the other zptys are no longer HUPed.

Now I'm not 100% on the root issue, I'm just seeing the correlation here.

---

@balta2ar I do use z but I, as you, am living with it and backing up my .z =/. Haven't taken the time to properly investigate the issue.

[ericfreese]

Thanks @mafredri. I've added it to the todo list on #218.

[ericfreese]

@mafredri I was just trying this out and I'm not able to reproduce the issue.

I'm running pure 291574e and z-asug c52c428 with sleep 0.5 added to _zsh_autosuggest_async_server.

Do you have any more information on how you were able to reproduce this? A failing spec would be awesome (relevant spec is client_zpty_spec).

[mafredri]

@ericfreese are you inside a git folder with e.g. changes when testing?

When I have the sleep there, the preprompt is never updated with a * to indicate dirty status.

Should be even more easily visible / reproducible with sindresorhus/pure#273 because it relies on async even more.

[ericfreese]

are you inside a git folder with e.g. changes when testing?

Yeah I was manipulating a git repo, staging, committing, resetting, etc. Everything seemed to work as expected (including seeing the * to indicate dirty status). The output of zpty also showed that all zptys were still alive (not terminated).

Should be even more easily visible / reproducible with sindresorhus/pure#273 because it relies on async even more.

I'll try with this branch to see if I can reproduce.

Edit: Maybe has to do w/ OS or zsh version? Would you mind giving your specs and I can try reproducing in a VM?

[mafredri]

@ericfreese sorry, missed your edit. I can reproduce on macOS (10.12.3 (16D32)) with zsh 5.3.1 from Homebrew, but not with the current HEAD (which I suspect is expected, due to the fix).

[mafredri]

@ericfreese here's a recording demonstrating the issue.

I do have other plugins as well, e.g. zsh-syntax-highlighting, zsh-history-substring-search, but I can easily reproduce without them.