-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
ripgrep doesn't stop when its pipe is closed #200
Comments
This one isn't going to be fun to fix. This is what I get for ignoring any errors that occur when printing to The primary difficulty at present is bubbling up an error from the printing code all the way through the search code. Both the search/print code assume no errors can happen. We could just thread an error through everything. My question for this in terms of UX is: do we treat all IO errors equally when writing output? Should we do something different if we see a pipe error versus, say, a permission error? Maybe any type of error causes ripgrep to stop whatever it's doing, but a pipe error indicates normal termination where as anything else results in a non-zero exit code and the error being printed to stderr. |
I might elect to forgo fixing this until I factor the search code out into a separate crate. (Which will be a while. My guess is at least a month.) |
I upgraded yesterday from rg 0.3.2-1 to 0.4 and now these commands are essentially useless:
This is too bad, since I use rg+fzf in my workflow. |
@danr There was no regression. This bug was in "useless" sounds like a bit of an exaggeration. |
Sorry, I really did not mean to sound harsh. Thank you for your work on |
This is how grep acts:
This is how rg acts:
|
@kpp Right, this bug is understood, it's just a pain to fix. Please do not fix it. I would like to fix it personally when I refactor this code into a separate crate. |
|
Actually, what that strace says is that grep does an explicit "panic" of some sort (it raises SIGPIPE) when it gets a EPIPE error from write. It doesn't bubble the error up or anything, which is what your proposed fix needs refactoring for. |
@glandium Right. Rust's standard library (IIRC) suppresses the |
Could we just |
@oconnor663 Graciously submitted a PR implementing the |
@BurntSushi Hi, any plans for a patch release including the fix? Many users, myself included, would want to install ripgrep using Homebrew/Linuxbrew, and use it with a secondary filter like fzf. |
@junegunn Sure, I can do that. Hopefully soon. |
Confirmed fixed in 0.7.1 on macOS. Thanks. |
Since BurntSushi/ripgrep#200 is fixed in 0.7.1, we can safely suggest ripgrep as the candidate generator as it has a more precise implementation of gitignore filtering than the silver searcher.
@junegunn it's still an issue for me on 0.7.1 on ubuntu - did you use a binary release? Using this command:
|
@dpnova Could you please provide a reproducible example on a corpus that is public without using fzf? I've tested this myself on Linux and it works fine. |
Just confirming... to repro I should be able to run
|
FWIW I can't repro without fzf. My test case is simply running the command. I'm sure I'm missing something though. |
Do you have any big file in your repo? I had this problem when there was like 2GB sql file in my folder. Once I've added this file to |
@pbogut The @dpnova I don't see how that is a complete test case. Could you please provide more details? This bug doesn't impact the correctness of ripgrep (so whether you're able to "run" an For example, if I run People, please, I'm begging you. Don't simply stop at "it doesn't work." Describe what you observe in as much detail as possible so that other people can diagnose your problem. Please, understand that not everyone uses FZF, so saying, "here's this FZF config and it doesn't work" will not get us anywhere. |
Sorry I wasn't clear enough that I was asking for help to repro, I didnt get a response, so I tried something. Now I have a concrete case, thanks. I'm currently waiting for the chromium repo to clone (yay Australian broadband). In my own repo where I'm seeing the issue in fzf.vim the two cases look like this:
|
This is running it in my home directory (fairly new formatted machine).
To me this says this specific github issue isn't the case I'm talking about, despite the fzf github referencing this one. I'll take my discussion back over there. Sorry for any confusion. |
@dpnova I agree with your conclusion. :-) Thanks for sticking it out and confirming that this particular bug isn't it! |
@BurntSushi : Can you have a look at this issue that use rg with fzf?. I can't find a way to reproduce it without fzf. |
@tuyenpm9 If you read the thread, you can see that this issue is not related to your problem. |
Yes, sorry about that. |
This commit updates the CHANGELOG to reflect all the work done to make libripgrep a reality. * Closes #162 (libripgrep) * Closes #176 (multiline search) * Closes #188 (opt-in PCRE2 support) * Closes #244 (JSON output) * Closes #416 (Windows CRLF support) * Closes #917 (trim prefix whitespace) * Closes #993 (add --null-data flag) * Closes #997 (--passthru works with --replace) * Fixes #2 (memory maps and context handling work) * Fixes #200 (ripgrep stops when pipe is closed) * Fixes #389 (more intuitive `-w/--word-regexp`) * Fixes #643 (detection of stdin on Windows is better) * Fixes #441, Fixes #690, Fixes #980 (empty matching lines are weird) * Fixes #764 (coalesce color escapes) * Fixes #922 (memory maps failing is no big deal) * Fixes #937 (color escapes no longer used for empty matches) * Fixes #940 (--passthru does not impact exit status) * Fixes #1013 (show runtime CPU features in --version output)
This commit updates the CHANGELOG to reflect all the work done to make libripgrep a reality. * Closes #162 (libripgrep) * Closes #176 (multiline search) * Closes #188 (opt-in PCRE2 support) * Closes #244 (JSON output) * Closes #416 (Windows CRLF support) * Closes #917 (trim prefix whitespace) * Closes #993 (add --null-data flag) * Closes #997 (--passthru works with --replace) * Fixes #2 (memory maps and context handling work) * Fixes #200 (ripgrep stops when pipe is closed) * Fixes #389 (more intuitive `-w/--word-regexp`) * Fixes #643 (detection of stdin on Windows is better) * Fixes #441, Fixes #690, Fixes #980 (empty matching lines are weird) * Fixes #764 (coalesce color escapes) * Fixes #922 (memory maps failing is no big deal) * Fixes #937 (color escapes no longer used for empty matches) * Fixes #940 (--passthru does not impact exit status) * Fixes #1013 (show runtime CPU features in --version output)
Although the fix definitely seems to have helped, ripgrep can still take a long time to notice SIGPIPE. I haven't looked at the source, but the behavior I'm seeing leads me to suspect that ripgrep doesn't notice the pipe has closed until the next time it attempts to write to it. This is problematic in a long-running search that has entered a phase in which matches are found infrequently (or not at all). I first noticed the problem with an |
@bpstahlman It would help if you could provide a more concrete reproduction that I can try. With that said, the behavior you're describing makes sense and it's what I would expect to happen.
That is correct and expected. That's when a pipe error occurs: Lines 95 to 98 in 73103df
AFAIK, you are the first one to report this as a significant problem.
ripgrep does not use any Rust "framework." For more context, Rust by default ignores SIGPIPE: rust-lang/rust#62569 (I make an appearance in that thread asking for something to be done, but there hasn't been any movement on that issue AFAIK.) This means that pipe errors are only detected once an actual write occurs. At that point, the pipe error is reported "in band" instead of as a signal. It is trivial to stop ignoring SIGPIPE. This would make ripgrep behave like a "normal" C UNIX application. SIGPIPE gets sent to the process, and since there is no signal handler for it, the process (by default) will terminate immediately. This likely achieves the behavior you want. ripgrep currently does not do that because it's not portable. I'm not a Windows expert, but AFAIK, there is no SIGPIPE on Windows. So ripgrep has to deal with in-band pipe errors correctly anyway for compatibility with Windows. Dealing with these types of errors has been subtle and difficult to get right. For that reason, I don't really want to support both in-band (synchronous) and out-of-band (asynchronous) ways of terminating on pipe errors. Because now I won't be dog-fooding the handling of in-band pipe errors on Unix. |
I understand your reasoning and appreciate the detailed explanation. I suspect the reason for the lack of complaints regarding the status quo is that for small to medium-sized projects with suitable .gitignore files, the ripgrep search will often complete before the fzf user has finished making his file selection. And if fzf occasionally appears to hang after Enter is pressed, a user is unlikely to bother reporting unless it happens often or the delays are exceptionally long. I don't recall noticing the issue until I started using ripgrep on my home directory (which is fairly large and doesn't have a .gitignore). As for the desire to avoid maintaining both in and out-of-band SIGPIPE handling logic... I wouldn't think it would be necessary to remove the "in-band" logic to add Linux-only "out-of-band" SIGPIPE handling. The "in-band" logic needed for Windows could still be tested in a Linux build compiled without the "out-of-band" logic, while the "out-of-band" logic would simply be compiled out of the Windows build. E.g.,
|
It isn't. The fact is that now two error paths need to be tested. That was my point. |
Having looked into this a bit more, I'm not convinced that a change to SIGPIPE handling would have any impact on the behavior I'm seeing. IIUC, SIGPIPE isn't even generated until the writer process attempts to write to the closed pipe, which is too late. And I'm not even sure there's a way for a writer process to check for a closed pipe without attempting to write. I've tried using select(), fstat(), write() of 0 bytes, etc., and the only thing that gave any indication that stdout had closed was an attempt to write actual data to it. I would have expected there would be some way - even if it involved a relatively costly system call - for a writer process to check the status of a pipe without writing unwanted data if it happens to be open. I'm not sure exactly how the fzf Vim plugin creates the |
I believe some people that find this issue (in relation to rg | fzf) will find their woes relieved by this issue: |
For example, the following two
rg
commands take the same amount of time, but the second one should be much shorter:The text was updated successfully, but these errors were encountered: