From 9a7fbe32101bce9487b772d944ec9f59fb4344fc Mon Sep 17 00:00:00 2001 From: Brian Tiger Chow Date: Tue, 13 Jan 2015 05:00:29 -0800 Subject: [PATCH 01/29] doc(fsrepo): Roadmap --- repo/fsrepo/doc.go | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) create mode 100644 repo/fsrepo/doc.go diff --git a/repo/fsrepo/doc.go b/repo/fsrepo/doc.go new file mode 100644 index 000000000..5fd5b077f --- /dev/null +++ b/repo/fsrepo/doc.go @@ -0,0 +1,20 @@ +// package fsrepo +// +// TODO explain the package roadmap... +// +// .go-ipfs/ +// ├── client/ +// | ├── client.lock <------ protects client/ + signals its own pid +// │ ├── ipfs-client.cpuprof +// │ ├── ipfs-client.memprof +// │ └── logs/ +// ├── config +// ├── daemon/ +// │ ├── daemon.lock <------ protects daemon/ + signals its own address +// │ ├── ipfs-daemon.cpuprof +// │ ├── ipfs-daemon.memprof +// │ └── logs/ +// ├── datastore/ +// ├── repo.lock <------ protects datastore/ and config +// └── version +package fsrepo From acbd9a220459c8397e971e879e70ac78ebdbac8b Mon Sep 17 00:00:00 2001 From: Brian Tiger Chow Date: Tue, 13 Jan 2015 06:19:46 -0800 Subject: [PATCH 02/29] rm unused function --- repo/fsrepo/serialize.go | 17 ----------------- 1 file changed, 17 deletions(-) diff --git a/repo/fsrepo/serialize.go b/repo/fsrepo/serialize.go index 9e291f586..c152b2936 100644 --- a/repo/fsrepo/serialize.go +++ b/repo/fsrepo/serialize.go @@ -43,23 +43,6 @@ func writeConfigFile(filename string, cfg interface{}) error { return encode(f, cfg) } -// writeFile writes the buffer at filename -func writeFile(filename string, buf []byte) error { - err := os.MkdirAll(filepath.Dir(filename), 0775) - if err != nil { - return err - } - - f, err := os.Create(filename) - if err != nil { - return err - } - defer f.Close() - - _, err = f.Write(buf) - return err -} - // encode configuration with JSON func encode(w io.Writer, value interface{}) error { // need to prettyprint, hence MarshalIndent, instead of Encoder From aacf8719554c50c5e98a4f15f4f2c54a600d3c7a Mon Sep 17 00:00:00 2001 From: Brian Tiger Chow Date: Tue, 13 Jan 2015 06:23:55 -0800 Subject: [PATCH 03/29] officially move to Go 1.4 @jbenet @whyrusleeping thoughts? --- Godeps/Godeps.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Godeps/Godeps.json b/Godeps/Godeps.json index 96c593128..ec16d0029 100644 --- a/Godeps/Godeps.json +++ b/Godeps/Godeps.json @@ -1,6 +1,6 @@ { "ImportPath": "github.com/jbenet/go-ipfs", - "GoVersion": "go1.3", + "GoVersion": "go1.4", "Packages": [ "./..." ], From 9700d2f94bf3f2ae6e0e031d282fc7ad6068a367 Mon Sep 17 00:00:00 2001 From: Brian Tiger Chow Date: Tue, 13 Jan 2015 06:27:24 -0800 Subject: [PATCH 04/29] use atomicfile for safer writes for now, allow daemon and client to both hit config --- Godeps/Godeps.json | 4 + .../facebookgo/atomicfile/.travis.yml | 24 ++++++ .../facebookgo/atomicfile/atomicfile.go | 54 ++++++++++++ .../facebookgo/atomicfile/atomicfile_test.go | 86 +++++++++++++++++++ .../github.com/facebookgo/atomicfile/license | 30 +++++++ .../github.com/facebookgo/atomicfile/patents | 23 +++++ .../facebookgo/atomicfile/readme.md | 4 + repo/fsrepo/serialize.go | 3 +- 8 files changed, 227 insertions(+), 1 deletion(-) create mode 100644 Godeps/_workspace/src/github.com/facebookgo/atomicfile/.travis.yml create mode 100644 Godeps/_workspace/src/github.com/facebookgo/atomicfile/atomicfile.go create mode 100644 Godeps/_workspace/src/github.com/facebookgo/atomicfile/atomicfile_test.go create mode 100644 Godeps/_workspace/src/github.com/facebookgo/atomicfile/license create mode 100644 Godeps/_workspace/src/github.com/facebookgo/atomicfile/patents create mode 100644 Godeps/_workspace/src/github.com/facebookgo/atomicfile/readme.md diff --git a/Godeps/Godeps.json b/Godeps/Godeps.json index ec16d0029..cb5bf158f 100644 --- a/Godeps/Godeps.json +++ b/Godeps/Godeps.json @@ -68,6 +68,10 @@ "ImportPath": "github.com/dustin/go-humanize", "Rev": "b198514c204f20799b91c93b6ffd8b26be04c2c9" }, + { + "ImportPath": "github.com/facebookgo/atomicfile", + "Rev": "6f117f2e7f224fb03eb5e5fba370eade6e2b90c8" + }, { "ImportPath": "github.com/facebookgo/stack", "Rev": "4da6d991fc3c389efa512151354d643eb5fae4e2" diff --git a/Godeps/_workspace/src/github.com/facebookgo/atomicfile/.travis.yml b/Godeps/_workspace/src/github.com/facebookgo/atomicfile/.travis.yml new file mode 100644 index 000000000..2cc62c5e8 --- /dev/null +++ b/Godeps/_workspace/src/github.com/facebookgo/atomicfile/.travis.yml @@ -0,0 +1,24 @@ +language: go + +go: + - 1.2 + - 1.3 + +matrix: + fast_finish: true + +before_install: + - go get -v code.google.com/p/go.tools/cmd/vet + - go get -v github.com/golang/lint/golint + - go get -v code.google.com/p/go.tools/cmd/cover + +install: + - go install -race -v std + - go get -race -t -v ./... + - go install -race -v ./... + +script: + - go vet ./... + - $HOME/gopath/bin/golint . + - go test -cpu=2 -race -v ./... + - go test -cpu=2 -covermode=atomic ./... diff --git a/Godeps/_workspace/src/github.com/facebookgo/atomicfile/atomicfile.go b/Godeps/_workspace/src/github.com/facebookgo/atomicfile/atomicfile.go new file mode 100644 index 000000000..60cda2a5b --- /dev/null +++ b/Godeps/_workspace/src/github.com/facebookgo/atomicfile/atomicfile.go @@ -0,0 +1,54 @@ +// Package atomicfile provides the ability to write a file with an eventual +// rename on Close. This allows for a file to always be in a consistent state +// and never represent an in-progress write. +package atomicfile + +import ( + "io/ioutil" + "os" + "path/filepath" +) + +// File behaves like os.File, but does an atomic rename operation at Close. +type File struct { + *os.File + path string +} + +// New creates a new temporary file that will replace the file at the given +// path when Closed. +func New(path string, mode os.FileMode) (*File, error) { + f, err := ioutil.TempFile(filepath.Dir(path), filepath.Base(path)) + if err != nil { + return nil, err + } + if err := os.Chmod(f.Name(), mode); err != nil { + os.Remove(f.Name()) + return nil, err + } + return &File{File: f, path: path}, nil +} + +// Close the file replacing the configured file. +func (f *File) Close() error { + if err := f.File.Close(); err != nil { + return err + } + if err := os.Rename(f.Name(), f.path); err != nil { + return err + } + return nil +} + +// Abort closes the file and removes it instead of replacing the configured +// file. This is useful if after starting to write to the file you decide you +// don't want it anymore. +func (f *File) Abort() error { + if err := f.File.Close(); err != nil { + return err + } + if err := os.Remove(f.Name()); err != nil { + return err + } + return nil +} diff --git a/Godeps/_workspace/src/github.com/facebookgo/atomicfile/atomicfile_test.go b/Godeps/_workspace/src/github.com/facebookgo/atomicfile/atomicfile_test.go new file mode 100644 index 000000000..e18683a72 --- /dev/null +++ b/Godeps/_workspace/src/github.com/facebookgo/atomicfile/atomicfile_test.go @@ -0,0 +1,86 @@ +package atomicfile_test + +import ( + "bytes" + "io/ioutil" + "os" + "testing" + + "github.com/jbenet/go-ipfs/Godeps/_workspace/src/github.com/facebookgo/atomicfile" +) + +func test(t *testing.T, dir, prefix string) { + t.Parallel() + + tmpfile, err := ioutil.TempFile(dir, prefix) + if err != nil { + t.Fatal(err) + } + name := tmpfile.Name() + + if err := os.Remove(name); err != nil { + t.Fatal(err) + } + + defer os.Remove(name) + f, err := atomicfile.New(name, os.FileMode(0666)) + if err != nil { + t.Fatal(err) + } + f.Write([]byte("foo")) + if _, err := os.Stat(name); !os.IsNotExist(err) { + t.Fatal("did not expect file to exist") + } + if err := f.Close(); err != nil { + t.Fatal(err) + } + if _, err := os.Stat(name); err != nil { + t.Fatalf("expected file to exist: %s", err) + } +} + +func TestCurrentDir(t *testing.T) { + cwd, _ := os.Getwd() + test(t, cwd, "atomicfile-current-dir-") +} + +func TestRootTmpDir(t *testing.T) { + test(t, "/tmp", "atomicfile-root-tmp-dir-") +} + +func TestDefaultTmpDir(t *testing.T) { + test(t, "", "atomicfile-default-tmp-dir-") +} + +func TestAbort(t *testing.T) { + contents := []byte("the answer is 42") + t.Parallel() + tmpfile, err := ioutil.TempFile("", "atomicfile-abort-") + if err != nil { + t.Fatal(err) + } + name := tmpfile.Name() + if _, err := tmpfile.Write(contents); err != nil { + t.Fatal(err) + } + defer os.Remove(name) + + f, err := atomicfile.New(name, os.FileMode(0666)) + if err != nil { + t.Fatal(err) + } + f.Write([]byte("foo")) + if err := f.Abort(); err != nil { + t.Fatal(err) + } + if _, err := os.Stat(name); err != nil { + t.Fatalf("expected file to exist: %s", err) + } + actual, err := ioutil.ReadFile(name) + if err != nil { + t.Fatal(err) + } + if !bytes.Equal(contents, actual) { + t.Fatalf(`did not find expected "%s" instead found "%s"`, contents, actual) + } +} diff --git a/Godeps/_workspace/src/github.com/facebookgo/atomicfile/license b/Godeps/_workspace/src/github.com/facebookgo/atomicfile/license new file mode 100644 index 000000000..4ce34257c --- /dev/null +++ b/Godeps/_workspace/src/github.com/facebookgo/atomicfile/license @@ -0,0 +1,30 @@ +BSD License + +For atomicfile software + +Copyright (c) 2014, Facebook, Inc. All rights reserved. + +Redistribution and use in source and binary forms, with or without modification, +are permitted provided that the following conditions are met: + + * Redistributions of source code must retain the above copyright notice, this + list of conditions and the following disclaimer. + + * Redistributions in binary form must reproduce the above copyright notice, + this list of conditions and the following disclaimer in the documentation + and/or other materials provided with the distribution. + + * Neither the name Facebook nor the names of its contributors may be used to + endorse or promote products derived from this software without specific + prior written permission. + +THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND +ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED +WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE +DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR +ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES +(INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; +LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON +ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS +SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. diff --git a/Godeps/_workspace/src/github.com/facebookgo/atomicfile/patents b/Godeps/_workspace/src/github.com/facebookgo/atomicfile/patents new file mode 100644 index 000000000..887426cf1 --- /dev/null +++ b/Godeps/_workspace/src/github.com/facebookgo/atomicfile/patents @@ -0,0 +1,23 @@ +Additional Grant of Patent Rights + +"Software" means the atomicfile software distributed by Facebook, Inc. + +Facebook hereby grants you a perpetual, worldwide, royalty-free, non-exclusive, +irrevocable (subject to the termination provision below) license under any +rights in any patent claims owned by Facebook, to make, have made, use, sell, +offer to sell, import, and otherwise transfer the Software. For avoidance of +doubt, no license is granted under Facebook’s rights in any patent claims that +are infringed by (i) modifications to the Software made by you or a third party, +or (ii) the Software in combination with any software or other technology +provided by you or a third party. + +The license granted hereunder will terminate, automatically and without notice, +for anyone that makes any claim (including by filing any lawsuit, assertion or +other action) alleging (a) direct, indirect, or contributory infringement or +inducement to infringe any patent: (i) by Facebook or any of its subsidiaries or +affiliates, whether or not such claim is related to the Software, (ii) by any +party if such claim arises in whole or in part from any software, product or +service of Facebook or any of its subsidiaries or affiliates, whether or not +such claim is related to the Software, or (iii) by any party relating to the +Software; or (b) that any right in any patent claim of Facebook is invalid or +unenforceable. diff --git a/Godeps/_workspace/src/github.com/facebookgo/atomicfile/readme.md b/Godeps/_workspace/src/github.com/facebookgo/atomicfile/readme.md new file mode 100644 index 000000000..80038c3e0 --- /dev/null +++ b/Godeps/_workspace/src/github.com/facebookgo/atomicfile/readme.md @@ -0,0 +1,4 @@ +atomicfile [![Build Status](https://secure.travis-ci.org/facebookgo/atomicfile.png)](http://travis-ci.org/facebookgo/atomicfile) +========== + +Documentation: http://godoc.org/github.com/facebookgo/atomicfile diff --git a/repo/fsrepo/serialize.go b/repo/fsrepo/serialize.go index c152b2936..dcbf4ccf1 100644 --- a/repo/fsrepo/serialize.go +++ b/repo/fsrepo/serialize.go @@ -7,6 +7,7 @@ import ( "os" "path/filepath" + "github.com/jbenet/go-ipfs/Godeps/_workspace/src/github.com/facebookgo/atomicfile" "github.com/jbenet/go-ipfs/repo/config" "github.com/jbenet/go-ipfs/util" "github.com/jbenet/go-ipfs/util/debugerror" @@ -34,7 +35,7 @@ func writeConfigFile(filename string, cfg interface{}) error { return err } - f, err := os.Create(filename) + f, err := atomicfile.New(filename, 0775) if err != nil { return err } From 67c161fb7282dfc9fbae00a7fba8aa5804557d82 Mon Sep 17 00:00:00 2001 From: Brian Tiger Chow Date: Tue, 13 Jan 2015 17:12:00 -0800 Subject: [PATCH 05/29] doc todo --- repo/fsrepo/doc.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/repo/fsrepo/doc.go b/repo/fsrepo/doc.go index 5fd5b077f..cbed3fd98 100644 --- a/repo/fsrepo/doc.go +++ b/repo/fsrepo/doc.go @@ -18,3 +18,5 @@ // ├── repo.lock <------ protects datastore/ and config // └── version package fsrepo + +// TODO prevent multiple daemons from running From 40e41d24f7fe57d33e3d73107653a1c50f4340c7 Mon Sep 17 00:00:00 2001 From: Brian Tiger Chow Date: Tue, 13 Jan 2015 17:19:31 -0800 Subject: [PATCH 06/29] feat(fsrepo): protect with a repo lockfile NB: daemon is one spot the repo lock is typically acquired --- cmd/ipfs/daemon.go | 10 ++-- cmd/ipfs/main.go | 3 +- repo/fsrepo/fsrepo.go | 53 +++++++++++++++++-- .../daemon.go => repo/fsrepo/lock/lock.go | 4 +- repo/fsrepo/opener/counter.go | 6 ++- 5 files changed, 60 insertions(+), 16 deletions(-) rename core/daemon/daemon.go => repo/fsrepo/lock/lock.go (92%) diff --git a/cmd/ipfs/daemon.go b/cmd/ipfs/daemon.go index 9f1ec8462..1dd5dc369 100644 --- a/cmd/ipfs/daemon.go +++ b/cmd/ipfs/daemon.go @@ -12,7 +12,7 @@ import ( cmdsHttp "github.com/jbenet/go-ipfs/commands/http" core "github.com/jbenet/go-ipfs/core" commands "github.com/jbenet/go-ipfs/core/commands" - daemon "github.com/jbenet/go-ipfs/core/daemon" + fsrepo "github.com/jbenet/go-ipfs/repo/fsrepo" util "github.com/jbenet/go-ipfs/util" "github.com/jbenet/go-ipfs/util/debugerror" ) @@ -89,13 +89,13 @@ func daemonFunc(req cmds.Request) (interface{}, error) { return nil, err } - // acquire the daemon lock _before_ constructing a node. we need to make + // acquire the repo lock _before_ constructing a node. we need to make // sure we are permitted to access the resources (datastore, etc.) - lock, err := daemon.Lock(req.Context().ConfigRoot) - if err != nil { + repo := fsrepo.At(req.Context().ConfigRoot) + if err := repo.Open(); err != nil { return nil, debugerror.Errorf("Couldn't obtain lock. Is another daemon already running?") } - defer lock.Close() + defer repo.Close() // OK!!! Now we're ready to construct the node. // make sure we construct an online node. diff --git a/cmd/ipfs/main.go b/cmd/ipfs/main.go index 561598c54..0dc7ddf0f 100644 --- a/cmd/ipfs/main.go +++ b/cmd/ipfs/main.go @@ -20,7 +20,6 @@ import ( cmdsCli "github.com/jbenet/go-ipfs/commands/cli" cmdsHttp "github.com/jbenet/go-ipfs/commands/http" core "github.com/jbenet/go-ipfs/core" - daemon "github.com/jbenet/go-ipfs/core/daemon" repo "github.com/jbenet/go-ipfs/repo" config "github.com/jbenet/go-ipfs/repo/config" fsrepo "github.com/jbenet/go-ipfs/repo/fsrepo" @@ -392,7 +391,7 @@ func commandShouldRunOnDaemon(details cmdDetails, req cmds.Request, root *cmds.C // at this point need to know whether daemon is running. we defer // to this point so that some commands dont open files unnecessarily. - daemonLocked := daemon.Locked(req.Context().ConfigRoot) + daemonLocked := fsrepo.LockedByOtherProcess(req.Context().ConfigRoot) if daemonLocked { diff --git a/repo/fsrepo/fsrepo.go b/repo/fsrepo/fsrepo.go index 5698acd44..877578770 100644 --- a/repo/fsrepo/fsrepo.go +++ b/repo/fsrepo/fsrepo.go @@ -10,6 +10,7 @@ import ( repo "github.com/jbenet/go-ipfs/repo" common "github.com/jbenet/go-ipfs/repo/common" config "github.com/jbenet/go-ipfs/repo/config" + lockfile "github.com/jbenet/go-ipfs/repo/fsrepo/lock" opener "github.com/jbenet/go-ipfs/repo/fsrepo/opener" util "github.com/jbenet/go-ipfs/util" debugerror "github.com/jbenet/go-ipfs/util/debugerror" @@ -24,10 +25,13 @@ var ( // 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. openerCounter *opener.Counter + + lockfiles map[string]io.Closer ) func init() { openerCounter = opener.NewCounter() + lockfiles = make(map[string]io.Closer) } // FSRepo represents an IPFS FileSystem Repo. It is not thread-safe. @@ -74,6 +78,15 @@ func Remove(path string) error { return os.RemoveAll(path) } +// 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 { + openerCounter.Lock() + defer openerCounter.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 { openerCounter.Lock() @@ -118,9 +131,7 @@ func (r *FSRepo) Open() error { return debugerror.Errorf("logs: %s", err) } - r.state = opened - openerCounter.AddOpener(r.path) - return nil + return transitionToOpened(r) } // Config returns the FSRepo's config. This method must not be called if the @@ -217,8 +228,7 @@ func (r *FSRepo) Close() error { if r.state != opened { return debugerror.Errorf("repo is %s", r.state) } - openerCounter.RemoveOpener(r.path) - return nil // TODO release repo lock + return transitionToClosed(r) } var _ io.Closer = &FSRepo{} @@ -258,3 +268,36 @@ func initCheckDir(path string) error { } return nil } + +// transitionToOpened manages the state transition to |opened|. Caller must hold +// openerCounter lock. +func transitionToOpened(r *FSRepo) error { + r.state = opened + if countBefore := openerCounter.NumOpeners(r.path); countBefore == 0 { // #first + closer, err := lockfile.Lock(r.path) + if err != nil { + return err + } + lockfiles[r.path] = closer + } + return openerCounter.AddOpener(r.path) +} + +// transitionToClosed manages the state transition to |closed|. Caller must +// hold openerCounter lock. +func transitionToClosed(r *FSRepo) error { + r.state = closed + if err := openerCounter.RemoveOpener(r.path); err != nil { + return err + } + if countAfter := openerCounter.NumOpeners(r.path); countAfter == 0 { + closer, ok := lockfiles[r.path] + if !ok { + return errors.New("package error: lockfile is not held") + } + if err := closer.Close(); err != nil { + return err + } + } + return nil +} diff --git a/core/daemon/daemon.go b/repo/fsrepo/lock/lock.go similarity index 92% rename from core/daemon/daemon.go rename to repo/fsrepo/lock/lock.go index f1c25fc7b..53a578b6b 100644 --- a/core/daemon/daemon.go +++ b/repo/fsrepo/lock/lock.go @@ -1,4 +1,4 @@ -package daemon +package lock import ( "io" @@ -10,6 +10,7 @@ import ( ) // LockFile is the filename of the daemon lock, relative to config dir +// TODO rename repo lock and hide name const LockFile = "daemon.lock" func Lock(confdir string) (io.Closer, error) { @@ -23,7 +24,6 @@ func Locked(confdir string) bool { } if lk, err := Lock(confdir); err != nil { return true - } else { lk.Close() return false diff --git a/repo/fsrepo/opener/counter.go b/repo/fsrepo/opener/counter.go index a9d034329..b2a43c230 100644 --- a/repo/fsrepo/opener/counter.go +++ b/repo/fsrepo/opener/counter.go @@ -38,15 +38,17 @@ func (l *Counter) NumOpeners(repoPath string) int { // 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 *Counter) AddOpener(repoPath string) { +func (l *Counter) AddOpener(repoPath string) error { l.repos[key(repoPath)]++ + return nil } // 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 *Counter) RemoveOpener(repoPath string) { +func (l *Counter) RemoveOpener(repoPath string) error { l.repos[key(repoPath)]-- + return nil } func key(repoPath string) string { From 76202a94442e1fdf95dae6da3e0c766c31ed5b13 Mon Sep 17 00:00:00 2001 From: Brian Tiger Chow Date: Tue, 13 Jan 2015 17:27:52 -0800 Subject: [PATCH 07/29] fix(repo): clean the path before using it no issue detected but it's good to be safe --- repo/fsrepo/fsrepo.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/repo/fsrepo/fsrepo.go b/repo/fsrepo/fsrepo.go index 877578770..c8cfd91d9 100644 --- a/repo/fsrepo/fsrepo.go +++ b/repo/fsrepo/fsrepo.go @@ -5,6 +5,7 @@ import ( "fmt" "io" "os" + "path" "path/filepath" repo "github.com/jbenet/go-ipfs/repo" @@ -42,10 +43,10 @@ type FSRepo struct { } // At returns a handle to an FSRepo at the provided |path|. -func At(path string) *FSRepo { +func At(repoPath string) *FSRepo { // This method must not have side-effects. return &FSRepo{ - path: path, + path: path.Clean(repoPath), state: unopened, // explicitly set for clarity } } From 6ec20b35749c3f9389b8d042b05616a4a38bbb36 Mon Sep 17 00:00:00 2001 From: Brian Tiger Chow Date: Tue, 13 Jan 2015 17:34:24 -0800 Subject: [PATCH 08/29] huh --- cmd/ipfs/main.go | 7 +------ cmd/ipfs/tour.go | 1 + repo/fsrepo/fsrepo.go | 8 ++++++++ repo/fsrepo/serialize.go | 1 + 4 files changed, 11 insertions(+), 6 deletions(-) diff --git a/cmd/ipfs/main.go b/cmd/ipfs/main.go index 0dc7ddf0f..550d9b605 100644 --- a/cmd/ipfs/main.go +++ b/cmd/ipfs/main.go @@ -444,12 +444,7 @@ func getConfigRoot(req cmds.Request) (string, error) { } func loadConfig(path string) (*config.Config, error) { - r := fsrepo.At(path) - if err := r.Open(); err != nil { - return nil, err - } - defer r.Close() - return r.Config(), nil + return fsrepo.ConfigAt(path) } // startProfiling begins CPU profiling and returns a `stop` function to be diff --git a/cmd/ipfs/tour.go b/cmd/ipfs/tour.go index b61cc50f7..168bacc9c 100644 --- a/cmd/ipfs/tour.go +++ b/cmd/ipfs/tour.go @@ -188,6 +188,7 @@ func tourGet(id tour.ID) (*tour.Topic, error) { // TODO share func func writeConfig(path string, cfg *config.Config) error { + // NB: This needs to run on the daemon. r := fsrepo.At(path) if err := r.Open(); err != nil { return err diff --git a/repo/fsrepo/fsrepo.go b/repo/fsrepo/fsrepo.go index c8cfd91d9..e73524073 100644 --- a/repo/fsrepo/fsrepo.go +++ b/repo/fsrepo/fsrepo.go @@ -51,6 +51,14 @@ func At(repoPath string) *FSRepo { } } +func ConfigAt(repoPath string) (*config.Config, error) { + configFilename, err := config.Filename(repoPath) + if err != nil { + return nil, err + } + return load(configFilename) +} + // Init initializes a new FSRepo at the given path with the provided config. func Init(path string, conf *config.Config) error { openerCounter.Lock() // lock must be held to ensure atomicity (prevent Removal) diff --git a/repo/fsrepo/serialize.go b/repo/fsrepo/serialize.go index dcbf4ccf1..559c04804 100644 --- a/repo/fsrepo/serialize.go +++ b/repo/fsrepo/serialize.go @@ -69,6 +69,7 @@ func load(filename string) (*config.Config, error) { } // tilde expansion on datastore path + // TODO why is this here?? cfg.Datastore.Path, err = util.TildeExpansion(cfg.Datastore.Path) if err != nil { return nil, err From 53e6a9bd1ad433cf49a3cffb8a3dffe3ca407d15 Mon Sep 17 00:00:00 2001 From: Brian Tiger Chow Date: Tue, 13 Jan 2015 20:43:35 -0800 Subject: [PATCH 09/29] fix(fsrepo): remove the Closer after closing it. --- repo/fsrepo/fsrepo.go | 1 + 1 file changed, 1 insertion(+) diff --git a/repo/fsrepo/fsrepo.go b/repo/fsrepo/fsrepo.go index e73524073..ce7c09fc6 100644 --- a/repo/fsrepo/fsrepo.go +++ b/repo/fsrepo/fsrepo.go @@ -307,6 +307,7 @@ func transitionToClosed(r *FSRepo) error { if err := closer.Close(); err != nil { return err } + delete(lockfiles, r.path) } return nil } From a3d236269108d759359109cc9d91922057cd58d3 Mon Sep 17 00:00:00 2001 From: Brian Tiger Chow Date: Tue, 13 Jan 2015 20:46:32 -0800 Subject: [PATCH 10/29] use a single coarse package lock --- repo/fsrepo/fsrepo.go | 38 ++++++++++++++++++++--------------- repo/fsrepo/opener/counter.go | 19 +++--------------- 2 files changed, 25 insertions(+), 32 deletions(-) diff --git a/repo/fsrepo/fsrepo.go b/repo/fsrepo/fsrepo.go index ce7c09fc6..2a9f8a10a 100644 --- a/repo/fsrepo/fsrepo.go +++ b/repo/fsrepo/fsrepo.go @@ -7,6 +7,7 @@ import ( "os" "path" "path/filepath" + "sync" repo "github.com/jbenet/go-ipfs/repo" common "github.com/jbenet/go-ipfs/repo/common" @@ -18,6 +19,13 @@ import ( ) var ( + + // packageLock must be held to while performing any operation that modifies an + // FSRepo's state field. This includes Init, Open, Close, and Remove. + packageLock sync.Mutex // protects openerCounter and lockfiles + // lockfiles holds references to the Closers that ensure that repos are + // only accessed by one process at a time. + lockfiles map[string]io.Closer // openerCounter prevents the fsrepo from being removed while there exist open // FSRepo handles. It also ensures that the Init is atomic. // @@ -26,8 +34,6 @@ var ( // 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. openerCounter *opener.Counter - - lockfiles map[string]io.Closer ) func init() { @@ -61,8 +67,8 @@ 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 { - openerCounter.Lock() // lock must be held to ensure atomicity (prevent Removal) - defer openerCounter.Unlock() + packageLock.Lock() // lock must be held to ensure atomicity (prevent Removal) + defer packageLock.Unlock() if isInitializedUnsynced(path) { return nil @@ -79,8 +85,8 @@ func Init(path string, conf *config.Config) error { // Remove recursively removes the FSRepo at |path|. func Remove(path string) error { - openerCounter.Lock() - defer openerCounter.Unlock() + packageLock.Lock() + defer packageLock.Unlock() if openerCounter.NumOpeners(path) != 0 { return errors.New("repo in use") } @@ -90,16 +96,16 @@ func Remove(path string) error { // 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 { - openerCounter.Lock() - defer openerCounter.Unlock() + 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 { - openerCounter.Lock() - defer openerCounter.Unlock() + packageLock.Lock() + defer packageLock.Unlock() if r.state != unopened { return debugerror.Errorf("repo is %s", r.state) } @@ -232,8 +238,8 @@ func (r *FSRepo) SetConfigKey(key string, value interface{}) error { // Close closes the FSRepo, releasing held resources. func (r *FSRepo) Close() error { - openerCounter.Lock() - defer openerCounter.Unlock() + packageLock.Lock() + defer packageLock.Unlock() if r.state != opened { return debugerror.Errorf("repo is %s", r.state) } @@ -245,8 +251,8 @@ var _ repo.Repo = &FSRepo{} // IsInitialized returns true if the repo is initialized at provided |path|. func IsInitialized(path string) bool { - openerCounter.Lock() - defer openerCounter.Unlock() + packageLock.Lock() + defer packageLock.Unlock() return isInitializedUnsynced(path) } @@ -279,7 +285,7 @@ func initCheckDir(path string) error { } // transitionToOpened manages the state transition to |opened|. Caller must hold -// openerCounter lock. +// the package mutex. func transitionToOpened(r *FSRepo) error { r.state = opened if countBefore := openerCounter.NumOpeners(r.path); countBefore == 0 { // #first @@ -293,7 +299,7 @@ func transitionToOpened(r *FSRepo) error { } // transitionToClosed manages the state transition to |closed|. Caller must -// hold openerCounter lock. +// hold the package mutex. func transitionToClosed(r *FSRepo) error { r.state = closed if err := openerCounter.RemoveOpener(r.path); err != nil { diff --git a/repo/fsrepo/opener/counter.go b/repo/fsrepo/opener/counter.go index b2a43c230..51cf3bee1 100644 --- a/repo/fsrepo/opener/counter.go +++ b/repo/fsrepo/opener/counter.go @@ -1,13 +1,10 @@ package fsrepo -import ( - "path" - "sync" -) +import "path" + +// TODO this could be made into something more generic. type Counter 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 @@ -19,16 +16,6 @@ func NewCounter() *Counter { } } -// 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 *Counter) Lock() { - l.lock.Lock() -} - -func (l *Counter) 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. From 6ec60ba861c6e39ba47b16ac8bea4bcd5e71f1f5 Mon Sep 17 00:00:00 2001 From: Brian Tiger Chow Date: Tue, 13 Jan 2015 21:13:33 -0800 Subject: [PATCH 11/29] 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 +} From 70dab069bd13f03c06737363b3d4b6bf6e476d92 Mon Sep 17 00:00:00 2001 From: Brian Tiger Chow Date: Wed, 14 Jan 2015 04:02:32 -0800 Subject: [PATCH 12/29] doc(fsrepo) --- repo/fsrepo/fsrepo.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/repo/fsrepo/fsrepo.go b/repo/fsrepo/fsrepo.go index 6e422c201..36bf123fc 100644 --- a/repo/fsrepo/fsrepo.go +++ b/repo/fsrepo/fsrepo.go @@ -86,6 +86,9 @@ func Init(path string, conf *config.Config) error { if err != nil { return err } + // initialization is the one time when it's okay to write to the config + // without reading the config from disk and merging any user-provided keys + // that may exist. if err := writeConfigFile(configFilename, conf); err != nil { return err } From ac530d0ab6a01c81420d8804c3fb6ad71c3c4ebb Mon Sep 17 00:00:00 2001 From: Brian Tiger Chow Date: Wed, 14 Jan 2015 04:06:47 -0800 Subject: [PATCH 13/29] refactor(fsrepo) move Close under Open --- repo/fsrepo/fsrepo.go | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/repo/fsrepo/fsrepo.go b/repo/fsrepo/fsrepo.go index 36bf123fc..fa70acd86 100644 --- a/repo/fsrepo/fsrepo.go +++ b/repo/fsrepo/fsrepo.go @@ -72,6 +72,7 @@ func ConfigAt(repoPath string) (*config.Config, error) { } // Init initializes a new FSRepo at the given path with the provided config. +// TODO add support for custom datastores. func Init(path string, conf *config.Config) error { // packageLock must be held to ensure that the repo is not initialized more @@ -175,6 +176,17 @@ func (r *FSRepo) Open() error { return transitionToOpened(r) } +// 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) + } + return transitionToClosed(r) +} + // Config returns the FSRepo's config. This method must not be called if the // repo is not open. // @@ -253,17 +265,6 @@ func (r *FSRepo) SetConfigKey(key string, value interface{}) error { 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) - } - return transitionToClosed(r) -} - var _ io.Closer = &FSRepo{} var _ repo.Repo = &FSRepo{} From 9a054c5800d6e799bbd7cc2f9d76055aab9c57b6 Mon Sep 17 00:00:00 2001 From: Brian Tiger Chow Date: Wed, 14 Jan 2015 04:26:19 -0800 Subject: [PATCH 14/29] doc(fsrepo) --- repo/fsrepo/fsrepo.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/repo/fsrepo/fsrepo.go b/repo/fsrepo/fsrepo.go index fa70acd86..007deeb7b 100644 --- a/repo/fsrepo/fsrepo.go +++ b/repo/fsrepo/fsrepo.go @@ -44,8 +44,13 @@ func init() { // FSRepo represents an IPFS FileSystem Repo. It is safe for use by multiple // callers. type FSRepo struct { - state state - path string + // state is the FSRepo's state (unopened, opened, closed) + state state + // path is the file-system path + path string + // config is loaded when FSRepo is opened and kept up to date when the + // FSRepo is modified. + // TODO test config *config.Config } From 9acc02461ebe739cb0e73a7b8e819404840d81fd Mon Sep 17 00:00:00 2001 From: Brian Tiger Chow Date: Wed, 14 Jan 2015 05:42:35 -0800 Subject: [PATCH 15/29] test(fsrepo) harden tests --- repo/fsrepo/fsrepo_test.go | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/repo/fsrepo/fsrepo_test.go b/repo/fsrepo/fsrepo_test.go index dde675a8f..caa473607 100644 --- a/repo/fsrepo/fsrepo_test.go +++ b/repo/fsrepo/fsrepo_test.go @@ -30,14 +30,18 @@ func TestCanManageReposIndependently(t *testing.T) { pathB := testRepoPath("b", t) t.Log("initialize two repos") - AssertNil(Init(pathA, &config.Config{}), t, "should initialize successfully") - AssertNil(Init(pathB, &config.Config{}), t, "should initialize successfully") + AssertNil(Init(pathA, &config.Config{}), t, "a", "should initialize successfully") + AssertNil(Init(pathB, &config.Config{}), t, "b", "should initialize successfully") + + t.Log("ensure repos initialized") + Assert(IsInitialized(pathA), t, "a should be initialized") + Assert(IsInitialized(pathB), t, "b should be initialized") t.Log("open the two repos") repoA := At(pathA) repoB := At(pathB) - AssertNil(repoA.Open(), t) - AssertNil(repoB.Open(), t) + AssertNil(repoA.Open(), t, "a") + AssertNil(repoB.Open(), t, "b") t.Log("close and remove b while a is open") AssertNil(repoB.Close(), t, "close b") @@ -50,12 +54,18 @@ func TestCanManageReposIndependently(t *testing.T) { func AssertNil(err error, t *testing.T, msgs ...string) { if err != nil { - t.Error(msgs, "error:", err) + t.Fatal(msgs, "error:", err) + } +} + +func Assert(v bool, t *testing.T, msgs ...string) { + if !v { + t.Fatal(msgs) } } func AssertErr(err error, t *testing.T, msgs ...string) { if err == nil { - t.Error(msgs, "error:", err) + t.Fatal(msgs, "error:", err) } } From c364b4c34c37c5a30042b4fca58c313421eefc55 Mon Sep 17 00:00:00 2001 From: Brian Tiger Chow Date: Wed, 14 Jan 2015 06:00:29 -0800 Subject: [PATCH 16/29] refactor(fsrepo): extract configCompoenent The struct was getting unmanageable. extracted the config component to reduce complexity. The datastore will be written as another component. --- repo/fsrepo/config_component.go | 145 ++++++++++++++++++++++++++++++++ repo/fsrepo/fsrepo.go | 131 ++++++++++------------------- 2 files changed, 189 insertions(+), 87 deletions(-) create mode 100644 repo/fsrepo/config_component.go diff --git a/repo/fsrepo/config_component.go b/repo/fsrepo/config_component.go new file mode 100644 index 000000000..57781cc4d --- /dev/null +++ b/repo/fsrepo/config_component.go @@ -0,0 +1,145 @@ +package fsrepo + +import ( + common "github.com/jbenet/go-ipfs/repo/common" + config "github.com/jbenet/go-ipfs/repo/config" + util "github.com/jbenet/go-ipfs/util" +) + +var _ component = &configComponent{} + +// configComponent abstracts the config component of the FSRepo. +// NB: create with makeConfigComponent function. +type configComponent struct { + path string // required at instantiation + config *config.Config // assigned on Open() +} + +// makeConfigComponent instantiates a valid configComponent. +func makeConfigComponent(path string) configComponent { + return configComponent{path: path} +} + +// fsrepoConfigInit initializes the FSRepo's configComponent. +func initConfigComponent(path string, conf *config.Config) error { + if configComponentIsInitialized(path) { + return nil + } + configFilename, err := config.Filename(path) + if err != nil { + return err + } + // initialization is the one time when it's okay to write to the config + // without reading the config from disk and merging any user-provided keys + // that may exist. + if err := writeConfigFile(configFilename, conf); err != nil { + return err + } + return nil +} + +// Open returns an error if the config file is not present. +func (c *configComponent) Open() error { + configFilename, err := config.Filename(c.path) + if err != nil { + return err + } + conf, err := load(configFilename) + if err != nil { + return err + } + c.config = conf + return nil +} + +// Close satisfies the fsrepoComponent interface. +func (c *configComponent) Close() error { + return nil // config doesn't need to be closed. +} + +func (c *configComponent) Config() *config.Config { + return c.config +} + +// SetConfig updates the config file. +func (c *configComponent) SetConfig(updated *config.Config) error { + return c.setConfigUnsynced(updated) +} + +// GetConfigKey retrieves only the value of a particular key. +func (c *configComponent) GetConfigKey(key string) (interface{}, error) { + filename, err := config.Filename(c.path) + if err != nil { + return nil, err + } + var cfg map[string]interface{} + if err := readConfigFile(filename, &cfg); err != nil { + return nil, err + } + return common.MapGetKV(cfg, key) +} + +// SetConfigKey writes the value of a particular key. +func (c *configComponent) SetConfigKey(key string, value interface{}) error { + filename, err := config.Filename(c.path) + if err != nil { + return err + } + var mapconf map[string]interface{} + if err := readConfigFile(filename, &mapconf); err != nil { + return err + } + if err := common.MapSetKV(mapconf, key, value); err != nil { + return err + } + if err := writeConfigFile(filename, mapconf); err != nil { + return err + } + // in order to get the updated values, read updated config from the + // file-system. + conf, err := config.FromMap(mapconf) + if err != nil { + return err + } + return c.setConfigUnsynced(conf) // TODO roll this into this method +} + +// configComponentIsInitialized returns true if the repo is initialized at +// provided |path|. +func configComponentIsInitialized(path string) bool { + configFilename, err := config.Filename(path) + if err != nil { + return false + } + if !util.FileExists(configFilename) { + return false + } + return true +} + +// setConfigUnsynced is for private use. +func (r *configComponent) setConfigUnsynced(updated *config.Config) error { + 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 +} diff --git a/repo/fsrepo/fsrepo.go b/repo/fsrepo/fsrepo.go index 007deeb7b..caeef670b 100644 --- a/repo/fsrepo/fsrepo.go +++ b/repo/fsrepo/fsrepo.go @@ -10,11 +10,9 @@ import ( "sync" repo "github.com/jbenet/go-ipfs/repo" - common "github.com/jbenet/go-ipfs/repo/common" config "github.com/jbenet/go-ipfs/repo/config" lockfile "github.com/jbenet/go-ipfs/repo/fsrepo/lock" opener "github.com/jbenet/go-ipfs/repo/fsrepo/opener" - util "github.com/jbenet/go-ipfs/util" debugerror "github.com/jbenet/go-ipfs/util/debugerror" ) @@ -51,15 +49,21 @@ type FSRepo struct { // config is loaded when FSRepo is opened and kept up to date when the // FSRepo is modified. // TODO test - config *config.Config + configComponent configComponent +} + +type component interface { + Open() error + io.Closer } // At returns a handle to an FSRepo at the provided |path|. func At(repoPath string) *FSRepo { // This method must not have side-effects. return &FSRepo{ - path: path.Clean(repoPath), - state: unopened, // explicitly set for clarity + path: path.Clean(repoPath), + configComponent: makeConfigComponent(repoPath), + state: unopened, // explicitly set for clarity } } @@ -88,16 +92,10 @@ func Init(path string, conf *config.Config) error { if isInitializedUnsynced(path) { return nil } - configFilename, err := config.Filename(path) - if err != nil { - return err - } - // initialization is the one time when it's okay to write to the config - // without reading the config from disk and merging any user-provided keys - // that may exist. - if err := writeConfigFile(configFilename, conf); err != nil { + if err := initConfigComponent(path, conf); err != nil { return err } + return nil } @@ -151,15 +149,11 @@ func (r *FSRepo) Open() error { return err } - configFilename, err := config.Filename(r.path) - if err != nil { - return err + for _, opener := range r.components() { + if err := opener.Open(); err != nil { + return err + } } - conf, err := load(configFilename) - if err != nil { - return err - } - r.config = conf // datastore dspath, err := config.DataStorePath("") @@ -189,6 +183,12 @@ func (r *FSRepo) Close() error { if r.state != opened { return debugerror.Errorf("repo is %s", r.state) } + + for _, closer := range r.components() { + if err := closer.Close(); err != nil { + return err + } + } return transitionToClosed(r) } @@ -209,7 +209,7 @@ func (r *FSRepo) Config() *config.Config { if r.state != opened { panic(fmt.Sprintln("repo is", r.state)) } - return r.config + return r.configComponent.Config() } // SetConfig updates the FSRepo's config. @@ -219,7 +219,7 @@ func (r *FSRepo) SetConfig(updated *config.Config) error { packageLock.Lock() defer packageLock.Unlock() - return r.setConfigUnsynced(updated) + return r.configComponent.SetConfig(updated) } // GetConfigKey retrieves only the value of a particular key. @@ -230,15 +230,7 @@ func (r *FSRepo) GetConfigKey(key string) (interface{}, error) { if r.state != opened { return nil, debugerror.Errorf("repo is %s", r.state) } - filename, err := config.Filename(r.path) - if err != nil { - return nil, err - } - var cfg map[string]interface{} - if err := readConfigFile(filename, &cfg); err != nil { - return nil, err - } - return common.MapGetKV(cfg, key) + return r.configComponent.GetConfigKey(key) } // SetConfigKey writes the value of a particular key. @@ -249,25 +241,7 @@ func (r *FSRepo) SetConfigKey(key string, value interface{}) error { if r.state != opened { return debugerror.Errorf("repo is %s", r.state) } - filename, err := config.Filename(r.path) - if err != nil { - return err - } - var mapconf map[string]interface{} - if err := readConfigFile(filename, &mapconf); err != nil { - return err - } - if err := common.MapSetKV(mapconf, key, value); err != nil { - return err - } - if err := writeConfigFile(filename, mapconf); err != nil { - return err - } - conf, err := config.FromMap(mapconf) - if err != nil { - return err - } - return r.setConfigUnsynced(conf) + return r.configComponent.SetConfigKey(key, value) } var _ io.Closer = &FSRepo{} @@ -279,7 +253,19 @@ func IsInitialized(path string) bool { // Init or Remove the repo while this call is in progress. packageLock.Lock() defer packageLock.Unlock() - return isInitializedUnsynced(path) + + // componentInitCheckers are functions that indicate whether the component + // is isInitialized + var componentInitCheckers = []func(path string) bool{ + configComponentIsInitialized, + // TODO add datastore component initialization checker + } + for _, isInitialized := range componentInitCheckers { + if !isInitialized(path) { + return false + } + } + return true } // private methods below this point. NB: packageLock must held by caller. @@ -287,14 +273,7 @@ func IsInitialized(path string) bool { // isInitializedUnsynced reports whether the repo is initialized. Caller must // hold openerCounter lock. func isInitializedUnsynced(path string) bool { - configFilename, err := config.Filename(path) - if err != nil { - return false - } - if !util.FileExists(configFilename) { - return false - } - return true + return configComponentIsInitialized(path) } // initCheckDir ensures the directory exists and is writable @@ -346,32 +325,10 @@ 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) +// components returns the FSRepo's constituent components +func (r *FSRepo) components() []component { + return []component{ + &r.configComponent, + // TODO add datastore } - 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 } From f37646bf1968ceade88419cd53d2ce97054067f7 Mon Sep 17 00:00:00 2001 From: Brian Tiger Chow Date: Wed, 14 Jan 2015 06:12:55 -0800 Subject: [PATCH 17/29] refactor(fsrepo) add interface to make it easier to understand how to extend the package --- repo/fsrepo/config_component.go | 1 + repo/fsrepo/fsrepo.go | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/repo/fsrepo/config_component.go b/repo/fsrepo/config_component.go index 57781cc4d..4ab77c8c4 100644 --- a/repo/fsrepo/config_component.go +++ b/repo/fsrepo/config_component.go @@ -7,6 +7,7 @@ import ( ) var _ component = &configComponent{} +var _ componentInitializationChecker = configComponentIsInitialized // configComponent abstracts the config component of the FSRepo. // NB: create with makeConfigComponent function. diff --git a/repo/fsrepo/fsrepo.go b/repo/fsrepo/fsrepo.go index caeef670b..c89820410 100644 --- a/repo/fsrepo/fsrepo.go +++ b/repo/fsrepo/fsrepo.go @@ -56,6 +56,7 @@ type component interface { Open() error io.Closer } +type componentInitializationChecker func(path string) bool // At returns a handle to an FSRepo at the provided |path|. func At(repoPath string) *FSRepo { @@ -256,7 +257,7 @@ func IsInitialized(path string) bool { // componentInitCheckers are functions that indicate whether the component // is isInitialized - var componentInitCheckers = []func(path string) bool{ + var componentInitCheckers = []componentInitializationChecker{ configComponentIsInitialized, // TODO add datastore component initialization checker } From 9f67ede6b22cfd2845b50f8f6a8b76c06d4cbb89 Mon Sep 17 00:00:00 2001 From: Brian Tiger Chow Date: Wed, 14 Jan 2015 07:54:18 -0800 Subject: [PATCH 18/29] refactor(fsrepo) extract component.Component --- repo/fsrepo/component/component.go | 14 ++++ .../config.go} | 66 +++++++-------- repo/fsrepo/fsrepo.go | 84 ++++++++++--------- repo/fsrepo/{ => serialize}/serialize.go | 14 ++-- repo/fsrepo/{ => serialize}/serialize_test.go | 4 +- 5 files changed, 101 insertions(+), 81 deletions(-) create mode 100644 repo/fsrepo/component/component.go rename repo/fsrepo/{config_component.go => component/config.go} (57%) rename repo/fsrepo/{ => serialize}/serialize.go (77%) rename repo/fsrepo/{ => serialize}/serialize_test.go (84%) diff --git a/repo/fsrepo/component/component.go b/repo/fsrepo/component/component.go new file mode 100644 index 000000000..5b8c63a1b --- /dev/null +++ b/repo/fsrepo/component/component.go @@ -0,0 +1,14 @@ +package component + +import ( + "io" + + "github.com/jbenet/go-ipfs/repo/config" +) + +type Component interface { + Open() error + io.Closer +} +type Initializer func(path string, conf *config.Config) error +type InitializationChecker func(path string) bool diff --git a/repo/fsrepo/config_component.go b/repo/fsrepo/component/config.go similarity index 57% rename from repo/fsrepo/config_component.go rename to repo/fsrepo/component/config.go index 4ab77c8c4..a21287029 100644 --- a/repo/fsrepo/config_component.go +++ b/repo/fsrepo/component/config.go @@ -1,29 +1,27 @@ -package fsrepo +package component import ( common "github.com/jbenet/go-ipfs/repo/common" config "github.com/jbenet/go-ipfs/repo/config" + serialize "github.com/jbenet/go-ipfs/repo/fsrepo/serialize" util "github.com/jbenet/go-ipfs/util" ) -var _ component = &configComponent{} -var _ componentInitializationChecker = configComponentIsInitialized +var _ Component = &ConfigComponent{} +var _ Initializer = InitConfigComponent +var _ InitializationChecker = ConfigComponentIsInitialized -// configComponent abstracts the config component of the FSRepo. +// ConfigComponent abstracts the config component of the FSRepo. // NB: create with makeConfigComponent function. -type configComponent struct { - path string // required at instantiation +// NOT THREAD-SAFE +type ConfigComponent struct { + Path string // required at instantiation config *config.Config // assigned on Open() } -// makeConfigComponent instantiates a valid configComponent. -func makeConfigComponent(path string) configComponent { - return configComponent{path: path} -} - -// fsrepoConfigInit initializes the FSRepo's configComponent. -func initConfigComponent(path string, conf *config.Config) error { - if configComponentIsInitialized(path) { +// fsrepoConfigInit initializes the FSRepo's ConfigComponent. +func InitConfigComponent(path string, conf *config.Config) error { + if ConfigComponentIsInitialized(path) { return nil } configFilename, err := config.Filename(path) @@ -33,19 +31,19 @@ func initConfigComponent(path string, conf *config.Config) error { // initialization is the one time when it's okay to write to the config // without reading the config from disk and merging any user-provided keys // that may exist. - if err := writeConfigFile(configFilename, conf); err != nil { + if err := serialize.WriteConfigFile(configFilename, conf); err != nil { return err } return nil } // Open returns an error if the config file is not present. -func (c *configComponent) Open() error { - configFilename, err := config.Filename(c.path) +func (c *ConfigComponent) Open() error { + configFilename, err := config.Filename(c.Path) if err != nil { return err } - conf, err := load(configFilename) + conf, err := serialize.Load(configFilename) if err != nil { return err } @@ -54,46 +52,46 @@ func (c *configComponent) Open() error { } // Close satisfies the fsrepoComponent interface. -func (c *configComponent) Close() error { +func (c *ConfigComponent) Close() error { return nil // config doesn't need to be closed. } -func (c *configComponent) Config() *config.Config { +func (c *ConfigComponent) Config() *config.Config { return c.config } // SetConfig updates the config file. -func (c *configComponent) SetConfig(updated *config.Config) error { +func (c *ConfigComponent) SetConfig(updated *config.Config) error { return c.setConfigUnsynced(updated) } // GetConfigKey retrieves only the value of a particular key. -func (c *configComponent) GetConfigKey(key string) (interface{}, error) { - filename, err := config.Filename(c.path) +func (c *ConfigComponent) GetConfigKey(key string) (interface{}, error) { + filename, err := config.Filename(c.Path) if err != nil { return nil, err } var cfg map[string]interface{} - if err := readConfigFile(filename, &cfg); err != nil { + if err := serialize.ReadConfigFile(filename, &cfg); err != nil { return nil, err } return common.MapGetKV(cfg, key) } // SetConfigKey writes the value of a particular key. -func (c *configComponent) SetConfigKey(key string, value interface{}) error { - filename, err := config.Filename(c.path) +func (c *ConfigComponent) SetConfigKey(key string, value interface{}) error { + filename, err := config.Filename(c.Path) if err != nil { return err } var mapconf map[string]interface{} - if err := readConfigFile(filename, &mapconf); err != nil { + if err := serialize.ReadConfigFile(filename, &mapconf); err != nil { return err } if err := common.MapSetKV(mapconf, key, value); err != nil { return err } - if err := writeConfigFile(filename, mapconf); err != nil { + if err := serialize.WriteConfigFile(filename, mapconf); err != nil { return err } // in order to get the updated values, read updated config from the @@ -105,9 +103,9 @@ func (c *configComponent) SetConfigKey(key string, value interface{}) error { return c.setConfigUnsynced(conf) // TODO roll this into this method } -// configComponentIsInitialized returns true if the repo is initialized at +// ConfigComponentIsInitialized returns true if the repo is initialized at // provided |path|. -func configComponentIsInitialized(path string) bool { +func ConfigComponentIsInitialized(path string) bool { configFilename, err := config.Filename(path) if err != nil { return false @@ -119,8 +117,8 @@ func configComponentIsInitialized(path string) bool { } // setConfigUnsynced is for private use. -func (r *configComponent) setConfigUnsynced(updated *config.Config) error { - configFilename, err := config.Filename(r.path) +func (r *ConfigComponent) setConfigUnsynced(updated *config.Config) error { + configFilename, err := config.Filename(r.Path) if err != nil { return err } @@ -128,7 +126,7 @@ func (r *configComponent) setConfigUnsynced(updated *config.Config) error { // 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 { + if err := serialize.ReadConfigFile(configFilename, &mapconf); err != nil { return err } m, err := config.ToMap(updated) @@ -138,7 +136,7 @@ func (r *configComponent) setConfigUnsynced(updated *config.Config) error { for k, v := range m { mapconf[k] = v } - if err := writeConfigFile(configFilename, mapconf); err != nil { + if err := serialize.WriteConfigFile(configFilename, mapconf); err != nil { return err } *r.config = *updated // copy so caller cannot modify this private config diff --git a/repo/fsrepo/fsrepo.go b/repo/fsrepo/fsrepo.go index c89820410..3741fbdaf 100644 --- a/repo/fsrepo/fsrepo.go +++ b/repo/fsrepo/fsrepo.go @@ -11,8 +11,10 @@ import ( repo "github.com/jbenet/go-ipfs/repo" config "github.com/jbenet/go-ipfs/repo/config" + component "github.com/jbenet/go-ipfs/repo/fsrepo/component" lockfile "github.com/jbenet/go-ipfs/repo/fsrepo/lock" opener "github.com/jbenet/go-ipfs/repo/fsrepo/opener" + serialize "github.com/jbenet/go-ipfs/repo/fsrepo/serialize" debugerror "github.com/jbenet/go-ipfs/util/debugerror" ) @@ -49,22 +51,21 @@ type FSRepo struct { // config is loaded when FSRepo is opened and kept up to date when the // FSRepo is modified. // TODO test - configComponent configComponent + configComponent component.ConfigComponent } -type component interface { - Open() error - io.Closer +type componentBuilder struct { + Init component.Initializer + IsInitialized component.InitializationChecker + OpenHandler func(*FSRepo) error } -type componentInitializationChecker func(path string) bool // At returns a handle to an FSRepo at the provided |path|. func At(repoPath string) *FSRepo { // This method must not have side-effects. return &FSRepo{ - path: path.Clean(repoPath), - configComponent: makeConfigComponent(repoPath), - state: unopened, // explicitly set for clarity + path: path.Clean(repoPath), + state: unopened, // explicitly set for clarity } } @@ -78,7 +79,7 @@ func ConfigAt(repoPath string) (*config.Config, error) { if err != nil { return nil, err } - return load(configFilename) + return serialize.Load(configFilename) } // Init initializes a new FSRepo at the given path with the provided config. @@ -93,10 +94,11 @@ func Init(path string, conf *config.Config) error { if isInitializedUnsynced(path) { return nil } - if err := initConfigComponent(path, conf); err != nil { - return err + for _, b := range componentBuilders() { + if err := b.Init(path, conf); err != nil { + return err + } } - return nil } @@ -150,21 +152,12 @@ func (r *FSRepo) Open() error { return err } - for _, opener := range r.components() { - if err := opener.Open(); err != nil { + for _, b := range componentBuilders() { + if err := b.OpenHandler(r); err != nil { return err } } - // datastore - dspath, err := config.DataStorePath("") - if err != nil { - return err - } - if err := initCheckDir(dspath); err != nil { - return debugerror.Errorf("datastore: %s", err) - } - logpath, err := config.LogsPath("") if err != nil { return debugerror.Wrap(err) @@ -255,18 +248,7 @@ func IsInitialized(path string) bool { packageLock.Lock() defer packageLock.Unlock() - // componentInitCheckers are functions that indicate whether the component - // is isInitialized - var componentInitCheckers = []componentInitializationChecker{ - configComponentIsInitialized, - // TODO add datastore component initialization checker - } - for _, isInitialized := range componentInitCheckers { - if !isInitialized(path) { - return false - } - } - return true + return isInitializedUnsynced(path) } // private methods below this point. NB: packageLock must held by caller. @@ -274,7 +256,12 @@ func IsInitialized(path string) bool { // isInitializedUnsynced reports whether the repo is initialized. Caller must // hold openerCounter lock. func isInitializedUnsynced(path string) bool { - return configComponentIsInitialized(path) + for _, b := range componentBuilders() { + if !b.IsInitialized(path) { + return false + } + } + return true } // initCheckDir ensures the directory exists and is writable @@ -327,9 +314,30 @@ func transitionToClosed(r *FSRepo) error { } // components returns the FSRepo's constituent components -func (r *FSRepo) components() []component { - return []component{ +func (r *FSRepo) components() []component.Component { + return []component.Component{ &r.configComponent, // TODO add datastore } } + +func componentBuilders() []componentBuilder { + return []componentBuilder{ + + // ConfigComponent + componentBuilder{ + Init: component.InitConfigComponent, + IsInitialized: component.ConfigComponentIsInitialized, + OpenHandler: func(r *FSRepo) error { + cc := component.ConfigComponent{Path: r.path} + if err := cc.Open(); err != nil { + return err + } + r.configComponent = cc + return nil + }, + }, + + // TODO add datastore builder + } +} diff --git a/repo/fsrepo/serialize.go b/repo/fsrepo/serialize/serialize.go similarity index 77% rename from repo/fsrepo/serialize.go rename to repo/fsrepo/serialize/serialize.go index 559c04804..864eb4fc1 100644 --- a/repo/fsrepo/serialize.go +++ b/repo/fsrepo/serialize/serialize.go @@ -15,8 +15,8 @@ import ( var log = util.Logger("fsrepo") -// readConfigFile reads the config from `filename` into `cfg`. -func readConfigFile(filename string, cfg interface{}) error { +// ReadConfigFile reads the config from `filename` into `cfg`. +func ReadConfigFile(filename string, cfg interface{}) error { f, err := os.Open(filename) if err != nil { return err @@ -28,8 +28,8 @@ func readConfigFile(filename string, cfg interface{}) error { return nil } -// writeConfigFile writes the config from `cfg` into `filename`. -func writeConfigFile(filename string, cfg interface{}) error { +// WriteConfigFile writes the config from `cfg` into `filename`. +func WriteConfigFile(filename string, cfg interface{}) error { err := os.MkdirAll(filepath.Dir(filename), 0775) if err != nil { return err @@ -55,15 +55,15 @@ func encode(w io.Writer, value interface{}) error { return err } -// load reads given file and returns the read config, or error. -func load(filename string) (*config.Config, error) { +// Load reads given file and returns the read config, or error. +func Load(filename string) (*config.Config, error) { // if nothing is there, fail. User must run 'ipfs init' if !util.FileExists(filename) { return nil, debugerror.New("ipfs not initialized, please run 'ipfs init'") } var cfg config.Config - err := readConfigFile(filename, &cfg) + err := ReadConfigFile(filename, &cfg) if err != nil { return nil, err } diff --git a/repo/fsrepo/serialize_test.go b/repo/fsrepo/serialize/serialize_test.go similarity index 84% rename from repo/fsrepo/serialize_test.go rename to repo/fsrepo/serialize/serialize_test.go index 9188b54ec..5de9674af 100644 --- a/repo/fsrepo/serialize_test.go +++ b/repo/fsrepo/serialize/serialize_test.go @@ -11,11 +11,11 @@ func TestConfig(t *testing.T) { const dsPath = "/path/to/datastore" cfgWritten := new(config.Config) cfgWritten.Datastore.Path = dsPath - err := writeConfigFile(filename, cfgWritten) + err := WriteConfigFile(filename, cfgWritten) if err != nil { t.Error(err) } - cfgRead, err := load(filename) + cfgRead, err := Load(filename) if err != nil { t.Error(err) return From 887fbe4a8ea3626b1399fce75f59ff458d255f0b Mon Sep 17 00:00:00 2001 From: Brian Tiger Chow Date: Wed, 14 Jan 2015 07:58:51 -0800 Subject: [PATCH 19/29] style(fsrepo) change func to method to be consistent --- repo/fsrepo/fsrepo.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/repo/fsrepo/fsrepo.go b/repo/fsrepo/fsrepo.go index 3741fbdaf..3e1760525 100644 --- a/repo/fsrepo/fsrepo.go +++ b/repo/fsrepo/fsrepo.go @@ -166,7 +166,7 @@ func (r *FSRepo) Open() error { return debugerror.Errorf("logs: %s", err) } - return transitionToOpened(r) + return r.transitionToOpened() } // Close closes the FSRepo, releasing held resources. @@ -183,7 +183,7 @@ func (r *FSRepo) Close() error { return err } } - return transitionToClosed(r) + return r.transitionToClosed() } // Config returns the FSRepo's config. This method must not be called if the @@ -281,7 +281,7 @@ func initCheckDir(path string) error { // transitionToOpened manages the state transition to |opened|. Caller must hold // the package mutex. -func transitionToOpened(r *FSRepo) error { +func (r *FSRepo) transitionToOpened() error { r.state = opened if countBefore := openerCounter.NumOpeners(r.path); countBefore == 0 { // #first closer, err := lockfile.Lock(r.path) @@ -295,7 +295,7 @@ func transitionToOpened(r *FSRepo) error { // transitionToClosed manages the state transition to |closed|. Caller must // hold the package mutex. -func transitionToClosed(r *FSRepo) error { +func (r *FSRepo) transitionToClosed() error { r.state = closed if err := openerCounter.RemoveOpener(r.path); err != nil { return err From b6603051428df8e1b886005608c57335495a7011 Mon Sep 17 00:00:00 2001 From: Brian Tiger Chow Date: Wed, 14 Jan 2015 08:10:03 -0800 Subject: [PATCH 20/29] extract initCheckDir to dir.Writable --- repo/fsrepo/dir/dir.go | 24 ++++++++++++++++++++++++ repo/fsrepo/fsrepo.go | 21 +++------------------ 2 files changed, 27 insertions(+), 18 deletions(-) create mode 100644 repo/fsrepo/dir/dir.go diff --git a/repo/fsrepo/dir/dir.go b/repo/fsrepo/dir/dir.go new file mode 100644 index 000000000..90554dce1 --- /dev/null +++ b/repo/fsrepo/dir/dir.go @@ -0,0 +1,24 @@ +package dir + +// TODO move somewhere generic + +import ( + "errors" + "os" + "path/filepath" +) + +// Writable ensures the directory exists and is writable +func Writable(path string) error { + // Construct the path if missing + if err := os.MkdirAll(path, os.ModePerm); err != nil { + return err + } + // Check the directory is writeable + if f, err := os.Create(filepath.Join(path, "._check_writeable")); err == nil { + os.Remove(f.Name()) + } else { + return errors.New("'" + path + "' is not writeable") + } + return nil +} diff --git a/repo/fsrepo/fsrepo.go b/repo/fsrepo/fsrepo.go index 3e1760525..dcf9da14a 100644 --- a/repo/fsrepo/fsrepo.go +++ b/repo/fsrepo/fsrepo.go @@ -6,12 +6,12 @@ import ( "io" "os" "path" - "path/filepath" "sync" repo "github.com/jbenet/go-ipfs/repo" config "github.com/jbenet/go-ipfs/repo/config" component "github.com/jbenet/go-ipfs/repo/fsrepo/component" + dir "github.com/jbenet/go-ipfs/repo/fsrepo/dir" lockfile "github.com/jbenet/go-ipfs/repo/fsrepo/lock" opener "github.com/jbenet/go-ipfs/repo/fsrepo/opener" serialize "github.com/jbenet/go-ipfs/repo/fsrepo/serialize" @@ -148,7 +148,7 @@ func (r *FSRepo) Open() error { // check repo path, then check all constituent parts. // TODO acquire repo lock // TODO if err := initCheckDir(logpath); err != nil { // } - if err := initCheckDir(r.path); err != nil { + if err := dir.Writable(r.path); err != nil { return err } @@ -162,7 +162,7 @@ func (r *FSRepo) Open() error { if err != nil { return debugerror.Wrap(err) } - if err := initCheckDir(logpath); err != nil { + if err := dir.Writable(logpath); err != nil { return debugerror.Errorf("logs: %s", err) } @@ -264,21 +264,6 @@ func isInitializedUnsynced(path string) bool { return true } -// initCheckDir ensures the directory exists and is writable -func initCheckDir(path string) error { - // Construct the path if missing - if err := os.MkdirAll(path, os.ModePerm); err != nil { - return err - } - // Check the directory is writeable - if f, err := os.Create(filepath.Join(path, "._check_writeable")); err == nil { - os.Remove(f.Name()) - } else { - return debugerror.New("'" + path + "' is not writeable") - } - return nil -} - // transitionToOpened manages the state transition to |opened|. Caller must hold // the package mutex. func (r *FSRepo) transitionToOpened() error { From 7ad559b8c72ceb560ae26e0835343be9f1c84df4 Mon Sep 17 00:00:00 2001 From: Brian Tiger Chow Date: Wed, 14 Jan 2015 08:26:27 -0800 Subject: [PATCH 21/29] refactor(fsrepo, component): expose SetPath to ensure that components handle paths --- repo/fsrepo/component/component.go | 1 + repo/fsrepo/component/config.go | 14 +++++++++----- repo/fsrepo/fsrepo.go | 3 ++- 3 files changed, 12 insertions(+), 6 deletions(-) diff --git a/repo/fsrepo/component/component.go b/repo/fsrepo/component/component.go index 5b8c63a1b..a8001b7f6 100644 --- a/repo/fsrepo/component/component.go +++ b/repo/fsrepo/component/component.go @@ -9,6 +9,7 @@ import ( type Component interface { Open() error io.Closer + SetPath(string) } type Initializer func(path string, conf *config.Config) error type InitializationChecker func(path string) bool diff --git a/repo/fsrepo/component/config.go b/repo/fsrepo/component/config.go index a21287029..087b20a99 100644 --- a/repo/fsrepo/component/config.go +++ b/repo/fsrepo/component/config.go @@ -15,7 +15,7 @@ var _ InitializationChecker = ConfigComponentIsInitialized // NB: create with makeConfigComponent function. // NOT THREAD-SAFE type ConfigComponent struct { - Path string // required at instantiation + path string // required at instantiation config *config.Config // assigned on Open() } @@ -39,7 +39,7 @@ func InitConfigComponent(path string, conf *config.Config) error { // Open returns an error if the config file is not present. func (c *ConfigComponent) Open() error { - configFilename, err := config.Filename(c.Path) + configFilename, err := config.Filename(c.path) if err != nil { return err } @@ -67,7 +67,7 @@ func (c *ConfigComponent) SetConfig(updated *config.Config) error { // GetConfigKey retrieves only the value of a particular key. func (c *ConfigComponent) GetConfigKey(key string) (interface{}, error) { - filename, err := config.Filename(c.Path) + filename, err := config.Filename(c.path) if err != nil { return nil, err } @@ -80,7 +80,7 @@ func (c *ConfigComponent) GetConfigKey(key string) (interface{}, error) { // SetConfigKey writes the value of a particular key. func (c *ConfigComponent) SetConfigKey(key string, value interface{}) error { - filename, err := config.Filename(c.Path) + filename, err := config.Filename(c.path) if err != nil { return err } @@ -103,6 +103,10 @@ func (c *ConfigComponent) SetConfigKey(key string, value interface{}) error { return c.setConfigUnsynced(conf) // TODO roll this into this method } +func (c *ConfigComponent) SetPath(p string) { + c.path = p +} + // ConfigComponentIsInitialized returns true if the repo is initialized at // provided |path|. func ConfigComponentIsInitialized(path string) bool { @@ -118,7 +122,7 @@ func ConfigComponentIsInitialized(path string) bool { // setConfigUnsynced is for private use. func (r *ConfigComponent) setConfigUnsynced(updated *config.Config) error { - configFilename, err := config.Filename(r.Path) + configFilename, err := config.Filename(r.path) if err != nil { return err } diff --git a/repo/fsrepo/fsrepo.go b/repo/fsrepo/fsrepo.go index dcf9da14a..e95e3ad48 100644 --- a/repo/fsrepo/fsrepo.go +++ b/repo/fsrepo/fsrepo.go @@ -314,7 +314,8 @@ func componentBuilders() []componentBuilder { Init: component.InitConfigComponent, IsInitialized: component.ConfigComponentIsInitialized, OpenHandler: func(r *FSRepo) error { - cc := component.ConfigComponent{Path: r.path} + cc := component.ConfigComponent{} + cc.SetPath(r.path) if err := cc.Open(); err != nil { return err } From 4ba4ee3a0dfbf6506d15103471b3c7ad5f8a4c25 Mon Sep 17 00:00:00 2001 From: Brian Tiger Chow Date: Wed, 14 Jan 2015 08:13:10 -0800 Subject: [PATCH 22/29] feat(fsrepo/component.datastore) basic shell --- repo/fsrepo/component/datastore.go | 64 ++++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+) create mode 100644 repo/fsrepo/component/datastore.go diff --git a/repo/fsrepo/component/datastore.go b/repo/fsrepo/component/datastore.go new file mode 100644 index 000000000..67e6c375b --- /dev/null +++ b/repo/fsrepo/component/datastore.go @@ -0,0 +1,64 @@ +package component + +import ( + datastore "github.com/jbenet/go-ipfs/Godeps/_workspace/src/github.com/jbenet/go-datastore" + levelds "github.com/jbenet/go-ipfs/Godeps/_workspace/src/github.com/jbenet/go-datastore/leveldb" + ldbopts "github.com/jbenet/go-ipfs/Godeps/_workspace/src/github.com/syndtr/goleveldb/leveldb/opt" + config "github.com/jbenet/go-ipfs/repo/config" + dir "github.com/jbenet/go-ipfs/repo/fsrepo/dir" + util "github.com/jbenet/go-ipfs/util" + ds2 "github.com/jbenet/go-ipfs/util/datastore2" + debugerror "github.com/jbenet/go-ipfs/util/debugerror" +) + +var _ Component = &DatastoreComponent{} +var _ Initializer = InitDatastoreComponent +var _ InitializationChecker = DatastoreComponentIsInitialized + +func InitDatastoreComponent(path string, conf *config.Config) error { + // The actual datastore contents are initialized lazily when Opened. + // During Init, we merely check that the directory is writeable. + dspath, err := config.DataStorePath(path) + if err != nil { + return err + } + if err := dir.Writable(dspath); err != nil { + return debugerror.Errorf("datastore: %s", err) + } + return nil +} + +// DatastoreComponentIsInitialized returns true if the datastore dir exists. +func DatastoreComponentIsInitialized(path string) bool { + dspath, err := config.DataStorePath(path) + if err != nil { + return false + } + if !util.FileExists(dspath) { + return false + } + return true +} + +// DatastoreComponent abstracts the datastore component of the FSRepo. +// NB: create with makeDatastoreComponent function. +type DatastoreComponent struct { + path string + ds ds2.ThreadSafeDatastoreCloser +} + +// Open returns an error if the config file is not present. +func (dsc *DatastoreComponent) Open() error { + ds, err := levelds.NewDatastore(dsc.path, &levelds.Options{ + Compression: ldbopts.NoCompression, + }) + if err != nil { + return err + } + dsc.ds = ds + return nil +} + +func (dsc *DatastoreComponent) Close() error { return dsc.ds.Close() } +func (dsc *DatastoreComponent) SetPath(p string) { dsc.path = p } +func (dsc *DatastoreComponent) Datastore() datastore.ThreadSafeDatastore { return dsc.ds } From ece9ed093314516c810de0f3e3c4a74d437eeba6 Mon Sep 17 00:00:00 2001 From: Brian Tiger Chow Date: Wed, 14 Jan 2015 08:37:50 -0800 Subject: [PATCH 23/29] feat(fsrepo): integrate datastore component into FSRepo "for each desired change, make the change easy (warning: this may be hard), then make the easy change" - Kent Beck https://twitter.com/KentBeck/status/250733358307500032 http://martinfowler.com/articles/preparatory-refactoring-example.html cc @jbenet @whyrusleeping --- repo/fsrepo/fsrepo.go | 31 +++++++++++++++++++++++-------- 1 file changed, 23 insertions(+), 8 deletions(-) diff --git a/repo/fsrepo/fsrepo.go b/repo/fsrepo/fsrepo.go index e95e3ad48..a9190eb3d 100644 --- a/repo/fsrepo/fsrepo.go +++ b/repo/fsrepo/fsrepo.go @@ -48,10 +48,12 @@ type FSRepo struct { state state // path is the file-system path path string - // config is loaded when FSRepo is opened and kept up to date when the - // FSRepo is modified. + // configComponent is loaded when FSRepo is opened and kept up to date when + // the FSRepo is modified. // TODO test configComponent component.ConfigComponent + // TODO test + datastoreComponent component.DatastoreComponent } type componentBuilder struct { @@ -302,7 +304,7 @@ func (r *FSRepo) transitionToClosed() error { func (r *FSRepo) components() []component.Component { return []component.Component{ &r.configComponent, - // TODO add datastore + &r.datastoreComponent, } } @@ -314,16 +316,29 @@ func componentBuilders() []componentBuilder { Init: component.InitConfigComponent, IsInitialized: component.ConfigComponentIsInitialized, OpenHandler: func(r *FSRepo) error { - cc := component.ConfigComponent{} - cc.SetPath(r.path) - if err := cc.Open(); err != nil { + c := component.ConfigComponent{} + c.SetPath(r.path) + if err := c.Open(); err != nil { return err } - r.configComponent = cc + r.configComponent = c return nil }, }, - // TODO add datastore builder + // DatastoreComponent + componentBuilder{ + Init: component.InitDatastoreComponent, + IsInitialized: component.DatastoreComponentIsInitialized, + OpenHandler: func(r *FSRepo) error { + c := component.DatastoreComponent{} + c.SetPath(r.path) + if err := c.Open(); err != nil { + return err + } + r.datastoreComponent = c + return nil + }, + }, } } From b685f92c953f202c9bf05df56c23d78038e8c4c9 Mon Sep 17 00:00:00 2001 From: Brian Tiger Chow Date: Wed, 14 Jan 2015 09:05:43 -0800 Subject: [PATCH 24/29] test(fsrepo): InitIdempotence, NilRemoval, ReopeningDisallowed --- repo/fsrepo/fsrepo_test.go | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/repo/fsrepo/fsrepo_test.go b/repo/fsrepo/fsrepo_test.go index caa473607..55b6aee50 100644 --- a/repo/fsrepo/fsrepo_test.go +++ b/repo/fsrepo/fsrepo_test.go @@ -15,6 +15,18 @@ func testRepoPath(p string, t *testing.T) string { return name } +func TestInitIdempotence(t *testing.T) { + path := testRepoPath("", t) + for i := 0; i < 10; i++ { + AssertNil(Init(path, &config.Config{}), t, "multiple calls to init should succeed") + } +} + +func TestRemove(t *testing.T) { + path := testRepoPath("foo", t) + AssertNil(Remove(path), t, "should be able to remove after closed") +} + func TestCannotRemoveIfOpen(t *testing.T) { path := testRepoPath("TestCannotRemoveIfOpen", t) AssertNil(Init(path, &config.Config{}), t, "should initialize successfully") @@ -25,6 +37,18 @@ func TestCannotRemoveIfOpen(t *testing.T) { AssertNil(Remove(path), t, "should be able to remove after closed") } +func TestCannotBeReopened(t *testing.T) { + path := testRepoPath("", t) + AssertNil(Init(path, &config.Config{}), t) + r := At(path) + AssertNil(r.Open(), t) + AssertNil(r.Close(), t) + AssertErr(r.Open(), t, "shouldn't be possible to re-open the repo") + + // mutable state is the enemy. Take Close() as an opportunity to reduce + // entropy. Callers ought to start fresh with a new handle by calling `At`. +} + func TestCanManageReposIndependently(t *testing.T) { pathA := testRepoPath("a", t) pathB := testRepoPath("b", t) From 6396123b7fe813c87328f765c8e06765adfb4cb8 Mon Sep 17 00:00:00 2001 From: Brian Tiger Chow Date: Wed, 14 Jan 2015 09:13:48 -0800 Subject: [PATCH 25/29] feat(fsrepo): expose Datastore in FSRepo interface (+ test) --- repo/fsrepo/fsrepo.go | 13 ++++++++++-- repo/fsrepo/fsrepo_test.go | 42 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 2 deletions(-) diff --git a/repo/fsrepo/fsrepo.go b/repo/fsrepo/fsrepo.go index a9190eb3d..898984e0a 100644 --- a/repo/fsrepo/fsrepo.go +++ b/repo/fsrepo/fsrepo.go @@ -8,6 +8,7 @@ import ( "path" "sync" + ds "github.com/jbenet/go-ipfs/Godeps/_workspace/src/github.com/jbenet/go-datastore" repo "github.com/jbenet/go-ipfs/repo" config "github.com/jbenet/go-ipfs/repo/config" component "github.com/jbenet/go-ipfs/repo/fsrepo/component" @@ -51,8 +52,7 @@ type FSRepo struct { // configComponent is loaded when FSRepo is opened and kept up to date when // the FSRepo is modified. // TODO test - configComponent component.ConfigComponent - // TODO test + configComponent component.ConfigComponent datastoreComponent component.DatastoreComponent } @@ -240,6 +240,15 @@ func (r *FSRepo) SetConfigKey(key string, value interface{}) error { return r.configComponent.SetConfigKey(key, value) } +// Datastore returns a repo-owned datastore. If FSRepo is Closed, return value +// is undefined. +func (r *FSRepo) Datastore() ds.ThreadSafeDatastore { + packageLock.Lock() + d := r.datastoreComponent.Datastore() + packageLock.Unlock() + return d +} + var _ io.Closer = &FSRepo{} var _ repo.Repo = &FSRepo{} diff --git a/repo/fsrepo/fsrepo_test.go b/repo/fsrepo/fsrepo_test.go index 55b6aee50..9bbc7d893 100644 --- a/repo/fsrepo/fsrepo_test.go +++ b/repo/fsrepo/fsrepo_test.go @@ -1,12 +1,15 @@ package fsrepo import ( + "bytes" "io/ioutil" "testing" + datastore "github.com/jbenet/go-ipfs/Godeps/_workspace/src/github.com/jbenet/go-datastore" "github.com/jbenet/go-ipfs/repo/config" ) +// swap arg order func testRepoPath(p string, t *testing.T) string { name, err := ioutil.TempDir("", p) if err != nil { @@ -76,6 +79,45 @@ func TestCanManageReposIndependently(t *testing.T) { AssertNil(Remove(pathA), t) } +func TestDatastoreGetNotAllowedAfterClose(t *testing.T) { + path := testRepoPath("test", t) + + Assert(!IsInitialized(path), t, "should NOT be initialized") + AssertNil(Init(path, &config.Config{}), t, "should initialize successfully") + r := At(path) + AssertNil(r.Open(), t, "should open successfully") + + k := "key" + data := []byte(k) + AssertNil(r.Datastore().Put(datastore.NewKey(k), data), t, "Put should be successful") + + AssertNil(r.Close(), t) + _, err := r.Datastore().Get(datastore.NewKey(k)) + AssertErr(err, t, "after closer, Get should be fail") +} + +func TestDatastorePersistsFromRepoToRepo(t *testing.T) { + path := testRepoPath("test", t) + + AssertNil(Init(path, &config.Config{}), t) + r1 := At(path) + AssertNil(r1.Open(), t) + + k := "key" + expected := []byte(k) + AssertNil(r1.Datastore().Put(datastore.NewKey(k), expected), t, "using first repo, Put should be successful") + AssertNil(r1.Close(), t) + + r2 := At(path) + AssertNil(r2.Open(), t) + v, err := r2.Datastore().Get(datastore.NewKey(k)) + AssertNil(err, t, "using second repo, Get should be successful") + actual, ok := v.([]byte) + Assert(ok, t, "value should be the []byte from r1's Put") + AssertNil(r2.Close(), t) + Assert(bytes.Compare(expected, actual) == 0, t, "data should match") +} + func AssertNil(err error, t *testing.T, msgs ...string) { if err != nil { t.Fatal(msgs, "error:", err) From b666163e523c76018e27226de7e80215cdcf31fb Mon Sep 17 00:00:00 2001 From: Brian Tiger Chow Date: Wed, 14 Jan 2015 09:15:06 -0800 Subject: [PATCH 26/29] test(fsrepo)PERF allow tests to run in parallel --- repo/fsrepo/fsrepo_test.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/repo/fsrepo/fsrepo_test.go b/repo/fsrepo/fsrepo_test.go index 9bbc7d893..fd2374c77 100644 --- a/repo/fsrepo/fsrepo_test.go +++ b/repo/fsrepo/fsrepo_test.go @@ -19,6 +19,7 @@ func testRepoPath(p string, t *testing.T) string { } func TestInitIdempotence(t *testing.T) { + t.Parallel() path := testRepoPath("", t) for i := 0; i < 10; i++ { AssertNil(Init(path, &config.Config{}), t, "multiple calls to init should succeed") @@ -26,11 +27,13 @@ func TestInitIdempotence(t *testing.T) { } func TestRemove(t *testing.T) { + t.Parallel() path := testRepoPath("foo", t) AssertNil(Remove(path), t, "should be able to remove after closed") } func TestCannotRemoveIfOpen(t *testing.T) { + t.Parallel() path := testRepoPath("TestCannotRemoveIfOpen", t) AssertNil(Init(path, &config.Config{}), t, "should initialize successfully") r := At(path) @@ -41,6 +44,7 @@ func TestCannotRemoveIfOpen(t *testing.T) { } func TestCannotBeReopened(t *testing.T) { + t.Parallel() path := testRepoPath("", t) AssertNil(Init(path, &config.Config{}), t) r := At(path) @@ -53,6 +57,7 @@ func TestCannotBeReopened(t *testing.T) { } func TestCanManageReposIndependently(t *testing.T) { + t.Parallel() pathA := testRepoPath("a", t) pathB := testRepoPath("b", t) @@ -80,6 +85,7 @@ func TestCanManageReposIndependently(t *testing.T) { } func TestDatastoreGetNotAllowedAfterClose(t *testing.T) { + t.Parallel() path := testRepoPath("test", t) Assert(!IsInitialized(path), t, "should NOT be initialized") @@ -97,6 +103,7 @@ func TestDatastoreGetNotAllowedAfterClose(t *testing.T) { } func TestDatastorePersistsFromRepoToRepo(t *testing.T) { + t.Parallel() path := testRepoPath("test", t) AssertNil(Init(path, &config.Config{}), t) From 12116dd6e4af4bb06c969f42e9f8e86973ae0d4f Mon Sep 17 00:00:00 2001 From: Brian Tiger Chow Date: Wed, 14 Jan 2015 09:18:57 -0800 Subject: [PATCH 27/29] style(fsrepo): rename to counter.Openers --- .../{opener/counter.go => counter/openers.go} | 14 +++++------ repo/fsrepo/fsrepo.go | 24 +++++++++---------- 2 files changed, 19 insertions(+), 19 deletions(-) rename repo/fsrepo/{opener/counter.go => counter/openers.go} (77%) diff --git a/repo/fsrepo/opener/counter.go b/repo/fsrepo/counter/openers.go similarity index 77% rename from repo/fsrepo/opener/counter.go rename to repo/fsrepo/counter/openers.go index 51cf3bee1..25e58107f 100644 --- a/repo/fsrepo/opener/counter.go +++ b/repo/fsrepo/counter/openers.go @@ -1,17 +1,17 @@ -package fsrepo +package counter import "path" // TODO this could be made into something more generic. -type Counter struct { +type Openers struct { // repos maps repo paths to the number of openers holding an FSRepo handle // to it repos map[string]int } -func NewCounter() *Counter { - return &Counter{ +func NewOpenersCounter() *Openers { + return &Openers{ repos: make(map[string]int), } } @@ -19,13 +19,13 @@ func NewCounter() *Counter { // 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 *Counter) NumOpeners(repoPath string) int { +func (l *Openers) 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 *Counter) AddOpener(repoPath string) error { +func (l *Openers) AddOpener(repoPath string) error { l.repos[key(repoPath)]++ return nil } @@ -33,7 +33,7 @@ func (l *Counter) AddOpener(repoPath string) error { // 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 *Counter) RemoveOpener(repoPath string) error { +func (l *Openers) RemoveOpener(repoPath string) error { l.repos[key(repoPath)]-- return nil } diff --git a/repo/fsrepo/fsrepo.go b/repo/fsrepo/fsrepo.go index 898984e0a..c96dc274b 100644 --- a/repo/fsrepo/fsrepo.go +++ b/repo/fsrepo/fsrepo.go @@ -12,9 +12,9 @@ import ( repo "github.com/jbenet/go-ipfs/repo" config "github.com/jbenet/go-ipfs/repo/config" component "github.com/jbenet/go-ipfs/repo/fsrepo/component" + counter "github.com/jbenet/go-ipfs/repo/fsrepo/counter" dir "github.com/jbenet/go-ipfs/repo/fsrepo/dir" lockfile "github.com/jbenet/go-ipfs/repo/fsrepo/lock" - opener "github.com/jbenet/go-ipfs/repo/fsrepo/opener" serialize "github.com/jbenet/go-ipfs/repo/fsrepo/serialize" debugerror "github.com/jbenet/go-ipfs/util/debugerror" ) @@ -23,22 +23,22 @@ var ( // packageLock must be held to while performing any operation that modifies an // FSRepo's state field. This includes Init, Open, Close, and Remove. - packageLock sync.Mutex // protects openerCounter and lockfiles + packageLock sync.Mutex // protects openersCounter and lockfiles // lockfiles holds references to the Closers that ensure that repos are // only accessed by one process at a time. lockfiles map[string]io.Closer - // openerCounter prevents the fsrepo from being removed while there exist open + // openersCounter 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. - openerCounter *opener.Counter + openersCounter *counter.Openers ) func init() { - openerCounter = opener.NewCounter() + openersCounter = counter.NewOpenersCounter() lockfiles = make(map[string]io.Closer) } @@ -113,7 +113,7 @@ func Remove(repoPath string) error { packageLock.Lock() defer packageLock.Unlock() - if openerCounter.NumOpeners(repoPath) != 0 { + if openersCounter.NumOpeners(repoPath) != 0 { return errors.New("repo in use") } return os.RemoveAll(repoPath) @@ -129,7 +129,7 @@ func LockedByOtherProcess(repoPath string) bool { defer packageLock.Unlock() // NB: the lock is only held when repos are Open - return lockfile.Locked(repoPath) && openerCounter.NumOpeners(repoPath) == 0 + return lockfile.Locked(repoPath) && openersCounter.NumOpeners(repoPath) == 0 } // Open returns an error if the repo is not initialized. @@ -265,7 +265,7 @@ func IsInitialized(path string) bool { // private methods below this point. NB: packageLock must held by caller. // isInitializedUnsynced reports whether the repo is initialized. Caller must -// hold openerCounter lock. +// hold the packageLock. func isInitializedUnsynced(path string) bool { for _, b := range componentBuilders() { if !b.IsInitialized(path) { @@ -279,24 +279,24 @@ func isInitializedUnsynced(path string) bool { // the package mutex. func (r *FSRepo) transitionToOpened() error { r.state = opened - if countBefore := openerCounter.NumOpeners(r.path); countBefore == 0 { // #first + if countBefore := openersCounter.NumOpeners(r.path); countBefore == 0 { // #first closer, err := lockfile.Lock(r.path) if err != nil { return err } lockfiles[r.path] = closer } - return openerCounter.AddOpener(r.path) + return openersCounter.AddOpener(r.path) } // transitionToClosed manages the state transition to |closed|. Caller must // hold the package mutex. func (r *FSRepo) transitionToClosed() error { r.state = closed - if err := openerCounter.RemoveOpener(r.path); err != nil { + if err := openersCounter.RemoveOpener(r.path); err != nil { return err } - if countAfter := openerCounter.NumOpeners(r.path); countAfter == 0 { + if countAfter := openersCounter.NumOpeners(r.path); countAfter == 0 { closer, ok := lockfiles[r.path] if !ok { return errors.New("package error: lockfile is not held") From 30a5aa9b0497ee8cde7d40497292752f421284b3 Mon Sep 17 00:00:00 2001 From: Brian Tiger Chow Date: Wed, 14 Jan 2015 09:26:13 -0800 Subject: [PATCH 28/29] feat(repo): expose the Datastore() in repo.Repo interface --- repo/repo.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/repo/repo.go b/repo/repo.go index dc7689f31..616c1ebc9 100644 --- a/repo/repo.go +++ b/repo/repo.go @@ -1,6 +1,7 @@ package repo import ( + datastore "github.com/jbenet/go-ipfs/Godeps/_workspace/src/github.com/jbenet/go-datastore" config "github.com/jbenet/go-ipfs/repo/config" util "github.com/jbenet/go-ipfs/util" ) @@ -11,6 +12,8 @@ type Repo interface { SetConfigKey(key string, value interface{}) error GetConfigKey(key string) (interface{}, error) + + Datastore() datastore.ThreadSafeDatastore } // IsInitialized returns true if the path is home to an initialized IPFS From bd1d8767eb0810de66ee5b226b937d7c81a29e13 Mon Sep 17 00:00:00 2001 From: Brian Tiger Chow Date: Wed, 14 Jan 2015 10:06:55 -0800 Subject: [PATCH 29/29] doc(fsrepo): explain ConfigAt --- repo/fsrepo/fsrepo.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/repo/fsrepo/fsrepo.go b/repo/fsrepo/fsrepo.go index c96dc274b..59ea07155 100644 --- a/repo/fsrepo/fsrepo.go +++ b/repo/fsrepo/fsrepo.go @@ -71,6 +71,9 @@ func At(repoPath string) *FSRepo { } } +// ConfigAt returns an error if the FSRepo at the given path is not +// initialized. This function allows callers to read the config file even when +// another process is running and holding the lock. func ConfigAt(repoPath string) (*config.Config, error) { // packageLock must be held to ensure that the Read is atomic.