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

--max-results=1 does not actually quit after the first result #867

Closed
durka opened this issue Oct 27, 2021 · 9 comments · Fixed by #902
Closed

--max-results=1 does not actually quit after the first result #867

durka opened this issue Oct 27, 2021 · 9 comments · Fixed by #902
Labels

Comments

@durka
Copy link

durka commented Oct 27, 2021

Describe the bug you encountered:

I am running fd on a slow FUSE filesystem (goofys). It seems like -1/--max-results=1 doesn't do what it says on the tin.

Describe what you expected to happen:

I expected fd to print the result and then quit. There is only one file on the path that matches my pattern.

I ran this command: fd -F --max-results=1 PATTERN PATH

One result is printed ~5 seconds later, but fd continues to run for a long time, at least a minute, until I interrupted it. FWIW, find can search the entire path in <25 seconds.

What version of fd are you using?

fd 8.2.1

Which operating system / distribution are you on?

Linux 5.8.0-63-generic x86_64
Distributor ID:	Ubuntu
Description:	Ubuntu 20.10
Release:	20.10
Codename:	groovy
@durka durka added the bug label Oct 27, 2021
@sharkdp
Copy link
Owner

sharkdp commented Oct 27, 2021

Thank you very much. That is definitely not expected. --max-results was implemented for this exact reason. Otherwise, we could have gone with fd … | head -n1. See #555 and #472 for details.

In your case, there might be something else is going on. Could you try to run fd with strace -f in order to see what fd is doing after this long time?

@durka
Copy link
Author

durka commented Oct 28, 2021

It doesn't really look any different before and after it prints the result, it's just opening directories and stating VCS directories (which don't exist). I do notice that if I pass -I it's quite a bit faster (since it doesn't look for the ignore files). But it still doesn't quit after printing that first result.

@durka
Copy link
Author

durka commented Oct 28, 2021

Now that I look more closely, soon after printing the result it transitions into a huge string of nanosleep({tv_sec=0, tv_nsec=1000000}) calls... seems suspicious.

@durka
Copy link
Author

durka commented Oct 28, 2021

strace -ftk on a debug build reveals the sleeps are from here: https://github.com/BurntSushi/ripgrep/blob/master/crates/ignore/src/walk.rs#L1644

Do we need to be telling the walker to quit scanning?

@durka
Copy link
Author

durka commented Oct 28, 2021

This makes a HUGE difference:

diff --git a/src/walk.rs b/src/walk.rs
index 789a500..6cbb175 100644
--- a/src/walk.rs
+++ b/src/walk.rs
@@ -274,6 +274,7 @@ fn spawn_receiver(
                         num_results += 1;
                         if let Some(max_results) = config.max_results {
                             if num_results >= max_results {
+                                wants_to_quit.store(true, Ordering::Relaxed);
                                 break;
                             }
                         }

I don't fully understand the purpose of this wants_to_quit, but could this be the fix?

@tavianator
Copy link
Collaborator

I can reproduce this. I think you're right about the cause, we're still waiting for the worker threads to finish even once the receiver thread is done.

@durka
Copy link
Author

durka commented Oct 28, 2021

Btw, I was looking into testing this by hacking https://github.com/wfraser/fuse-mt/blob/master/example/src/passthrough.rs to insert a delay before all filesystem operations. More consistent (and cheaper) than hitting AWS over and over. Might be a good way to set up unit tests even.

@sharkdp
Copy link
Owner

sharkdp commented Nov 14, 2021

Thank you for reporting this.

I can reproduce this. I think you're right about the cause, we're still waiting for the worker threads to finish even once the receiver thread is done.

I don't think we are waiting for the workers threads to completely finish the search, but it might be the case that some of them keep running until … well to be honest, I don't know for sure. They will definitely quit once they attempt to send something back over the MPSC channel to the receiver thread (SendError / send_result.is_err()). But for details we would need to check the WalkParallel implementation in ignore. There seems to be a is_quit shortcut to stop some worker threads.

I don't fully understand the purpose of this wants_to_quit, but could this be the fix?

wants_to_quit is only used for Ctrl-C detection. We shouldn't reuse it (otherwise we would quit with an error), but we could introduce a second atomic bool for that purpose. I don't really know how to easily reproduce this error (I tried by introducing some thread::sleep calls) though.

@tavianator
Copy link
Collaborator

The key to reproducing this is to search for something that has only one match For example, I have only one file in my home directory called title_t00.mkv, and it is found reasonably early in the search. But fd title_t00.mkv ~ -1 -j1 takes a long time to finish. I assume it's because nothing gets sent from the workers, so they don't see the closed channel.

tavianator added a commit to tavianator/fd that referenced this issue Dec 5, 2021
quit_flag is now used to quit the sender threads for any reason, either
due to an interrupt or because the receiver is done.

interrupt_flag is used specifically for ^C interrupts, and causes the
receiver to stop between printing paths, to avoid unfinished escape
sequences when colors are being used.

Fixes sharkdp#867.
tavianator added a commit to tavianator/fd that referenced this issue Dec 5, 2021
quit_flag is now used to quit the sender threads for any reason, either
due to an interrupt or because the receiver is done.

interrupt_flag is used specifically for ^C interrupts, and causes the
receiver to stop between printing paths, to avoid unfinished escape
sequences when colors are being used.

Fixes sharkdp#867.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants