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.

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

After IO buffer is overrun, IO buffering is broken for future commands #5267

Closed
mqudsi opened this issue Oct 19, 2018 · 16 comments
Closed

After IO buffer is overrun, IO buffering is broken for future commands #5267

mqudsi opened this issue Oct 19, 2018 · 16 comments
Labels
bug regression
Milestone

Comments

@mqudsi
Copy link
Contributor

@mqudsi mqudsi commented Oct 19, 2018

all-the-package-names has recently passed the 10MiB boundary, which uncovered some problems:

mqudsi@Blitzkrieg /m/d/r/fish> math (all-the-package-names | wc -c) / 1024 / 1024
10.534367

When the io buffer boundary is reached, the shell can be left unusable:

mqudsi@Blitzkrieg /m/d/r/fish> fg
/usr/local/share/fish/config.fish (line 279): Too much data emitted by command substitution so it was discarded

    builtin fg (__fish_expand_pid_args $argv)
               ^
in function “fg”
        called on standard input

Obviously, $argv is empty here and wouldn't have returned any data, let alone > 10MB.

To reproduce,

rubish.fish:

function foo1
    for i in (seq 1 20)
        cat /usr/share/dict/words
    end
end

# Entire list of packages is too long to be used efficiently in a `complete` subcommand.
# Search it for matches instead.
function foo2
    foo1 | string match -re -- ^(commandline -ct) #| head -n 50
end

complete -c rubbish -xa '(foo2)'

followed by the following:

source ./rubbish.fish
touch ./rubbish
chmod +x ./rubbish
./rubbish <TAB>
fg
@mqudsi mqudsi changed the title After IO buffer is overrun, execution of future functions breaks After IO buffer is overrun, command substitution remains broken Oct 19, 2018
@mqudsi mqudsi changed the title After IO buffer is overrun, command substitution remains broken After IO buffer is overrun, io buffering is broken for future commands Oct 19, 2018
@faho faho added the bug label Oct 20, 2018
@faho faho added this to the fish-future milestone Oct 20, 2018
@mqudsi
Copy link
Contributor Author

@mqudsi mqudsi commented Oct 20, 2018

@faho: With regards to milestone, etc: I'm pretty sure this was caused by 4197420, which never shipped in any release.

Before that, there was no limit imposed on the command substitution size, so I don't think the shell could have reached this state.

@faho
Copy link
Member

@faho faho commented Oct 20, 2018

I'm pretty sure this was caused by 4197420, which never shipped in any release.

Sure, yeah.

More specifically, the "read too much" flag on the buffer is probably never reset.

Also, since we now have something that actually outputs 10MiB.... should we increase the limit?

I mean that entire thing came about because somebody noticed that reading from (IIRC) /dev/zero would "hang" fish and be generally unhelpful. So it just needs to be large enough that it's "hang detection", not something to discourage you from using the shell. So 100MiB or even 1GiB would probably be alright on most systems (obviously if you ran this on a raspberry pi with 256MiB of RAM and no swap the latter won't really work).

@faho faho removed this from the fish-future milestone Oct 20, 2018
@faho faho added this to the fish-3.0 milestone Oct 20, 2018
@faho
Copy link
Member

@faho faho commented Oct 20, 2018

Oh, wait, there actually is an underlying limit: ARG_MAX.

If we're over that the OS won't accept the arguments anyway. On my system getconf ARG_MAX returns 2097152, which is 2MiB.

So that 10MiB won't work anyway.

@mqudsi
Copy link
Contributor Author

@mqudsi mqudsi commented Oct 20, 2018

If we're over that the OS won't accept the arguments anyway. On my system getconf ARG_MAX returns 2097152, which is 2MiB.

No, the problem is that IO buffering is not only used for command substitution but also where it shouldn't be used such as piping output from a function's stdout to another function or process' stdin.

If you look at the example you'll note that there's no command substitution (apart from (commandline -ct), that is) going on. It's all stdout buffering.

IMHO this is fish's biggest job-related failure (way worse than the keepalive process), all function/block output is buffered when all of it should be using anonymous pipes connected directly to the read end instead. Moreover, the IO buffer isn't really a "buffer" in the traditional sense in that it's written to then read from (such as the OS' own pipe buffer) but rather a write-only buffer that becomes available for reading only after the write is completely finished, i.e. it must buffer the entire content.

@faho
Copy link
Member

@faho faho commented Oct 20, 2018

If you look at the example you'll note that there's no command substitution

The (foo2) in

complete -c rubbish -xa '(foo2)'

is a command substitution.

@faho
Copy link
Member

@faho faho commented Oct 20, 2018

I can only confirm this with rubbish <TAB>, but not rubbish a<TAB>, and if I replace (foo2) with (foo2 | string match -re ^a), it also works.

So it really does seem like it's the command substitution buffer being used for a command substitution.

@mqudsi
Copy link
Contributor Author

@mqudsi mqudsi commented Oct 20, 2018

Oh, I completely missed that, I was just focusing on the function definition. I'm pretty sure function output is buffered even without command substitution, but I think this bug (shell unusable) only affects command substitution.

@faho
Copy link
Member

@faho faho commented Oct 20, 2018

I'm pretty sure function output is buffered even without command substitution

Yup - #1396.

