OK, there's a number of things I'd change about your code. I'll go through what you posted, making suggestions based on what you have already, then I'll make some additional suggestions.
const tofind = "select-test.go"
Just a small nit-pcik here: to improve readability, would suggest camel-casing constants like this. IMO, toFind is easier to read. In a small piece of code like this, it doesn't make that much of a difference, but as code bases grow, and the number of constants increases, it starts adding up.
func main() {
wg := sync.WaitGroup{}
search("/Users/user/Documents", &wg)
wg.Add(1)
wg.Wait()
}
Seeing the call to search, passing it a pointer to the wait group, and only incrementing it after the call is bad form. It's clear that this search function will at some point call wg.Done(). Should, for whatever reason, the search function return before the main function calls wg.Add(1), you'll get a runtime panic. It's easily avoided by simply incrementing your wait group before calling functions which decrement it.
When passing a waitgroup to a function, it's also more common (to the point where it's pretty much the de facto standard) to pass in the wait group first. You main function, as is, should look something like this:
wg.Add(1)
search(&wg, "/path/to/dir")
wg.Wait()
Obviously, you'll need to change the search function to match the different argument order. I'd change this function even more, but we'll cover that a bit later on.
var permit = make(chan struct{}, 30)
A global variable holding a channel works, but it's code smell. Declaring and assigning this variable in between functions if perfectly valid code, but it doesn't look clean. Code is written for people to read, compilers translate human readable code into instructions for computers to execute. Generally speaking, I prefer it when I'm able to open source files and see what types, constants, and variables it creates/declares/uses at the top of the file. The structure I tend to follow is:
package
imports
constants
variables
types
functions (New first, Exported functions next, unexported functions towards the bottom)
This is a common pattern to follow, I've found. In some cases you'll see variables before constants, or the types being defined first, but generally speaking, code tends to be broken down into these groups.
There is, however, a bigger concern here: because your channel is a global variable, it's not exactly "owned" by any particular function/scope. You don't explicitly close the channel, which is considered bad form. You're using this channel to ensure you have no more than 30 goroutines (not coroutines) searching the path at any given time. Once you've found what you're looking for, the channel is no longer needed, can be closed and GC'ed. That essentially means that, once the wait group is done, it should be perfectly safe to close the channel. In this case, that means the channel can be moved to the scope of main:
func main() {
wg := sync.WaitGroup{}
ch := make(chan struct{}, 30)
wg.Add(1)
search(&wg, ch, "/path/to/dir")
wg.Wait()
wg.Done()
close(ch)
}
Once again, you'll need to add the channel to the search arguments to something like this:
func search(wg *sync.WaitGroup, ch chan struct{}, path string)
Now let's look at the main part of your code, this search function. You start out writing to the channel (as mentioned above to ensure no more than 30 concurrent routines). After that you're reading from the channel and decrementing the wait group in seperate defer functions. The order in which these functions are executed is LIFO. Basically, you'll decrement the waitgroup first, and drain the channel last. Reading from a closed channel is not an issue, but I thought I'd mention it. The main thing here is that there is no real reason to have 2 anonymous functions in these defer statements. You could either write:
defer func() {
wg.Done()
<- ch // or <-permit in your case
}()
If, for whatever reason, you want to keep these two operations as distinct defer calls, you can simply write this:
defer func() {
<-ch // <-permit
}()
defer wg.Done() // no need to wrap this call in to a function...
As peterSO mentioned, in terms of performance improvements, you can easily replace ioutil.ReadDir with os.ReadDir. It'll return a slice of DirEntry objects, sorted by name. You can get the name, file-type, etc... through its interface, as documented here. The code should look something like this:
entries, err := os.ReadDir(path)
if err != nil {
fmt.Println(err)
return // exit here!
}
for _, entry := range entries {
if entry.FileInfo().IsDir() {
wg.Add(1)
go search(wg, ch, filepath.Join(path, entry.Name()))
continue
}
if entry.Name() == toFind {
fmt.Printf("Found %s here: %s\n", toFind, path)
return // return, we're done
}
}
Having this return statement in the loop can be quite a big deal, depending on what you want your program to do. I've chosen to stop iterating over the directory items once we found a match. Having 2 files with the exact same name in a directory simply doesn't make sense, so there is no reason to keep looking in this path.
The problem here is that we could very well end up in a situation where we found a file with the name we're looking for in a particular path, but we've already spun up 29 other routines that are all recursively looking for a file with the desired name. If we only want to find one match, we might end up looking through the entire file system recursively. This could take a long time, and yield no results. Imagine you're looking for a specific file (e.g. my_app_logs_20220808_err.log. There probably is only one file with this name on your system. Now imagine someone ran this code starting from the path /. That's going to scan the entire filesystem. It could relatively quickly find /logs/my_app_logs_20220808_err.log, but the program will continue to scan the entire filesystem. You'll encounter errors when you're trying to search paths like /root or /home/some_other_user_with_encrypted_files and so on...
What you want to be able to do is to cancel the entire recursive search. Basically, you want to be able to cancel all search routines as soon as you found a match. Thankfully, golang has something that allows you to do exactly that very easily:
func main() {
ctx, cfunc := context.WithCancel(context.Background())
defer cfunc() // make sure this will always be called
wg := sync.WaitGroup{}
ch := make(ch struct{}, 30)
wg.Add(1)
go search(ctx, &wg, ch, cfunc, "/path/to/search")
wg.Wait()
close(ch)
}
Through context, we can signal to all routines within this particular context that they should stop doing whatever they do. To achieve this, all we have to do is pass in the cancel function and the context. Then just change the search function to this:
func search(ctx context.Context, wg *sync.WaitGroup, ch chan struct{}, found func(), path string) {
entries, err := os.ReadDir(path)
if err != nil {
fmt.Println(err)
return // exit here!
}
for _, entry := range entries {
select {
case <-ctx.Done(): // run is done?
return // exit this function
default: // if we haven't found what we're looking for, carry on
if entry.FileInfo().IsDir() {
wg.Add(1)
go search(ctx, wg, ch, found, filepath.Join(path, entry.Name()))
continue // remove this if we're lookging for directories
}
if entry.Name() == toFind {
fmt.Printf("Found %s here: %s\n", toFind, path)
found() // call the "found" callback, which cancels the context
return // and return
}
}
}
}
Lastly, in case we want to be able to run this program from the command line using arguments/flags to perform a quick recursive file search using something like:
$ my_search -f filename.foo -p /path/to/search
We have to account for our process to be interrupted/cancelled by the user. In the main function, we basically have to handle system signals. To stop all search routines and cleanly exit the program, we can simply cancel the context, signalling any and all routines to stop doing what they're doing and return:
import (
"os"
"os/signal"
)
func main() {
ctx, cfunc := signal.NotifyContext(context.Background(), os.Interrupt, os.Kill)
defer cfunc()
// the rest is the same
}
With this, the ctx.Done() channel will be closed when we receive an interrupt or kill signal, and in the search function, the select statement will enter the case <-ctx.Done() block, and return from search, execute the defer calls (decrementing the waitgroup, drain the channel). Then, the main when all this is done, the main function will close the channel and return. Nice and clean.
