From 52230b8f0738c11444d70232905d1c1d32f7cb5e Mon Sep 17 00:00:00 2001 From: Alexander Neumann Date: Sat, 20 Jan 2018 13:43:07 +0100 Subject: [PATCH] backend: Rework List() For a discussion see #1567 --- internal/backend/local/layout_test.go | 9 ++- internal/backend/local/local.go | 81 ++++++++++++++------------- internal/backend/mem/mem_backend.go | 37 ++++++------ internal/backend/test/tests.go | 19 +++++-- internal/restic/backend.go | 15 ++--- internal/restic/backend_find.go | 31 +++++++--- internal/restic/backend_find_test.go | 25 ++++----- internal/restic/repository.go | 2 +- 8 files changed, 124 insertions(+), 95 deletions(-) diff --git a/internal/backend/local/layout_test.go b/internal/backend/local/layout_test.go index 05eba96f0..f2531a332 100644 --- a/internal/backend/local/layout_test.go +++ b/internal/backend/local/layout_test.go @@ -49,8 +49,13 @@ func TestLayout(t *testing.T) { } datafiles := make(map[string]bool) - for id := range be.List(context.TODO(), restic.DataFile) { - datafiles[id] = false + err = be.List(context.TODO(), restic.DataFile, func(fi restic.FileInfo) error { + datafiles[fi.Name] = false + return nil + }) + + if err != nil { + t.Fatalf("List() returned error %v", err) } if len(datafiles) == 0 { diff --git a/internal/backend/local/local.go b/internal/backend/local/local.go index b7afd96c1..02b3be4b2 100644 --- a/internal/backend/local/local.go +++ b/internal/backend/local/local.go @@ -228,50 +228,53 @@ func isFile(fi os.FileInfo) bool { // List returns a channel that yields all names of blobs of type t. A // goroutine is started for this. -func (b *Local) List(ctx context.Context, t restic.FileType) <-chan string { +func (b *Local) List(ctx context.Context, t restic.FileType, fn func(restic.FileInfo) error) error { debug.Log("List %v", t) - ch := make(chan string) - - go func() { - defer close(ch) - - basedir, subdirs := b.Basedir(t) - err := fs.Walk(basedir, func(path string, fi os.FileInfo, err error) error { - debug.Log("walk on %v\n", path) - if err != nil { - return err - } - - if path == basedir { - return nil - } - - if !isFile(fi) { - return nil - } - - if fi.IsDir() && !subdirs { - return filepath.SkipDir - } - - debug.Log("send %v\n", filepath.Base(path)) - - select { - case ch <- filepath.Base(path): - case <-ctx.Done(): - return nil - } - - return nil - }) - + basedir, subdirs := b.Basedir(t) + err := fs.Walk(basedir, func(path string, fi os.FileInfo, err error) error { + debug.Log("walk on %v\n", path) if err != nil { - debug.Log("Walk %v", err) + return err } - }() - return ch + if path == basedir { + return nil + } + + if !isFile(fi) { + return nil + } + + if fi.IsDir() && !subdirs { + return filepath.SkipDir + } + + debug.Log("send %v\n", filepath.Base(path)) + + rfi := restic.FileInfo{ + Name: filepath.Base(path), + Size: fi.Size(), + } + + err = fn(rfi) + if err != nil { + return err + } + + if ctx.Err() != nil { + return ctx.Err() + } + + return nil + }) + + if err != nil { + debug.Log("Walk %v", err) + return err + } + + return ctx.Err() } // Delete removes the repository and all files. diff --git a/internal/backend/mem/mem_backend.go b/internal/backend/mem/mem_backend.go index 9e7de51d5..68041ac55 100644 --- a/internal/backend/mem/mem_backend.go +++ b/internal/backend/mem/mem_backend.go @@ -163,34 +163,31 @@ func (be *MemoryBackend) Remove(ctx context.Context, h restic.Handle) error { } // List returns a channel which yields entries from the backend. -func (be *MemoryBackend) List(ctx context.Context, t restic.FileType) <-chan string { +func (be *MemoryBackend) List(ctx context.Context, t restic.FileType, fn func(restic.FileInfo) error) error { be.m.Lock() defer be.m.Unlock() - ch := make(chan string) - - var ids []string - for entry := range be.data { + for entry, buf := range be.data { if entry.Type != t { continue } - ids = append(ids, entry.Name) + + fi := restic.FileInfo{ + Name: entry.Name, + Size: int64(len(buf)), + } + + err := fn(fi) + if err != nil { + return err + } + + if ctx.Err() != nil { + return ctx.Err() + } } - debug.Log("list %v: %v", t, ids) - - go func() { - defer close(ch) - for _, id := range ids { - select { - case ch <- id: - case <-ctx.Done(): - return - } - } - }() - - return ch + return ctx.Err() } // Location returns the location of the backend (RAM). diff --git a/internal/backend/test/tests.go b/internal/backend/test/tests.go index 9e88a12ba..9f60dc1da 100644 --- a/internal/backend/test/tests.go +++ b/internal/backend/test/tests.go @@ -283,12 +283,17 @@ func (s *Suite) TestList(t *testing.T) { s.SetListMaxItems(test.maxItems) } - for name := range b.List(context.TODO(), restic.DataFile) { - id, err := restic.ParseID(name) + err := b.List(context.TODO(), restic.DataFile, func(fi restic.FileInfo) error { + id, err := restic.ParseID(fi.Name) if err != nil { t.Fatal(err) } list2.Insert(id) + return nil + }) + + if err != nil { + t.Fatalf("List returned error %v", err) } t.Logf("loaded %v IDs from backend", len(list2)) @@ -556,10 +561,16 @@ func delayedList(t testing.TB, b restic.Backend, tpe restic.FileType, max int, m list := restic.NewIDSet() start := time.Now() for i := 0; i < max; i++ { - for s := range b.List(context.TODO(), tpe) { - id := restic.TestParseID(s) + err := b.List(context.TODO(), tpe, func(fi restic.FileInfo) error { + id := restic.TestParseID(fi.Name) list.Insert(id) + return nil + }) + + if err != nil { + t.Fatal(err) } + if len(list) < max && time.Since(start) < maxwait { time.Sleep(500 * time.Millisecond) } diff --git a/internal/restic/backend.go b/internal/restic/backend.go index 9174b5522..07d0c1739 100644 --- a/internal/restic/backend.go +++ b/internal/restic/backend.go @@ -32,10 +32,9 @@ type Backend interface { // Stat returns information about the File identified by h. Stat(ctx context.Context, h Handle) (FileInfo, error) - // List returns a channel that yields all names of files of type t in an - // arbitrary order. A goroutine is started for this, which is stopped when - // ctx is cancelled. - List(ctx context.Context, t FileType) <-chan string + // List runs fn for each file in the backend which has the type t. When an + // error occurs (or fn returns an error), List stops and returns it. + List(ctx context.Context, t FileType, fn func(FileInfo) error) error // IsNotExist returns true if the error was caused by a non-existing file // in the backend. @@ -45,6 +44,8 @@ type Backend interface { Delete(ctx context.Context) error } -// FileInfo is returned by Stat() and contains information about a file in the -// backend. -type FileInfo struct{ Size int64 } +// FileInfo is contains information about a file in the backend. +type FileInfo struct { + Size int64 + Name string +} diff --git a/internal/restic/backend_find.go b/internal/restic/backend_find.go index 02521a25d..722a42dd2 100644 --- a/internal/restic/backend_find.go +++ b/internal/restic/backend_find.go @@ -20,15 +20,23 @@ var ErrMultipleIDMatches = errors.New("multiple IDs with prefix found") func Find(be Lister, t FileType, prefix string) (string, error) { match := "" - // TODO: optimize by sorting list etc. - for name := range be.List(context.TODO(), t) { - if prefix == name[:len(prefix)] { + ctx, cancel := context.WithCancel(context.TODO()) + defer cancel() + + err := be.List(ctx, t, func(fi FileInfo) error { + if prefix == fi.Name[:len(prefix)] { if match == "" { - match = name + match = fi.Name } else { - return "", ErrMultipleIDMatches + return ErrMultipleIDMatches } } + + return nil + }) + + if err != nil { + return "", err } if match != "" { @@ -45,8 +53,17 @@ const minPrefixLength = 8 func PrefixLength(be Lister, t FileType) (int, error) { // load all IDs of the given type list := make([]string, 0, 100) - for name := range be.List(context.TODO(), t) { - list = append(list, name) + + ctx, cancel := context.WithCancel(context.TODO()) + defer cancel() + + err := be.List(ctx, t, func(fi FileInfo) error { + list = append(list, fi.Name) + return nil + }) + + if err != nil { + return 0, err } // select prefixes of length l, test if the last one is the same as the current one diff --git a/internal/restic/backend_find_test.go b/internal/restic/backend_find_test.go index 032c8a9d9..2cec35b1f 100644 --- a/internal/restic/backend_find_test.go +++ b/internal/restic/backend_find_test.go @@ -6,11 +6,11 @@ import ( ) type mockBackend struct { - list func(context.Context, FileType) <-chan string + list func(context.Context, FileType, func(FileInfo) error) error } -func (m mockBackend) List(ctx context.Context, t FileType) <-chan string { - return m.list(ctx, t) +func (m mockBackend) List(ctx context.Context, t FileType, fn func(FileInfo) error) error { + return m.list(ctx, t, fn) } var samples = IDs{ @@ -28,19 +28,14 @@ func TestPrefixLength(t *testing.T) { list := samples m := mockBackend{} - m.list = func(ctx context.Context, t FileType) <-chan string { - ch := make(chan string) - go func() { - defer close(ch) - for _, id := range list { - select { - case ch <- id.String(): - case <-ctx.Done(): - return - } + m.list = func(ctx context.Context, t FileType, fn func(FileInfo) error) error { + for _, id := range list { + err := fn(FileInfo{Name: id.String()}) + if err != nil { + return err } - }() - return ch + } + return nil } l, err := PrefixLength(m, SnapshotFile) diff --git a/internal/restic/repository.go b/internal/restic/repository.go index 2daae41b2..defd6174f 100644 --- a/internal/restic/repository.go +++ b/internal/restic/repository.go @@ -46,7 +46,7 @@ type Repository interface { // Lister allows listing files in a backend. type Lister interface { - List(context.Context, FileType) <-chan string + List(context.Context, FileType, func(FileInfo) error) error } // Index keeps track of the blobs are stored within files.