kelleyma49 / PSFzf

A PowerShell wrapper around the fuzzy finder fzf
MIT License
787 stars 34 forks source link

Various problems when accepting entry before input list completed streaming #112

Closed pitkali closed 2 years ago

pitkali commented 2 years ago

Do

  1. Attempt to change directory using Alt+C with a very deep directory tree.
  2. Accept a selection while the input is still streaming.

Expected

Directory is changed to the selection.

Actual

The selection UI disappears, but the directory is not changed.

Remarks

I have noticed this on Powershell 7.2.1 on a Linux system. I'll try reproducing later on Windows. This is harder to hit with fd for listing directories, but I have very deep directory tree.

I tried doing something like "fd -td | Invoke-Fzf | Set-Location". It changes the directory but then also shows an error message:

MethodInvocationException: Exception calling "Flush" with "0" argument(s): "Broken pipe"

Similar problem happens with Ctrl+t — under the circumstances described the selection is not added to the command line.

mattcargile commented 2 years ago

Maybe it is because the default behavior on *nix based systems is to return files ( instead of directories ) with find. I wasn't able to return a directory to ALT + C into. Do you have FZF_ALT_C_COMMAND or any of the other environment variables set?

Additionally, I didn't realize the below would work. The below does work for me on WSL/Ubuntu 20.04. Are you on 2.3.2 of PSFzf?

fd -td | Invoke-Fzf | Set-Location

Does this work the same or throw the same error?

Set-Location ( fd -td | Invoke-Fzf )

pitkali commented 2 years ago

Out of curiosity, when you were trying to reproduce the behaviour, did you make sure to accept a selection while the command listing directories/files was still streaming its output?

I guess I have a slow day at work, so I'll see if I can find something useful in the source.

pitkali commented 2 years ago

It looks to me that Get-FileSystemCmd does a very poor job of supporting dirOnly parameter. Essentially the only way for default commands to work with ALT+C (that is, to filter out non-directories) is to -EnableFd, although I find the default fd command quite different from my version based on default commands in ZSH integration (no link following for example, which is probably not the best choice for Linux).

My working hypothesis is that stopping the pipeline doesn't work that well when its first component is a regular old command that just writes strings to the output and it messes up handling the result. Which raises a question: why aren't default non-fd commands just based around Get-ChildItem which would surely not suffer from such issue? I remember doing some experiments on my tree and it didn't come slower than find on Linux.

I guess I'll work on setting up some dev environment and see if I can work it out.

By the way: if you look into zsh integration that comes with FZF you'll notice that it specifically does not use FZF_DEFAULT_COMMAND for Ctrl+T and Alt+C key binds because of different requirements. Perhaps it doesn't matter that much for the former but the latter is a clear example where PSFzf would work better by following that example.

pitkali commented 2 years ago

Note that if I switch Alt+C command to 'gci -Recurse -Directory -Name' (which is actually much slower, not sure how I tested it in the past), everything works as expected. This supports my early pipeline stop hypothesis.

mattcargile commented 2 years ago

I wasn't clear on your full configuration. I was attempting to recreate the behavior with the default PSFzf configuration on Ubuntu.

So I tried $env:FZF_ALT_C_COMMAND = 'fd -td' on Ubuntu 20.04 on WSL2 on Windows 10 and it worked. I didn't wait until it finished streaming either. I'm on fd.exe version 7.4.0. What is your *nix config?

We don't use Get-ChildItem because it is too slow so we shell out to cmd.exe to get the "DOS" dir command.

pitkali commented 2 years ago

I'm on Ubuntu 20.04 here but not inside WSL. The error is not specific to fd, actually. The default command (find) also fails in the same way for me.

mattcargile commented 2 years ago

Weird. I wonder why it is complaining about 0 arguments on Flush? I was trying hard to get it to error and couldn't get that error. I was ALT + C-ing and as soon as I saw text I was hitting <ENTER>.

The code within your PR should probably be wrapped in a try/catch block with a Close method in the finally block. I haven't read that entire functions' source though so I could be missing something.

pitkali commented 2 years ago

It is not the 0 arguments that is the problem — it is the broken pipe. The stream that is being flushed is broken and no further IO will succeed. It seems this is the stream leading to standard input of fzf, so it's broken because fzf exited, which does make me wonder why &cleanup hasn't reset $utf8Stream. I tried moving null assignment there to a finally block but I just got a Broken pipe error when calling WriteLine instead.

I have literally 0 experience developing and debugging on Powershell so not sure yet what else would be a better target for a fix. Interestingly enough, I can remove the exception by wrapping WriteLine in try/catch instead and stopping the pipeline from the catch section, but Ctrl+t and Alt+c still don't work if I do that. Only checking for errors when flushing actually fixes those for me.

As for closing, I thought it would be handled by End stage, especially now that the exception is swallowed?

mattcargile commented 2 years ago

I don't know. What version of PowerShell are you on and what is your .Net Runtime Info.

pwsh --version [System.Runtime.InteropServices.RuntimeInformation]::get_FrameworkDescription()

Mine is 7.2.1 and .NET 6.0.0-rtm.21522.10 respectively.

pitkali commented 2 years ago

I'm on Powershell 7.2.1 and .NET 6.0.0-rtm.21522.10 as well. Yay for obscure bugs.

Wouldn't be the first powershell bug either -- it propagates Ctrl+C to all the subprocesses including background jobs instead of just the foreground process running in it!

mattcargile commented 2 years ago

I gotcha. Are you bare metal Linux or using a VM? Maybe I could get the behavior on a Ubuntu VM on Windows? I thought WSL2 was essentially the same thing.

pitkali commented 2 years ago

My work machine is a company provided bare metal Linux beast (my work actually requires a Linux system). I had no reason or desire to play with WSL1 or 2 so far.

pitkali commented 2 years ago

I can confirm that the problem does not reproduce for me on Windows 11.

pitkali commented 2 years ago

Ok, so I installed WSL2 on my Windows 11, and found out the following:

mattcargile commented 2 years ago

I gotcha. How many levels deep is your folder structure? What is your longest path?

pitkali commented 2 years ago

Actually, I was trying again, and I can no longer reproduce it in WSL2…

You know, now that I think about it, the one time I saw it in WSL2, it was also in a different place — at WriteLine. The error was the same, however: broken pipe. This leads me to believe that it is generally a timing problem — when fzf exits between a check for process exit and next attempt to write/flush the stream.

This, of course, does not explain why I could reproduce it reliably on bare Linux installation, although I have seen such reproducible timing bugs for extended periods of time in the past. However, it suggests that a more reliable fix would surround all writes to the stream with try/catch blocks to protect against broken pipe errors at least long enough to check for exit status and shutdown the pipeline gracefully.

I'm going to enjoy the rest of my evening for the time being and see if the problem persists on my linux machine tomorrow.

pitkali commented 2 years ago

I couldn't actually stop thinking about it so I went back to my work machine (that's what I get for WFH) and ran more tests. Upon closer inspection I realised that reproduction ratio is not, in fact, 100%, although it is still nearly that high. I don't know why, but I think it reinforces theory about pipe going away in between checks of process status.

That would mean that the existing pull request only fixes the most common case. This might be overwhelmingly most common case simply becasue writes are buffered and broken pipe will only happen if flush is triggered. This will hardly ever happen when executing WriteLine, but will always happen when calling Flush explicitly, which is why I get an exception almost always there.

So I wrote def9f621b8a33cb3a0df1670cabc01f4dcd65683 that would comprehensively make sure you can never hit that edge case. It also stops the pipeline early if flush throws — I realised that it may not be the last call to Process.

And with this, I am actually done for now.