@mqudsi
Copy link
Contributor Author

@mqudsi mqudsi commented Oct 20, 2018

Ah, yes. That's the one.

The reason function stdout buffering doesn't run into this bug is that functions there's no limit on how big the function buffer can get, they're explicitly allowed an uncapped buffer.

@faho
Copy link
Member

@faho faho commented Oct 21, 2018

This behaves kind of weird.

$ rubbish <TAB>
fish: Too much data emitted by command substitution so it was discarded #....
$ fg
fish: Too much data emitted by command substitution so it was discarded
$ echo (echo abc)
$ fg
fg: There are no suitable jobs

Do we somehow share buffers sometimes?

@mqudsi mqudsi changed the title After IO buffer is overrun, io buffering is broken for future commands After IO buffer is overrun, IO buffering is broken for future commands Oct 22, 2018
@mqudsi mqudsi changed the title After IO buffer is overrun, IO buffering is broken for future commands After IO buffer is overrun, iobuffering is broken for future commands Oct 22, 2018
@mqudsi mqudsi changed the title After IO buffer is overrun, iobuffering is broken for future commands After IO buffer is overrun, IO buffering is broken for future commands Oct 22, 2018
mqudsi added a commit that referenced this issue Oct 29, 2018
Don't attempt to complete against package names if the user is trying to
enter a switch to speed things up.

Also work around #5267 by not wrapping unfiltered `all-the-package-name`
calls in a function.
ridiculousfish pushed a commit to ridiculousfish/fish-shell that referenced this issue Nov 24, 2018
When we discard output because there's been too much, we print a
warning, but subsequent uses of the same buffer still discard.

Now we explicitly reset the flag, so we warn once and everything works
normal after.

Fixes fish-shell#5267.
ridiculousfish pushed a commit to ridiculousfish/fish-shell that referenced this issue Nov 24, 2018
Don't attempt to complete against package names if the user is trying to
enter a switch to speed things up.

Also work around fish-shell#5267 by not wrapping unfiltered `all-the-package-name`
calls in a function.
@mqudsi mqudsi reopened this Jan 11, 2019
@mqudsi
Copy link
Contributor Author

@mqudsi mqudsi commented Jan 11, 2019

I just ran into this working on the yarn completions again. It's definitely not fixed.

mqudsi@Blitzkrieg /m/d/r/fish> fg
Send job 1, “nvim share/completions/yarn.fish” to foreground
Job 1, 'nvim share/completions/yarn.fish' has stopped
mqudsi@Blitzkrieg /m/d/r/fish> fg
/usr/local/share/fish/config.fish (line 267): Too much data emitted by command substitution so it was discarded

    builtin fg (__fish_expand_pid_args $argv)
               ^
in function “fg”
        called on standard input

mqudsi@Blitzkrieg /m/d/r/fish> exit
There are still jobs active:

   PID  Command
 26038  nvim share/completions/yarn.fish

A second attempt to exit will terminate them.
Use 'disown PID' to remove jobs from the list without terminating them.

@mqudsi mqudsi removed this from the fish 3.0.0 milestone Jan 11, 2019
@mqudsi mqudsi added this to the fish 3.1.0 milestone Jan 11, 2019
@faho
Copy link
Member

@faho faho commented Jan 11, 2019

@mqudsi: This works for me. Please confirm your $version.

@mqudsi
Copy link
Contributor Author

@mqudsi mqudsi commented Jan 11, 2019

It’s a recent master build but I know the test as documented here was fixed since I confirmed as much at the time. But the issue can be triggered differently.

@faho
Copy link
Member

@faho faho commented Jan 13, 2019

But the issue can be triggered differently.

@mqudsi: Any idea how? What did you do differently?

@mqudsi
Copy link
Contributor Author

@mqudsi mqudsi commented Jan 13, 2019

This worked (it's an even simpler test than the original one):

mqudsi@Blitzkrieg /m/d/D/CPSA Board> function foo
                                         bar (all-the-package-names)
                                     end
mqudsi@Blitzkrieg /m/d/D/CPSA Board> function bar
                                     end
mqudsi@Blitzkrieg /m/d/D/CPSA Board> foo
fish: Too much data emitted by command substitution so it was discarded

bar (all-the-package-names)
    ^
in function “foo”
        called on standard input

mqudsi@Blitzkrieg /m/d/D/CPSA Board> fg
/usr/local/share/fish/config.fish (line 267): Too much data emitted by command substitution so it was discarded

    builtin fg (__fish_expand_pid_args $argv)
               ^
in functionfg”
        called on standard input

@faho
Copy link
Member

@faho faho commented May 29, 2019

Okay, this works for me with 29c627.

In addition, I'm going to increase the read limit to 100MiB. That still might not blow up your little Raspi, and you could possibly pass it in multiple passes. I don't think fish is used on smaller machines, so we don't need to optimize for those.

@faho faho closed this May 29, 2019
faho added a commit that referenced this issue May 29, 2019
Someone has hit the 10MiB limit (and of course it's the number of
javascript packages), and we don't handle it fantastically currently.

And even though you can't pass a variable of that size in one go, it's
plausible that someone might do it in multiple passes.

See #5267.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug regression
Projects
None yet
Development

No branches or pull requests

2 participants