It seems that I find a zle -F full CPU bug

It seems that I find a zle -F full CPU bug

[lilydjwg]

Hi there,

I get to know "zle -F" from the mail by Bart Schaefer[1], but it seems
that I find a bug in zle.

I use similar code to achieve the background updating, but after the
update and *before* I type anything into zsh again, zsh is busy poll the
coprocess's file descriptor, which has been invalid.

I've checked Src/Zle/zle_main.c and see a "for(;;)" loop. That loop will
break only when no fds are ready, or the user types something. So,
before the user types something and after the watched fd becomes
invalid, zsh keeps poll that fd and gets lots of POLLNVAL.

You can type the following to reproduce this:

print_and_close () {
  print $*
  zle -F $1
  exec {1}>&-
}
coproc echo "a"
exec {test}<&p
zle -F $test print_and_close

Now zsh is eating a lot of CPU. Type anything and it will become normal
again.

I'm using zsh 5.0.5 on Arch Linux x86_64.

[1]: http://www.zsh.org/mla/users/2014/msg00204.html

-- 
Best regards,
lilydjwg

Re: It seems that I find a zle -F full CPU bug

[Bart Schaefer]

[-- Attachment #1: Type: text/plain, Size: 1061 bytes --]

On Wednesday, February 19, 2014, lilydjwg <lilydjwg@gmail.com> wrote:
>
>
> I've checked Src/Zle/zle_main.c and see a "for(;;)" loop. That loop will
> break only when no fds are ready, or the user types something. So,
> before the user types something and after the watched fd becomes
> invalid, zsh keeps poll that fd and gets lots of POLLNVAL.


Although it would be better if zsh did not begin hogging the CPU, it's
possible for that closed descriptor to be re-opened again (e.g. by a signal
handler), so unless we are willing to ignore that I'm not sure what to do.
 I suppose we could drop the fd on the first POLLNVAL for the duration of
that particular loop, but then reinstate it before restarting the loop
after the next user input.  Or something more complicated where we drop it
for only one call to poll() that has an artificial timeout to limit how
long it can be before we discover that the fd is valid again.

I don't think we want to go so far as to make it an error to pass a closed
fd to zle -F ... and that wouldn't help in this case anyway?

Re: It seems that I find a zle -F full CPU bug

[Peter Stephenson]

On Wed, 19 Feb 2014 23:30:52 -0800
Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Wednesday, February 19, 2014, lilydjwg <lilydjwg@gmail.com> wrote:
> >
> >
> > I've checked Src/Zle/zle_main.c and see a "for(;;)" loop. That loop will
> > break only when no fds are ready, or the user types something. So,
> > before the user types something and after the watched fd becomes
> > invalid, zsh keeps poll that fd and gets lots of POLLNVAL.
> 
> 
> Although it would be better if zsh did not begin hogging the CPU, it's
> possible for that closed descriptor to be re-opened again (e.g. by a signal
> handler), so unless we are willing to ignore that I'm not sure what to do.
>  I suppose we could drop the fd on the first POLLNVAL for the duration of
> that particular loop, but then reinstate it before restarting the loop
> after the next user input.

That doesn't sound a bad compromise.

Ideally we'd want to make the user aware of the error, but once per user
input, i.e. typically a key press, is probably too often for that.

> I don't think we want to go so far as to make it an error to pass a closed
> fd to zle -F ... and that wouldn't help in this case anyway?

I don't see any real point in testing it before we use it.  I think this
is part of the same problem of how we signal to the user their fd isn't
working without messing up the display.

pws

Re: It seems that I find a zle -F full CPU bug

[lilydjwg]

On Thu, Feb 20, 2014 at 09:40:53AM +0000, Peter Stephenson wrote:
> On Wed, 19 Feb 2014 23:30:52 -0800
> Bart Schaefer <schaefer@brasslantern.com> wrote:
> > Although it would be better if zsh did not begin hogging the CPU, it's
> > possible for that closed descriptor to be re-opened again (e.g. by a signal
> > handler), so unless we are willing to ignore that I'm not sure what to do.
> >  I suppose we could drop the fd on the first POLLNVAL for the duration of
> > that particular loop, but then reinstate it before restarting the loop
> > after the next user input.
> 
> That doesn't sound a bad compromise.

I think this will work. Or we can just break the loop if all we get from
the poll result is POLLNVAL so that handlers can remove bad fds.

> Ideally we'd want to make the user aware of the error, but once per user
> input, i.e. typically a key press, is probably too often for that.
> 
> > I don't think we want to go so far as to make it an error to pass a closed
> > fd to zle -F ... and that wouldn't help in this case anyway?

Actually there is no closed fd passed to zle -F in my case. It's closed
after that.

> I don't see any real point in testing it before we use it.  I think this
> is part of the same problem of how we signal to the user their fd isn't
> working without messing up the display.

The handler can know that the fd is closed in case of 'poll': the
handler will get an argument like 'hup' or 'nval'. But it doesn't help
in this case: the handler can uninstall itself, but this only takes
effect after the for loop is done.

-- 
Best regards,
lilydjwg

Re: It seems that I find a zle -F full CPU bug

[Nick Cross]

Using Fedora 5.0.5 and similar background code from Bart 
(http://www.zsh.org/mla/users/2014/msg00448.html) I can see the same 
POLLNVAL high CPU from strace.

Is there any suitable workaround for 5.0.5 ?

Thanks

Nick

On 20/02/14 13:16, lilydjwg wrote:
> On Thu, Feb 20, 2014 at 09:40:53AM +0000, Peter Stephenson wrote:
>> On Wed, 19 Feb 2014 23:30:52 -0800
>> Bart Schaefer <schaefer@brasslantern.com> wrote:
>>> Although it would be better if zsh did not begin hogging the CPU, it's
>>> possible for that closed descriptor to be re-opened again (e.g. by a signal
>>> handler), so unless we are willing to ignore that I'm not sure what to do.
>>>   I suppose we could drop the fd on the first POLLNVAL for the duration of
>>> that particular loop, but then reinstate it before restarting the loop
>>> after the next user input.
>>
>> That doesn't sound a bad compromise.
>
> I think this will work. Or we can just break the loop if all we get from
> the poll result is POLLNVAL so that handlers can remove bad fds.
>
>> Ideally we'd want to make the user aware of the error, but once per user
>> input, i.e. typically a key press, is probably too often for that.
>>
>>> I don't think we want to go so far as to make it an error to pass a closed
>>> fd to zle -F ... and that wouldn't help in this case anyway?
>
> Actually there is no closed fd passed to zle -F in my case. It's closed
> after that.
>
>> I don't see any real point in testing it before we use it.  I think this
>> is part of the same problem of how we signal to the user their fd isn't
>> working without messing up the display.
>
> The handler can know that the fd is closed in case of 'poll': the
> handler will get an argument like 'hup' or 'nval'. But it doesn't help
> in this case: the handler can uninstall itself, but this only takes
> effect after the for loop is done.
>

Re: It seems that I find a zle -F full CPU bug

[Bart Schaefer]

On Jul 16,  2:55pm, Nick Cross wrote:
} Subject: Re: It seems that I find a zle -F full CPU bug
}
} Using Fedora 5.0.5 and similar background code from Bart 
} (http://www.zsh.org/mla/users/2014/msg00448.html) I can see the same 
} POLLNVAL high CPU from strace.
} 
} Is there any suitable workaround for 5.0.5 ?

