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

Create a proper Lock/Unlock #2

Closed
rwxrob opened this issue Mar 16, 2022 · 0 comments
Closed

Create a proper Lock/Unlock #2

rwxrob opened this issue Mar 16, 2022 · 0 comments

Comments

@rwxrob
Copy link
Owner

rwxrob commented Mar 16, 2022

Just found out that there is a proposal to externalize good file
locking: golang/go#33974, but it hasn't seen
attention in a while. I think "good" locking will have to wait. For now,
I'm going to just use what kubeconfig uses (which is far from good).

So many seriously bad implementations of file locking exist. None of
them take into account the potential race condition of just looking for
the existence of a file (this ain't Windows). The proper way to do it is
to flock the file in addition to checking if it exists.

Here's an example of a failed lock
design
that is all
throughout the Kubernetes code base (and Kind decided to copy it). Any
seasoned UNIX systems developer will immediately see the failures here:

package kubeconfig

import (
	"os"
	"path/filepath"
)

// these are from
// https://github.com/kubernetes/client-go/blob/611184f7c43ae2d520727f01d49620c7ed33412d/tools/clientcmd/loader.go#L439-L440

func lockFile(filename string) error {
	// Make sure the dir exists before we try to create a lock file.
	dir := filepath.Dir(filename)
	if _, err := os.Stat(dir); os.IsNotExist(err) {
		if err = os.MkdirAll(dir, 0755); err != nil {
			return err
		}
	}
	f, err := os.OpenFile(lockName(filename), os.O_CREATE|os.O_EXCL, 0)
	if err != nil {
		return err
	}
	f.Close()
	return nil
}

func unlockFile(filename string) error {
	return os.Remove(lockName(filename))
}

func lockName(filename string) string {
	return filename + ".lock"
}

I need to add a good Lock/Unlock to this package just so people can
see the difference for themselves and compare.

@rwxrob rwxrob closed this as completed Dec 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant