diff --git a/cmd/ipfs/init.go b/cmd/ipfs/init.go index 4a65d18ce..29f5dbe39 100644 --- a/cmd/ipfs/init.go +++ b/cmd/ipfs/init.go @@ -101,6 +101,9 @@ func doInit(repoRoot string, force bool, nBitsForKeypair int) (interface{}, erro return nil, err } } else { + if err := fsrepo.Remove(repoRoot); err != nil { + return nil, err + } r := fsrepo.At(repoRoot) if err := r.Open(); err != nil { return nil, err diff --git a/repo/fsrepo/fsrepo.go b/repo/fsrepo/fsrepo.go index 5d3dcd1b6..f57acb118 100644 --- a/repo/fsrepo/fsrepo.go +++ b/repo/fsrepo/fsrepo.go @@ -1,6 +1,7 @@ package fsrepo import ( + "errors" "fmt" "io" "os" @@ -13,7 +14,22 @@ import ( debugerror "github.com/jbenet/go-ipfs/util/debugerror" ) -// FSRepo represents an IPFS FileSystem Repo +var ( + // pkgLock prevents the fsrepo from being removed while there exist open + // FSRepo handles. It also ensures that the Init is atomic. + // + // packageLock also protects numOpenedRepos + // + // If an operation is used when repo is Open and the operation does not + // change the repo's state, the package lock does not need to be acquired. + pkgLock *packageLock +) + +func init() { + pkgLock = makePackageLock() +} + +// FSRepo represents an IPFS FileSystem Repo. It is not thread-safe. type FSRepo struct { state state path string @@ -22,6 +38,7 @@ type FSRepo struct { // At returns a handle to an FSRepo at the provided |path|. func At(path string) *FSRepo { + // This method must not have side-effects. return &FSRepo{ path: path, state: unopened, // explicitly set for clarity @@ -30,7 +47,10 @@ func At(path string) *FSRepo { // Init initializes a new FSRepo at the given path with the provided config. func Init(path string, conf *config.Config) error { - if IsInitialized(path) { + pkgLock.Lock() // lock must be held to ensure atomicity (prevent Removal) + defer pkgLock.Unlock() + + if isInitializedUnsynced(path) { return nil } configFilename, err := config.Filename(path) @@ -43,12 +63,24 @@ func Init(path string, conf *config.Config) error { return nil } +// Remove recursively removes the FSRepo at |path|. +func Remove(path string) error { + pkgLock.Lock() + defer pkgLock.Unlock() + if pkgLock.NumOpeners(path) != 0 { + return errors.New("repo in use") + } + return os.RemoveAll(path) +} + // Open returns an error if the repo is not initialized. func (r *FSRepo) Open() error { + pkgLock.Lock() + defer pkgLock.Unlock() if r.state != unopened { return debugerror.Errorf("repo is %s", r.state) } - if !IsInitialized(r.path) { + if !isInitializedUnsynced(r.path) { return debugerror.New("ipfs not initialized, please run 'ipfs init'") } // check repo path, then check all constituent parts. @@ -86,12 +118,17 @@ func (r *FSRepo) Open() error { } r.state = opened + pkgLock.AddOpener(r.path) return nil } -// Config returns the FSRepo's config. Result is undefined if the Repo is not -// Open. +// Config returns the FSRepo's config. This method must not be called if the +// repo is not open. +// +// Result when not Open is undefined. The method may panic if it pleases. func (r *FSRepo) Config() *config.Config { + // no lock necessary because repo is either Open (and thus protected from + // Removal) or has no side-effect if r.state != opened { panic(fmt.Sprintln("repo is", r.state)) } @@ -100,6 +137,7 @@ func (r *FSRepo) Config() *config.Config { // SetConfig updates the FSRepo's config. func (r *FSRepo) SetConfig(updated *config.Config) error { + // no lock required because repo should be Open if r.state != opened { panic(fmt.Sprintln("repo is", r.state)) } @@ -146,6 +184,7 @@ func (r *FSRepo) GetConfigKey(key string) (interface{}, error) { // SetConfigKey writes the value of a particular key. func (r *FSRepo) SetConfigKey(key string, value interface{}) error { + // no lock required because repo should be Open if r.state != opened { return debugerror.Errorf("repo is %s", r.state) } @@ -172,9 +211,12 @@ func (r *FSRepo) SetConfigKey(key string, value interface{}) error { // Close closes the FSRepo, releasing held resources. func (r *FSRepo) Close() error { + pkgLock.Lock() + defer pkgLock.Unlock() if r.state != opened { return debugerror.Errorf("repo is %s", r.state) } + pkgLock.RemoveOpener(r.path) return nil // TODO release repo lock } @@ -183,6 +225,14 @@ var _ repo.Interface = &FSRepo{} // IsInitialized returns true if the repo is initialized at provided |path|. func IsInitialized(path string) bool { + pkgLock.Lock() + defer pkgLock.Unlock() + return isInitializedUnsynced(path) +} + +// isInitializedUnsynced reports whether the repo is initialized. Caller must +// hold pkgLock. +func isInitializedUnsynced(path string) bool { configFilename, err := config.Filename(path) if err != nil { return false diff --git a/repo/fsrepo/fsrepo_test.go b/repo/fsrepo/fsrepo_test.go new file mode 100644 index 000000000..6650c2602 --- /dev/null +++ b/repo/fsrepo/fsrepo_test.go @@ -0,0 +1,67 @@ +package fsrepo + +import ( + "os" + "path" + "testing" + + "github.com/jbenet/go-ipfs/repo/config" +) + +// NB: These tests cannot be run in parallel + +func init() { + // ensure tests begin in clean state + os.RemoveAll(testRepoDir) +} + +const testRepoDir = "./fsrepo_test/repos" + +func testRepoPath(p string) string { + return path.Join(testRepoDir, p) +} + +func TestCannotRemoveIfOpen(t *testing.T) { + path := testRepoPath("TestCannotRemoveIfOpen") + AssertNil(Init(path, &config.Config{}), t, "should initialize successfully") + r := At(path) + AssertNil(r.Open(), t) + AssertErr(Remove(path), t, "should not be able to remove while open") + AssertNil(r.Close(), t) + AssertNil(Remove(path), t, "should be able to remove after closed") +} + +func TestCanManageReposIndependently(t *testing.T) { + pathA := testRepoPath("a") + pathB := testRepoPath("b") + + t.Log("initialize two repos") + AssertNil(Init(pathA, &config.Config{}), t, "should initialize successfully") + AssertNil(Init(pathB, &config.Config{}), t, "should initialize successfully") + + t.Log("open the two repos") + repoA := At(pathA) + repoB := At(pathB) + AssertNil(repoA.Open(), t) + AssertNil(repoB.Open(), t) + + t.Log("close and remove b while a is open") + AssertNil(repoB.Close(), t, "close b") + AssertNil(Remove(pathB), t, "remove b") + + t.Log("close and remove a") + AssertNil(repoA.Close(), t) + AssertNil(Remove(pathA), t) +} + +func AssertNil(err error, t *testing.T, msgs ...string) { + if err != nil { + t.Error(msgs, "error:", err) + } +} + +func AssertErr(err error, t *testing.T, msgs ...string) { + if err == nil { + t.Error(msgs, "error:", err) + } +} diff --git a/repo/fsrepo/fsrepo_test/.gitkeep b/repo/fsrepo/fsrepo_test/.gitkeep new file mode 100644 index 000000000..e69de29bb diff --git a/repo/fsrepo/fsrepo_test/README.md b/repo/fsrepo/fsrepo_test/README.md new file mode 100644 index 000000000..2c80c41bb --- /dev/null +++ b/repo/fsrepo/fsrepo_test/README.md @@ -0,0 +1 @@ +This directory is used to store FSRepos generated during go tests. diff --git a/repo/fsrepo/lock.go b/repo/fsrepo/lock.go new file mode 100644 index 000000000..be3653e9a --- /dev/null +++ b/repo/fsrepo/lock.go @@ -0,0 +1,54 @@ +package fsrepo + +import ( + "path" + "sync" +) + +type packageLock struct { + // lock protects repos + lock sync.Mutex + // repos maps repo paths to the number of openers holding an FSRepo handle + // to it + repos map[string]int +} + +func makePackageLock() *packageLock { + return &packageLock{ + repos: make(map[string]int), + } +} + +// Lock must be held to while performing any operation that modifies an +// FSRepo's state field. This includes Init, Open, Close, and Remove. +func (l *packageLock) Lock() { + l.lock.Lock() +} + +func (l *packageLock) Unlock() { + l.lock.Unlock() +} + +// NumOpeners returns the number of FSRepos holding a handle to the repo at +// this path. This method is not thread-safe. The caller must have this object +// locked. +func (l *packageLock) NumOpeners(repoPath string) int { + return l.repos[key(repoPath)] +} + +// AddOpener messages that an FSRepo holds a handle to the repo at this path. +// This method is not thread-safe. The caller must have this object locked. +func (l *packageLock) AddOpener(repoPath string) { + l.repos[key(repoPath)]++ +} + +// RemoveOpener messgaes that an FSRepo no longer holds a handle to the repo at +// this path. This method is not thread-safe. The caller must have this object +// locked. +func (l *packageLock) RemoveOpener(repoPath string) { + l.repos[key(repoPath)]-- +} + +func key(repoPath string) string { + return path.Clean(repoPath) +}