A possibly needed workaround for <( ) process substitution #364

psprint · 2018-08-07T19:38:35Z

Hello,
in branch develop there's currently use of <( ... ) process substitution to do asynchronous calls:

zsh-autosuggestions/zsh-autosuggestions.zsh

Lines 717 to 725 in 681ffc7

    
           	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" 
        
           	)

I use the same technique in fast-syntax-highlighting, however, I add command sleep 0 (could make it command true) after the exec: https://github.com/zdharma/fast-syntax-highlighting/blob/e06288a617b12e8038fdf5bc4bd40b6a25ef5332/fast-highlight#L917-L918

The reason for this additional call are severe problems with Ctrl-C that occur when using {MYFD}< construct, as my investigation showed.

So in this issue I just want to mention that in case of any Ctrl-C problems, command true after exec should help.

The text was updated successfully, but these errors were encountered:

ericfreese · 2018-08-09T00:55:48Z

Thanks for this! I was actually having issues with Ctrl-C on develop but wasn't sure exactly why and hadn't yet put in the effort to figure out what was going on. I'm busy for the next few weeks but I'll take a closer look at this before cutting a release from develop.

ericfreese · 2018-09-19T16:44:52Z

@psprint Did you ever figure out why exactly this was necessary? I'm really curious what's going on that makes ctrl-c stop working.

ericfreese · 2018-09-19T17:19:45Z

I've found something interesting that you may have also found. At least in my case, it seems the problem only occurs with the monitor option set (does not occur with it unset).

psprint · 2018-09-20T11:17:59Z

Interesting. I was tracking this for many days and when workaround appeared I was too tired to continue, so I dont know why this problem occurs

ericfreese · 2018-09-20T20:38:57Z

Fair enough. I'll see what I can find...

ericfreese · 2019-04-09T18:36:07Z

@psprint I've submitted a patch that fixes this issue upstream

https://www.zsh.org/mla/workers/2019/msg00252.html

See technique used in `fast-syntax-highlighting`: - zdharma/fast-syntax-highlighting@ca2e18b - https://www.zsh.org/mla/users/2018/msg00424.html Also see https://www.zsh.org/mla/users/2018/msg00432.html In async response handler: - We only want to read data in case of POLLIN or POLLHUP. Not POLLNVAL or select error. - We always want to remove the handler, so it doesn't get called in an infinite loop when error is nval or err. There is an upstream bug that prevents ctrl-c from resetting the prompt immediately after a suggestion has been fetched asynchronously. A patch has been submitted, but a workaround for now is to add `command true` after the exec. See #364

ericfreese · 2019-04-09T20:31:07Z

FYI @psprint it looks like this workaround doesn't work for zsh versions older than 5.0.8.

See technique used in `fast-syntax-highlighting`: - zdharma/fast-syntax-highlighting@ca2e18b - https://www.zsh.org/mla/users/2018/msg00424.html Also see https://www.zsh.org/mla/users/2018/msg00432.html In async response handler: - We only want to read data in case of POLLIN or POLLHUP. Not POLLNVAL or select error. - We always want to remove the handler, so it doesn't get called in an infinite loop when error is nval or err. There is an upstream bug that prevents ctrl-c from resetting the prompt immediately after a suggestion has been fetched asynchronously. A patch has been submitted, but a workaround for now is to add `command true` after the exec. See #364

ericfreese · 2019-04-09T20:51:56Z

Closing this. I've added the workaround with PR #417

psprint · 2019-04-10T12:32:08Z

Great job! Wt., 9.04.2019, 20:36 użytkownik Eric Freese <[email protected]> napisał:

…

@psprint <https://github.com/psprint> I've submitted a patch that fixes this issue upstream https://www.zsh.org/mla/workers/2019/msg00252.html — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#364 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFxOCIlUus63CaFRz0WMWCxdWyLv9AjFks5vfN2bgaJpZM4VyxCm> .

abcdw · 2021-04-14T06:09:37Z

Can we use command -v true instead of command true here?

command true breaks, when I have no true binary in my $PATH, it is relatively often use case for me, because I have pure environment for various build tasks using guix environment --pure.

BTW, Thank you very much for your great work.

danielshahaf · 2021-04-14T10:36:49Z

Can we use command -v true instead of command true here?

Probably not, given that the comment just above that line of code says a fork is needed, and command -v true doesn't fork. (Also, it spams stdout, so a > /dev/null would be required.)

zsh-autosuggestions/src/async.zsh

Lines 45 to 47 in ae315de

    
           # 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 
        
           command true

command true breaks, when I have no true binary in my $PATH, it is relatively often use case for me, because I have pure environment for various build tasks using guix environment --pure.

