reader: Fixed race condition

[alex-huff]

reader: Fixed race condition where reader would be terminated before it was properly initialized.

Currently, when reloading input very quickly (happens when updating the prompt quickly with 'reload' bound to 'change') it is possible for the reader's terminate method to be called before readFromCommand properly spawns the new reader process. This means the process is never killed and input is not reloaded until the process completes naturally or another 'EvtSearchNew' is triggered.

An example where this becomes a problem is when using fzf with rg integration and searching a large directory. Occasionally, the query being processed by rg is not the same as the query displayed by fzf and only after further modifying the query or waiting until rg finishes will the input reload to the correct state. This commit introduces a 'started' condition that the terminate method will wait for before trying to kill the process. The 'started' condition is signalled only when the reader reaches a state where it can be safely terminated.

[junegunn]

Is there any way I can easily reproduce the problem without having to rely on ripgrep and a large directory? Maybe using a contrived command with sleep for an artificial delay.

[alex-huff]

fzf --disabled --bind 'change:reload:echo Current query: {q} && sleep 1d'

When running this command, sometimes when you stop typing a query you may see that the item list does not update for the latest version of the query. You either have to wait 1 day or modify the query for the item list to update.

fzf-race-2-20fps.mp4

Notice at the end of the video the query is "i like to use fzf" but the item in the list of results says "i like to use fz".

[junegunn]

Thanks, I can reproduce the problem. I believe your patch will fix the problem, but the fundamental reason we have this race in the first place is that we "restart" the process asynchronously in a go routine.

fzf/src/core.go

Line 302 in 82ebcd9

go reader.restart(command, environ)

So I think a more direct way to fix it is to not do it asynchronously, i.e. remove go. I'll restructure the code so that we can call restart synchronously.

[junegunn]

Please see dcb4c3d and let me know if it doesn't fix the problem, or you find any flaw in the patch. Thanks.

[alex-huff]

Please see dcb4c3d and let me know if it doesn't fix the problem, or you find any flaw in the patch. Thanks.

Thank you! dcb4c3d does fix the problem I was encountering.

I did notice 3 minor issues when reading through reader.go/core.go.

1. In restart, if startCommand returns nil, nil then fin is never called. This means the coordinator routine never receives a EvtReadFin event. So, if exec.Cmd.Start fails, no future reload commands will ever be run. I verified this in a contrived way by entering a query, moving my shell, attempting to modify the query (input did not reload as expected), moving my shell back, and then further modifying the query (still the input did not reload even with valid shell). exec.Cmd.Start is unlikely to fail but in the case that it does I think that fin should be called to properly shutdown the event poller and allow future input reloading.

2. There is still a race condition with ReadSource when !streamingFilter.
   

   fzf/src/core.go

   Lines 170 to 176 in d938fdc

   	if !streamingFilter {
   	reader = NewReader(func(data []byte) bool {
   	return chunkList.Push(data)
   	}, eventBox, executor, opts.ReadZero, opts.Filter == nil)
   	go reader.ReadSource(opts.Input, opts.WalkerRoot, opts.WalkerOpts, opts.WalkerSkip, initialReload, initialEnv)
   	}

   
   This race condition is much less likely to cause problems then the original one, but still exists. It can be simulated by adding a time.Sleep(time.Second) to ReadSource and then entering a query when fzf launches. When ReadSource wakes up, the reader's termFunc will have already been set to nil. fzf is then in a state where the item list will not be updated until either the initial command completes or the query is modified.

In action:

fzf --disabled --bind "start:reload:echo start command; sleep 1d" --bind "change:reload:echo reloaded input with query {q}"

read-source-race-20fps.mp4

3. When piping data to fzf and then trying to reload the input, the Read call in feed will not return until more data is sent to stdin. This means that when piping data to fzf from a long running program that only occasionally (or never) produces output, attempting to reload input will fail for potentially long periods of time.

Here is a contrived example:

(while true; do echo stdin data; sleep 10; done) | fzf --disabled --bind "change:reload:echo reloaded with: {q}"

fzf-stdin-reload-20fps.mp4

This is because the stdin file is in blocking mode. Calling util.SetNonblock(os.Stdin, true) before readFromStdin and then appropriately handling syscall.EAGAIN errors from Read seemed to fix the issue on my system.

[junegunn]

Thank you very much for the detailed analysis. I'll address the issues and let you know of the progress.

[junegunn]

I believe 19495eb addresses the first two issues you mentioned. It restored the previous code structure, but added a barrier using a channel to ensure that the process has started.

I'm not going to think too much about the third issue for now, as I'm afraid trying to fix it might cause other side effects (e.g. affecting the ingestion performance) and we can work around it using a start:reload binding.

fzf --disabled \
    --bind 'change:reload:echo reloaded with: {q}' \
    --bind 'start:reload:while true; do echo stdin data; sleep 10; done'

Thanks for your feedback!

[junegunn]

Hmm, the last commit has a bug that can freeze fzf when a process failed to start. Let me see..

fzf --bind change:reload:ls --with-shell xxx

EDIT: Fixed in acdf265

[alex-huff]

Thank you!