Unfortunately I don't think there's any shell-language-level workaround.

You could try fiddling with the order in which the zle -F handlers are
removed and skip closing the descriptor in update_super_status, but it
will become complicated to keep track of the un-closed descriptors in
order to avoid leaking (i.e., never closing) them.

Re: It seems that I find a zle -F full CPU bug

[Nick Cross]

On 16/07/14 17:01, Bart Schaefer wrote:
> On Jul 16,  2:55pm, Nick Cross wrote:
> } Subject: Re: It seems that I find a zle -F full CPU bug
> }
> } Using Fedora 5.0.5 and similar background code from Bart
> } (http://www.zsh.org/mla/users/2014/msg00448.html) I can see the same
> } POLLNVAL high CPU from strace.
> }
> } Is there any suitable workaround for 5.0.5 ?
>
> Unfortunately I don't think there's any shell-language-level workaround.
>
> You could try fiddling with the order in which the zle -F handlers are
> removed and skip closing the descriptor in update_super_status, but it
> will become complicated to keep track of the un-closed descriptors in
> order to avoid leaking (i.e., never closing) them.
>


Darn ;-) Ok, is there a fix/alternative solution planned for the next 
version? (and do you happen what the schedule for that is? ).

The implementations you kindly provided for background update for prompt 
data is *extremely* handy when working with a lot of very large 
repositories.

Thanks very much

Nick

Re: It seems that I find a zle -F full CPU bug

[Bart Schaefer]

On Jul 16,  6:10pm, Nick Cross wrote:
} Subject: Re: It seems that I find a zle -F full CPU bug
}
} On 16/07/14 17:01, Bart Schaefer wrote:
} > On Jul 16,  2:55pm, Nick Cross wrote:
} > } Subject: Re: It seems that I find a zle -F full CPU bug
} > }
} > } Using Fedora 5.0.5 and similar background code from Bart
} > } (http://www.zsh.org/mla/users/2014/msg00448.html) I can see the same
} > } POLLNVAL high CPU from strace.
} >
} > Unfortunately I don't think there's any shell-language-level workaround.
} 
} Darn ;-) Ok, is there a fix/alternative solution planned for the next 
} version? (and do you happen what the schedule for that is? ).

There's already been a patch, zsh-workers/32427.  Fedora certainly has
the option of folding that in early via RPM spec if they can be caused
to care.

As for a schedule for 5.0.6, that's sort of up to Peter, who seems to be
pretty busy right now ...

Re: It seems that I find a zle -F full CPU bug

[Nick Cross]

Hi Bart,

I compiled up https://github.com/zsh-users/zsh (SHA 
4e54648add79f7cb0c0fe81e46f49817d4555f2a ) which contains

7e04c1a - 32427: avoid busy loop on closed descriptors for "zle -F" 
handlers (5 months ago) (Barton E. Schaefer)

I am using your second suggested 'git info in background' 
(http://www.zsh.org/mla/users/2014/msg00448.html) rather than 
http://www.zsh.org/mla/users/2014/msg00204.html and am happy to report 
it works fine :-) Interestingly it also fixes the multiple Ctrl-C which 
I reported in another thread.

Thanks!

Nick



On 16/07/14 18:50, Bart Schaefer wrote:
> On Jul 16,  6:10pm, Nick Cross wrote:
> } Subject: Re: It seems that I find a zle -F full CPU bug
> }
> } On 16/07/14 17:01, Bart Schaefer wrote:
> } > On Jul 16,  2:55pm, Nick Cross wrote:
> } > } Subject: Re: It seems that I find a zle -F full CPU bug
> } > }
> } > } Using Fedora 5.0.5 and similar background code from Bart
> } > } (http://www.zsh.org/mla/users/2014/msg00448.html) I can see the same
> } > } POLLNVAL high CPU from strace.
> } >
> } > Unfortunately I don't think there's any shell-language-level workaround.
> }
> } Darn ;-) Ok, is there a fix/alternative solution planned for the next
> } version? (and do you happen what the schedule for that is? ).
>
> There's already been a patch, zsh-workers/32427.  Fedora certainly has
> the option of folding that in early via RPM spec if they can be caused
> to care.
>
> As for a schedule for 5.0.6, that's sort of up to Peter, who seems to be
> pretty busy right now ...
>

Re: It seems that I find a zle -F full CPU bug

[Dominic Hopf]

[-- Attachment #1.1: Type: text/plain, Size: 2267 bytes --]

Greetings,

I've tried applying the patch for the Fedora but unfortunately the build
fails with attached output. Basically, I just did a

    git diff -u  79abe00e1d73b3f0681343a70e3e7f84e5e3d647
7e04c1a53ddada7a848753d151e18f9469788b98 -- Doc/Zsh/zle.yo
Src/Zle/zle_main.c >| ~/rpmbuild/SOURCES/0001-zsh-fix-cpuload-issue.patch

to create the patch I'm applying in the SPEC file. Is this something I have
done wrong?

Regards,.
Dominic


2014-07-17 15:52 GMT+02:00 Nick Cross <nick@goots.org>:

>
> Hi Bart,
>
> I compiled up https://github.com/zsh-users/zsh (SHA
> 4e54648add79f7cb0c0fe81e46f49817d4555f2a ) which contains
>
> 7e04c1a - 32427: avoid busy loop on closed descriptors for "zle -F"
> handlers (5 months ago) (Barton E. Schaefer)
>
> I am using your second suggested 'git info in background' (
> http://www.zsh.org/mla/users/2014/msg00448.html) rather than
> http://www.zsh.org/mla/users/2014/msg00204.html and am happy to report it
> works fine :-) Interestingly it also fixes the multiple Ctrl-C which I
> reported in another thread.
>
> Thanks!
>
> Nick
>
>
>
>
> On 16/07/14 18:50, Bart Schaefer wrote:
>
>> On Jul 16,  6:10pm, Nick Cross wrote:
>> } Subject: Re: It seems that I find a zle -F full CPU bug
>> }
>> } On 16/07/14 17:01, Bart Schaefer wrote:
>> } > On Jul 16,  2:55pm, Nick Cross wrote:
>> } > } Subject: Re: It seems that I find a zle -F full CPU bug
>> } > }
>> } > } Using Fedora 5.0.5 and similar background code from Bart
>> } > } (http://www.zsh.org/mla/users/2014/msg00448.html) I can see the
>> same
>> } > } POLLNVAL high CPU from strace.
>> } >
>> } > Unfortunately I don't think there's any shell-language-level
>> workaround.
>> }
>> } Darn ;-) Ok, is there a fix/alternative solution planned for the next
>> } version? (and do you happen what the schedule for that is? ).
>>
>> There's already been a patch, zsh-workers/32427.  Fedora certainly has
>> the option of folding that in early via RPM spec if they can be caused
>> to care.
>>
>> As for a schedule for 5.0.6, that's sort of up to Peter, who seems to be
>> pretty busy right now ...
>>
>>
>


-- 
Diese E-Mail ist nicht mit GPG signiert, da ich sie vom Webinterface aus
geschrieben habe.

This mail is not signed with GPG because I wrote it from web interface.