Your setup violates POSIX (https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap01.html#tag_17_06) and therefore is likely to break other upstreams too:

However, all of the standard utilities, including [true] shall be implemented in a manner so that they can be accessed via the exec family of functions [...]

(I'm not a z-asug developer.)

abcdw · 2021-04-15T09:13:50Z

@danielshahaf Thank you for the reply!

However, all of the standard utilities, including [true] shall be implemented in a manner so that they can be accessed via the exec family of functions [...]

Not sure if it violates, exec true works perfectly fine just out of the box.

>> which true
true: shell built-in command

>> command true
zsh: command not found: true

>> setopt posixbuiltins
>> command true
>>

Probably not, given that the comment just above that line of code says a fork is needed.

I'm not sure how this workaround works, but maybe something like $(true) will work too?

I get a lot of _zsh_autosuggest_async_request:41: command not found: true when I'm in pure environment without true binary. It's quite annoying. For now I disabled async for pure shells with [ -n $GUIX_ENVIRONMENT ] || ZSH_AUTOSUGGEST_USE_ASYNC=true, but looks a little hacky. Would be glad to have more robust solution, but I'm not very into shells development and do not know all the tricky things related to it(

ericfreese · 2021-04-15T14:39:05Z

There is a test for this workaround here

zsh-autosuggestions/spec/async_spec.rb

Line 53 in ae315de

describe 'pressing ^C after fetching a suggestion' do

If you can get the tests running, you can try some alternatives to command true to see if we can work around the issue in a different way.

The upstream bug has also been fixed in more recent versions of zsh so at this point we may be able to only run the command true for certain older versions. I know that the workaround doesn’t work for versions older than 5.0.8 but I would have to look into the exact version that the bug was fixed in.

danielshahaf · 2021-04-15T16:05:04Z

Not sure if it violates, exec true works perfectly fine just out of the box.

The quoted sentence refers to the execvp(3) C function, not to the exec utility/builtin.

Incidentally, zsh's exec can only be used to execute the true builtin when POSIX_BUILTINS is unset; see the third paragraph of POSIX_BUILTINS' documentation.

Would be glad to have more robust solution, but I'm not very into shells development and do not know all the tricky things related to it(

I recommend to install in the build environment whichever package provides a true executable in $PATH.

The upstream bug has also been fixed in more recent versions of zsh so at this point we may be able to only run the command true for certain older versions. I know that the workaround doesn’t work for versions older than 5.0.8 but I would have to look into the exact version that the bug was fixed in.

https://zsh.org/workers/44214 is zsh commit zsh-5.7.1-46-g5c165453a, first released in 5.8.

abcdw · 2021-05-15T05:28:50Z

--- Sorry for the long response, I've set up a testing environment, made a fix and checked it on versions 5.6.2 and 5.8. Tests are passed. src/async.zsh | 3 ++- zsh-autosuggestions.zsh | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/async.zsh b/src/async.zsh index 4314e8c..218eb26 100644 --- a/src/async.zsh +++ b/src/async.zsh @@ -44,7 +44,8 @@ _zsh_autosuggest_async_request() { # There's a weird bug here where ^C stops working unless we force a fork # See #364 - command true + 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 diff --git a/zsh-autosuggestions.zsh b/zsh-autosuggestions.zsh index a8ef6c4..7ebc490 100644 --- a/zsh-autosuggestions.zsh +++ b/zsh-autosuggestions.zsh @@ -804,7 +804,8 @@ _zsh_autosuggest_async_request() { # There's a weird bug here where ^C stops working unless we force a fork # See #364 - command true + 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 -- 2.31.1

abcdw · 2021-06-03T12:08:32Z

Dear maintainers, is it possible to apply this patch? Seems like a reasonable solution to me.

ericfreese · 2021-06-03T16:17:22Z

Hi, thanks for the patch! Yeah, we should be able to apply this. I'm sorry I haven't had much time for this project lately but should be able to get this in soon. I've also been meaning to cut a new release for some time.

Can you send a pull request to develop branch with these changes?

abcdw · 2021-06-03T16:24:24Z

Sure!

ericfreese added bug reproduced labels Oct 15, 2018

ericfreese mentioned this issue Dec 17, 2018

Features/completion suggestions #401

Merged

ericfreese mentioned this issue Apr 9, 2019

Refactor async mode to no longer use zpty #417

Merged

ericfreese closed this as completed Apr 9, 2019

ericfreese mentioned this issue Jan 18, 2020

shell under Cygwin with ZSH_AUTOSUGGEST_STRATEGY=(completion) completly non responsive #497

Closed

abcdw mentioned this issue Jun 3, 2021

Disable ^C async workaround for zsh version 5.8 and later #612

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

A possibly needed workaround for <( ) process substitution #364

A possibly needed workaround for <( ) process substitution #364

psprint commented Aug 7, 2018

ericfreese commented Aug 9, 2018

ericfreese commented Sep 19, 2018

ericfreese commented Sep 19, 2018 •

edited

Loading

psprint commented Sep 20, 2018 •

edited

Loading

ericfreese commented Sep 20, 2018

ericfreese commented Apr 9, 2019

ericfreese commented Apr 9, 2019

ericfreese commented Apr 9, 2019

psprint commented Apr 10, 2019 via email

abcdw commented Apr 14, 2021

danielshahaf commented Apr 14, 2021

abcdw commented Apr 15, 2021 •

edited

Loading

ericfreese commented Apr 15, 2021

danielshahaf commented Apr 15, 2021

abcdw commented May 15, 2021 via email

abcdw commented Jun 3, 2021 via email

ericfreese commented Jun 3, 2021

abcdw commented Jun 3, 2021 via email

A possibly needed workaround for <( ) process substitution #364

A possibly needed workaround for <( ) process substitution #364

Comments

psprint commented Aug 7, 2018

ericfreese commented Aug 9, 2018

ericfreese commented Sep 19, 2018

ericfreese commented Sep 19, 2018 • edited Loading

psprint commented Sep 20, 2018 • edited Loading

ericfreese commented Sep 20, 2018

ericfreese commented Apr 9, 2019

ericfreese commented Apr 9, 2019

ericfreese commented Apr 9, 2019

psprint commented Apr 10, 2019 via email

abcdw commented Apr 14, 2021

danielshahaf commented Apr 14, 2021

abcdw commented Apr 15, 2021 • edited Loading

ericfreese commented Apr 15, 2021

danielshahaf commented Apr 15, 2021

abcdw commented May 15, 2021 via email

abcdw commented Jun 3, 2021 via email

ericfreese commented Jun 3, 2021

abcdw commented Jun 3, 2021 via email

ericfreese commented Sep 19, 2018 •

edited

Loading

psprint commented Sep 20, 2018 •

edited

Loading

abcdw commented Apr 15, 2021 •

edited

Loading