From 7bf8f846c277b67a55e377d7bafbabdf892b0514 Mon Sep 17 00:00:00 2001 From: Stephen J Day Date: Tue, 1 Dec 2015 16:24:07 -0800 Subject: [PATCH 1/3] storage: correctly handle error during Walk Signed-off-by: Stephen J Day --- docs/storage/walk.go | 4 +++- docs/storage/walk_test.go | 12 +++++++++--- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/docs/storage/walk.go b/docs/storage/walk.go index 3d8912765..a27c2b032 100644 --- a/docs/storage/walk.go +++ b/docs/storage/walk.go @@ -38,7 +38,9 @@ func Walk(ctx context.Context, driver storageDriver.StorageDriver, from string, } if fileInfo.IsDir() && !skipDir { - Walk(ctx, driver, child, f) + if err := Walk(ctx, driver, child, f); err != nil { + return err + } } } return nil diff --git a/docs/storage/walk_test.go b/docs/storage/walk_test.go index 40b8547cf..684155b21 100644 --- a/docs/storage/walk_test.go +++ b/docs/storage/walk_test.go @@ -41,10 +41,12 @@ func TestWalkErrors(t *testing.T) { t.Error("Expected invalid root err") } + errEarlyExpected := fmt.Errorf("Early termination") + err = Walk(ctx, d, "/", func(fileInfo driver.FileInfo) error { // error on the 2nd file if fileInfo.Path() == "/a/b" { - return fmt.Errorf("Early termination") + return errEarlyExpected } delete(expected, fileInfo.Path()) return nil @@ -52,8 +54,12 @@ func TestWalkErrors(t *testing.T) { if len(expected) != fileCount-1 { t.Error("Walk failed to terminate with error") } - if err != nil { - t.Error(err.Error()) + if err != errEarlyExpected { + if err == nil { + t.Fatalf("expected an error due to early termination") + } else { + t.Error(err.Error()) + } } err = Walk(ctx, d, "/nonexistant", func(fileInfo driver.FileInfo) error { From 93f92498ce26873fe40ddc71381e1f615cc58ceb Mon Sep 17 00:00:00 2001 From: Stephen J Day Date: Tue, 1 Dec 2015 16:24:31 -0800 Subject: [PATCH 2/3] storage: enforce sorted traversal during Walk Signed-off-by: Stephen J Day --- docs/storage/walk.go | 6 ++++++ docs/storage/walk_test.go | 8 ++++++++ 2 files changed, 14 insertions(+) diff --git a/docs/storage/walk.go b/docs/storage/walk.go index a27c2b032..d979796eb 100644 --- a/docs/storage/walk.go +++ b/docs/storage/walk.go @@ -3,6 +3,7 @@ package storage import ( "errors" "fmt" + "sort" "github.com/docker/distribution/context" storageDriver "github.com/docker/distribution/registry/storage/driver" @@ -26,7 +27,12 @@ func Walk(ctx context.Context, driver storageDriver.StorageDriver, from string, if err != nil { return err } + sort.Stable(sort.StringSlice(children)) for _, child := range children { + // TODO(stevvooe): Calling driver.Stat for every entry is quite + // expensive when running against backends with a slow Stat + // implementation, such as s3. This is very likely a serious + // performance bottleneck. fileInfo, err := driver.Stat(ctx, child) if err != nil { return err diff --git a/docs/storage/walk_test.go b/docs/storage/walk_test.go index 684155b21..5b922e0fb 100644 --- a/docs/storage/walk_test.go +++ b/docs/storage/walk_test.go @@ -2,6 +2,7 @@ package storage import ( "fmt" + "sort" "testing" "github.com/docker/distribution/context" @@ -73,6 +74,7 @@ func TestWalkErrors(t *testing.T) { func TestWalk(t *testing.T) { d, expected, ctx := testFS(t) + var traversed []string err := Walk(ctx, d, "/", func(fileInfo driver.FileInfo) error { filePath := fileInfo.Path() filetype, ok := expected[filePath] @@ -90,11 +92,17 @@ func TestWalk(t *testing.T) { } } delete(expected, filePath) + traversed = append(traversed, filePath) return nil }) if len(expected) > 0 { t.Errorf("Missed files in walk: %q", expected) } + + if !sort.StringsAreSorted(traversed) { + t.Errorf("result should be sorted: %v", traversed) + } + if err != nil { t.Fatalf(err.Error()) } From 6693e9667cd9e06319a5945c64d0fbc48859d49d Mon Sep 17 00:00:00 2001 From: Stephen J Day Date: Tue, 1 Dec 2015 16:55:10 -0800 Subject: [PATCH 3/3] storage: add further tests for Walk implementation Signed-off-by: Stephen J Day --- docs/storage/walk_test.go | 31 ++++++++++++++++++++++++------- 1 file changed, 24 insertions(+), 7 deletions(-) diff --git a/docs/storage/walk_test.go b/docs/storage/walk_test.go index 5b922e0fb..42f67dbaf 100644 --- a/docs/storage/walk_test.go +++ b/docs/storage/walk_test.go @@ -12,14 +12,7 @@ import ( func testFS(t *testing.T) (driver.StorageDriver, map[string]string, context.Context) { d := inmemory.New() - c := []byte("") ctx := context.Background() - if err := d.PutContent(ctx, "/a/b/c/d", c); err != nil { - t.Fatalf("Unable to put to inmemory fs") - } - if err := d.PutContent(ctx, "/a/b/c/e", c); err != nil { - t.Fatalf("Unable to put to inmemory fs") - } expected := map[string]string{ "/a": "dir", @@ -27,6 +20,22 @@ func testFS(t *testing.T) (driver.StorageDriver, map[string]string, context.Cont "/a/b/c": "dir", "/a/b/c/d": "file", "/a/b/c/e": "file", + "/a/b/f": "dir", + "/a/b/f/g": "file", + "/a/b/f/h": "file", + "/a/b/f/i": "file", + "/z": "dir", + "/z/y": "file", + } + + for p, typ := range expected { + if typ != "file" { + continue + } + + if err := d.PutContent(ctx, p, []byte(p)); err != nil { + t.Fatalf("unable to put content into fixture: %v", err) + } } return d, expected, ctx @@ -49,6 +58,7 @@ func TestWalkErrors(t *testing.T) { if fileInfo.Path() == "/a/b" { return errEarlyExpected } + delete(expected, fileInfo.Path()) return nil }) @@ -90,6 +100,13 @@ func TestWalk(t *testing.T) { if filetype != "file" { t.Errorf("Unexpected file type: %q", filePath) } + + // each file has its own path as the contents. If the length + // doesn't match the path length, fail. + if fileInfo.Size() != int64(len(fileInfo.Path())) { + t.Fatalf("unexpected size for %q: %v != %v", + fileInfo.Path(), fileInfo.Size(), len(fileInfo.Path())) + } } delete(expected, filePath) traversed = append(traversed, filePath)