[-- Attachment #1.2: Type: text/html, Size: 3429 bytes --]

[-- Attachment #2: output.txt --]
[-- Type: text/plain, Size: 3720 bytes --]

ccache gcc -c -I. -I../../Src -I../../Src -I../../Src/Zle -I.  -DHAVE_CONFIG_H -DMODULE -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches  -m64 -mtune=generic -fPIC -o zle_bindings..o zle_bindings.c
ccache gcc -c -I. -I../../Src -I../../Src -I../../Src/Zle -I.  -DHAVE_CONFIG_H -DMODULE -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches  -m64 -mtune=generic -fPIC -o zle_hist..o zle_hist.c
ccache gcc -c -I. -I../../Src -I../../Src -I../../Src/Zle -I.  -DHAVE_CONFIG_H -DMODULE -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches  -m64 -mtune=generic -fPIC -o zle_keymap..o zle_keymap.c
ccache gcc -c -I. -I../../Src -I../../Src -I../../Src/Zle -I.  -DHAVE_CONFIG_H -DMODULE -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches  -m64 -mtune=generic -fPIC -o zle_main..o zle_main.c
zle_main.c: In function ‘zleread’:
zle_main.c:1256:2: warning: implicit declaration of function ‘viinsert_init’ [-Wimplicit-function-declaration]
  viinsert_init();
  ^
ccache gcc -c -I. -I../../Src -I../../Src -I../../Src/Zle -I.  -DHAVE_CONFIG_H -DMODULE -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches  -m64 -mtune=generic -fPIC -o zle_misc..o zle_misc.c
ccache gcc -c -I. -I../../Src -I../../Src -I../../Src/Zle -I.  -DHAVE_CONFIG_H -DMODULE -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches  -m64 -mtune=generic -fPIC -o zle_move..o zle_move.c
ccache gcc -c -I. -I../../Src -I../../Src -I../../Src/Zle -I.  -DHAVE_CONFIG_H -DMODULE -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches  -m64 -mtune=generic -fPIC -o zle_params..o zle_params.c
ccache gcc -c -I. -I../../Src -I../../Src -I../../Src/Zle -I.  -DHAVE_CONFIG_H -DMODULE -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches  -m64 -mtune=generic -fPIC -o zle_refresh..o zle_refresh.c
zle_refresh.c: In function ‘zrefresh’:
zle_refresh.c:1646:17: warning: ‘rprompt_off’ may be used uninitialized in this function [-Wmaybe-uninitialized]
      vcs = winw - rprompt_off;
                 ^
ccache gcc -c -I. -I../../Src -I../../Src -I../../Src/Zle -I.  -DHAVE_CONFIG_H -DMODULE -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches  -m64 -mtune=generic -fPIC -o zle_thingy..o zle_thingy.c
ccache gcc -c -I. -I../../Src -I../../Src -I../../Src/Zle -I.  -DHAVE_CONFIG_H -DMODULE -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches  -m64 -mtune=generic -fPIC -o zle_tricky..o zle_tricky.c
zle_tricky.c: In function ‘docomplete’:
zle_tricky.c:614:9: error: ‘undoing’ undeclared (first use in this function)
     if (undoing)
         ^
zle_tricky.c:614:9: note: each undeclared identifier is reported only once for each function it appears in
make[3]: *** [zle_tricky..o] Error 1
make[3]: Leaving directory `/home/dmaphy/rpmbuild/BUILD/zsh-5.0.5/Src/Zle'
make[2]: *** [modules] Error 1
make[2]: Leaving directory `/home/dmaphy/rpmbuild/BUILD/zsh-5.0.5/Src'
make[1]: *** [modules] Error 2
make[1]: Leaving directory `/home/dmaphy/rpmbuild/BUILD/zsh-5.0.5/Src'
make: *** [all] Error 1

Re: It seems that I find a zle -F full CPU bug

[Bart Schaefer]

On Jul 17,  5:52pm, Dominic Hopf wrote:
}
} 
} I've tried applying the patch for the Fedora but unfortunately the build
} fails with attached output. Basically, I just did a
} 
}     git diff -u  79abe00e1d73b3f0681343a70e3e7f84e5e3d647
} 7e04c1a53ddada7a848753d151e18f9469788b98 -- Doc/Zsh/zle.yo
} Src/Zle/zle_main.c >| ~/rpmbuild/SOURCES/0001-zsh-fix-cpuload-issue.patch
} 
} to create the patch I'm applying in the SPEC file. Is this something I have
} done wrong?

Your "git diff" is covering too much ground, you've pulled in multiple
unrelated patches but only gotten the subset of the deltas that apply
to zle_main.c, so interdependent changes are lost and breakage results.

Try (backslash-newlines inserted for readability):

    git diff -u 73db206838e427348cb91f1955692da5d2820d00 \
    		7e04c1a53ddada7a848753d151e18f9469788b98 -- \
    	Doc/Zsh/zle.yo Src/Zle/zle_main.c \
    	>| ~/rpmbuild/SOURCES/0001-zsh-fix-cpuload-issue.patch

Re: It seems that I find a zle -F full CPU bug

[Dominic Hopf]

[-- Attachment #1: Type: text/plain, Size: 1378 bytes --]

That did it, thanks very much for your support. Fedora Updates are on their
way… :-)


2014-07-17 19:18 GMT+02:00 Bart Schaefer <schaefer@brasslantern.com>:

> On Jul 17,  5:52pm, Dominic Hopf wrote:
> }
> }
> } I've tried applying the patch for the Fedora but unfortunately the build
> } fails with attached output. Basically, I just did a
> }
> }     git diff -u  79abe00e1d73b3f0681343a70e3e7f84e5e3d647
> } 7e04c1a53ddada7a848753d151e18f9469788b98 -- Doc/Zsh/zle.yo
> } Src/Zle/zle_main.c >| ~/rpmbuild/SOURCES/0001-zsh-fix-cpuload-issue.patch
> }
> } to create the patch I'm applying in the SPEC file. Is this something I
> have
> } done wrong?
>
> Your "git diff" is covering too much ground, you've pulled in multiple
> unrelated patches but only gotten the subset of the deltas that apply
> to zle_main.c, so interdependent changes are lost and breakage results.
>
> Try (backslash-newlines inserted for readability):
>
>     git diff -u 73db206838e427348cb91f1955692da5d2820d00 \
>                 7e04c1a53ddada7a848753d151e18f9469788b98 -- \
>         Doc/Zsh/zle.yo Src/Zle/zle_main.c \
>         >| ~/rpmbuild/SOURCES/0001-zsh-fix-cpuload-issue.patch
>



-- 
Diese E-Mail ist nicht mit GPG signiert, da ich sie vom Webinterface aus
geschrieben habe.

This mail is not signed with GPG because I wrote it from web interface.