From 6ec60ba861c6e39ba47b16ac8bea4bcd5e71f1f5 Mon Sep 17 00:00:00 2001 From: Brian Tiger Chow Date: Tue, 13 Jan 2015 21:13:33 -0800 Subject: [PATCH] feat(fsrepo): document lock usage and make the fsrepo thread-safe fix(fsrepo): extract private, unsynced method to prevent deadlock --- repo/fsrepo/fsrepo.go | 121 +++++++++++++++++++++++++++++------------- 1 file changed, 85 insertions(+), 36 deletions(-) diff --git a/repo/fsrepo/fsrepo.go b/repo/fsrepo/fsrepo.go index 2a9f8a10a..6e422c201 100644 --- a/repo/fsrepo/fsrepo.go +++ b/repo/fsrepo/fsrepo.go @@ -41,7 +41,8 @@ func init() { lockfiles = make(map[string]io.Closer) } -// FSRepo represents an IPFS FileSystem Repo. It is not thread-safe. +// FSRepo represents an IPFS FileSystem Repo. It is safe for use by multiple +// callers. type FSRepo struct { state state path string @@ -58,6 +59,11 @@ func At(repoPath string) *FSRepo { } func ConfigAt(repoPath string) (*config.Config, error) { + + // packageLock must be held to ensure that the Read is atomic. + packageLock.Lock() + defer packageLock.Unlock() + configFilename, err := config.Filename(repoPath) if err != nil { return nil, err @@ -67,7 +73,10 @@ func ConfigAt(repoPath string) (*config.Config, error) { // Init initializes a new FSRepo at the given path with the provided config. func Init(path string, conf *config.Config) error { - packageLock.Lock() // lock must be held to ensure atomicity (prevent Removal) + + // packageLock must be held to ensure that the repo is not initialized more + // than once. + packageLock.Lock() defer packageLock.Unlock() if isInitializedUnsynced(path) { @@ -84,28 +93,42 @@ func Init(path string, conf *config.Config) error { } // Remove recursively removes the FSRepo at |path|. -func Remove(path string) error { +func Remove(repoPath string) error { + repoPath = path.Clean(repoPath) + + // packageLock must be held to ensure that the repo is not removed while + // being accessed by others. packageLock.Lock() defer packageLock.Unlock() - if openerCounter.NumOpeners(path) != 0 { + + if openerCounter.NumOpeners(repoPath) != 0 { return errors.New("repo in use") } - return os.RemoveAll(path) + return os.RemoveAll(repoPath) } // LockedByOtherProcess returns true if the FSRepo is locked by another // process. If true, then the repo cannot be opened by this process. func LockedByOtherProcess(repoPath string) bool { + repoPath = path.Clean(repoPath) + + // packageLock must be held to check the number of openers. packageLock.Lock() defer packageLock.Unlock() + // NB: the lock is only held when repos are Open return lockfile.Locked(repoPath) && openerCounter.NumOpeners(repoPath) == 0 } // Open returns an error if the repo is not initialized. func (r *FSRepo) Open() error { + + // packageLock must be held to make sure that the repo is not destroyed by + // another caller. It must not be released until initialization is complete + // and the number of openers is incremeneted. packageLock.Lock() defer packageLock.Unlock() + if r.state != unopened { return debugerror.Errorf("repo is %s", r.state) } @@ -154,8 +177,15 @@ func (r *FSRepo) Open() error { // // 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 + + // It is not necessary to hold the package lock since the repo is in an + // opened state. The package lock is _not_ meant to ensure that the repo is + // thread-safe. The package lock is only meant to guard againt removal and + // coordinate the lockfile. However, we provide thread-safety to keep + // things simple. + packageLock.Lock() + defer packageLock.Unlock() + if r.state != opened { panic(fmt.Sprintln("repo is", r.state)) } @@ -164,37 +194,19 @@ 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)) - } - configFilename, err := config.Filename(r.path) - if err != nil { - return err - } - // to avoid clobbering user-provided keys, must read the config from disk - // as a map, write the updated struct values to the map and write the map - // to disk. - var mapconf map[string]interface{} - if err := readConfigFile(configFilename, &mapconf); err != nil { - return err - } - m, err := config.ToMap(updated) - if err != nil { - return err - } - for k, v := range m { - mapconf[k] = v - } - if err := writeConfigFile(configFilename, mapconf); err != nil { - return err - } - *r.config = *updated // copy so caller cannot modify this private config - return nil + + // packageLock is held to provide thread-safety. + packageLock.Lock() + defer packageLock.Unlock() + + return r.setConfigUnsynced(updated) } // GetConfigKey retrieves only the value of a particular key. func (r *FSRepo) GetConfigKey(key string) (interface{}, error) { + packageLock.Lock() + defer packageLock.Unlock() + if r.state != opened { return nil, debugerror.Errorf("repo is %s", r.state) } @@ -211,7 +223,9 @@ 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 + packageLock.Lock() + defer packageLock.Unlock() + if r.state != opened { return debugerror.Errorf("repo is %s", r.state) } @@ -233,13 +247,14 @@ func (r *FSRepo) SetConfigKey(key string, value interface{}) error { if err != nil { return err } - return r.SetConfig(conf) + return r.setConfigUnsynced(conf) } // Close closes the FSRepo, releasing held resources. func (r *FSRepo) Close() error { packageLock.Lock() defer packageLock.Unlock() + if r.state != opened { return debugerror.Errorf("repo is %s", r.state) } @@ -251,11 +266,15 @@ var _ repo.Repo = &FSRepo{} // IsInitialized returns true if the repo is initialized at provided |path|. func IsInitialized(path string) bool { + // packageLock is held to ensure that another caller doesn't attempt to + // Init or Remove the repo while this call is in progress. packageLock.Lock() defer packageLock.Unlock() return isInitializedUnsynced(path) } +// private methods below this point. NB: packageLock must held by caller. + // isInitializedUnsynced reports whether the repo is initialized. Caller must // hold openerCounter lock. func isInitializedUnsynced(path string) bool { @@ -317,3 +336,33 @@ func transitionToClosed(r *FSRepo) error { } return nil } + +// setConfigUnsynced is for private use. Callers must hold the packageLock. +func (r *FSRepo) setConfigUnsynced(updated *config.Config) error { + if r.state != opened { + return fmt.Errorf("repo is", r.state) + } + configFilename, err := config.Filename(r.path) + if err != nil { + return err + } + // to avoid clobbering user-provided keys, must read the config from disk + // as a map, write the updated struct values to the map and write the map + // to disk. + var mapconf map[string]interface{} + if err := readConfigFile(configFilename, &mapconf); err != nil { + return err + } + m, err := config.ToMap(updated) + if err != nil { + return err + } + for k, v := range m { + mapconf[k] = v + } + if err := writeConfigFile(configFilename, mapconf); err != nil { + return err + } + *r.config = *updated // copy so caller cannot modify this private config + return nil +}