feat(fsrepo): handle safe Removal

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

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

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)
+	}
+}

repo/fsrepo/fsrepo_test/README.md

@@ -0,0 +1 @@
+This directory is used to store FSRepos generated during go tests.

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)
+}