diff options
author | Kamil Trzciński <ayufan@ayufan.eu> | 2020-10-07 18:58:59 +0300 |
---|---|---|
committer | Kamil Trzciński <ayufan@ayufan.eu> | 2020-10-13 00:13:32 +0300 |
commit | a4ccdd055c0b532a9aed9887cdc016c363eab15d (patch) | |
tree | 408f7b7e3e6b9c8c6a08725e63ecc116e00bc1c9 /internal | |
parent | e7a571d304492cbca9f54d10f08374100f4cd66a (diff) |
Improve LRU cache tests and implementation
Diffstat (limited to 'internal')
-rw-r--r-- | internal/vfs/zip/archive.go | 5 | ||||
-rw-r--r-- | internal/vfs/zip/archive_test.go | 83 | ||||
-rw-r--r-- | internal/vfs/zip/lru_cache.go | 1 | ||||
-rw-r--r-- | internal/vfs/zip/vfs_test.go | 4 |
4 files changed, 75 insertions, 18 deletions
diff --git a/internal/vfs/zip/archive.go b/internal/vfs/zip/archive.go index 630d5fd5..6675e199 100644 --- a/internal/vfs/zip/archive.go +++ b/internal/vfs/zip/archive.go @@ -34,6 +34,7 @@ const ( var ( errNotSymlink = errors.New("not a symlink") errSymlinkSize = errors.New("symlink too long") + errNotFile = errors.New("not a file") ) // zipArchive implements the vfs.Root interface. @@ -167,6 +168,10 @@ func (a *zipArchive) Open(ctx context.Context, name string) (vfs.File, error) { return nil, os.ErrNotExist } + if !file.Mode().IsRegular() { + return nil, errNotFile + } + dataOffset, err := a.fs.dataOffsetCache.findOrFetch(a.namespace, name, func() (interface{}, error) { return file.DataOffset() }) diff --git a/internal/vfs/zip/archive_test.go b/internal/vfs/zip/archive_test.go index d778eefb..bd7627b1 100644 --- a/internal/vfs/zip/archive_test.go +++ b/internal/vfs/zip/archive_test.go @@ -6,6 +6,7 @@ import ( "net/http" "net/http/httptest" "os" + "sync/atomic" "testing" "time" @@ -17,7 +18,7 @@ import ( var chdirSet = false func TestOpen(t *testing.T) { - zip, cleanup := openZipArchive(t) + zip, cleanup := openZipArchive(t, nil) defer cleanup() tests := map[string]struct { @@ -38,11 +39,11 @@ func TestOpen(t *testing.T) { "file_exists_symlink": { file: "symlink.html", expectedContent: "subdir/linked.html", - expectedErr: nil, + expectedErr: errNotFile, }, "is_dir": { file: "subdir", - expectedErr: nil, + expectedErr: errNotFile, }, "file_does_not_exist": { file: "unknown.html", @@ -59,12 +60,6 @@ func TestOpen(t *testing.T) { } require.NoError(t, err) - - if tt.expectedContent == "" { - // cannot ioutil.ReadAll dirs but zip.Open should not fail - return - } - data, err := ioutil.ReadAll(f) require.NoError(t, err) @@ -74,8 +69,36 @@ func TestOpen(t *testing.T) { } } +func TestOpenCached(t *testing.T) { + var requests int64 + zip, cleanup := openZipArchive(t, &requests) + defer cleanup() + + t.Run("open file first time", func(t *testing.T) { + requestsStart := requests + f, err := zip.Open(context.Background(), "index.html") + require.NoError(t, err) + defer f.Close() + + _, err = ioutil.ReadAll(f) + require.NoError(t, err) + require.Equal(t, int64(2), atomic.LoadInt64(&requests)-requestsStart, "we expect two requests to read file: data offset and content") + }) + + t.Run("open file second time", func(t *testing.T) { + requestsStart := atomic.LoadInt64(&requests) + f, err := zip.Open(context.Background(), "index.html") + require.NoError(t, err) + defer f.Close() + + _, err = ioutil.ReadAll(f) + require.NoError(t, err) + require.Equal(t, int64(1), atomic.LoadInt64(&requests)-requestsStart, "we expect one request to read file with cached data offset") + }) +} + func TestLstat(t *testing.T) { - zip, cleanup := openZipArchive(t) + zip, cleanup := openZipArchive(t, nil) defer cleanup() tests := map[string]struct { @@ -135,7 +158,7 @@ func TestLstat(t *testing.T) { } func TestReadLink(t *testing.T) { - zip, cleanup := openZipArchive(t) + zip, cleanup := openZipArchive(t, nil) defer cleanup() tests := map[string]struct { @@ -177,8 +200,28 @@ func TestReadLink(t *testing.T) { } } +func TestReadlinkCached(t *testing.T) { + var requests int64 + zip, cleanup := openZipArchive(t, &requests) + defer cleanup() + + t.Run("readlink first time", func(t *testing.T) { + requestsStart := atomic.LoadInt64(&requests) + _, err := zip.Readlink(context.Background(), "symlink.html") + require.NoError(t, err) + require.Equal(t, int64(2), atomic.LoadInt64(&requests)-requestsStart, "we expect two requests to read symlink: data offset and link") + }) + + t.Run("readlink second time", func(t *testing.T) { + requestsStart := atomic.LoadInt64(&requests) + _, err := zip.Readlink(context.Background(), "symlink.html") + require.NoError(t, err) + require.Equal(t, int64(0), atomic.LoadInt64(&requests)-requestsStart, "we expect no additional requests to read cached symlink") + }) +} + func TestArchiveCanBeReadAfterOpenCtxCanceled(t *testing.T) { - testServerURL, cleanup := newZipFileServerURL(t, "group/zip.gitlab.io/public.zip") + testServerURL, cleanup := newZipFileServerURL(t, "group/zip.gitlab.io/public.zip", nil) defer cleanup() fs := New().(*zipVFS) @@ -201,7 +244,7 @@ func TestArchiveCanBeReadAfterOpenCtxCanceled(t *testing.T) { } func TestReadArchiveFails(t *testing.T) { - testServerURL, cleanup := newZipFileServerURL(t, "group/zip.gitlab.io/public.zip") + testServerURL, cleanup := newZipFileServerURL(t, "group/zip.gitlab.io/public.zip", nil) defer cleanup() fs := New().(*zipVFS) @@ -215,10 +258,14 @@ func TestReadArchiveFails(t *testing.T) { require.EqualError(t, err, os.ErrNotExist.Error()) } -func openZipArchive(t *testing.T) (*zipArchive, func()) { +func openZipArchive(t *testing.T, requests *int64) (*zipArchive, func()) { t.Helper() - testServerURL, cleanup := newZipFileServerURL(t, "group/zip.gitlab.io/public.zip") + if requests == nil { + requests = new(int64) + } + + testServerURL, cleanup := newZipFileServerURL(t, "group/zip.gitlab.io/public.zip", requests) fs := New().(*zipVFS) zip := newArchive(fs, testServerURL+"/public.zip", time.Second) @@ -230,13 +277,14 @@ func openZipArchive(t *testing.T) (*zipArchive, func()) { // public/subdir/ public/subdir/hello.html public/subdir/linked.html // public/bad_symlink.html public/subdir/2bp3Qzs... require.NotZero(t, zip.files) + require.Equal(t, int64(3), atomic.LoadInt64(requests), "we expect three requests to open ZIP archive: size and two to seek central directory") return zip, func() { cleanup() } } -func newZipFileServerURL(t *testing.T, zipFilePath string) (string, func()) { +func newZipFileServerURL(t *testing.T, zipFilePath string, requests *int64) (string, func()) { t.Helper() chdir := testhelpers.ChdirInPath(t, "../../../shared/pages", &chdirSet) @@ -244,6 +292,9 @@ func newZipFileServerURL(t *testing.T, zipFilePath string) (string, func()) { m := http.NewServeMux() m.HandleFunc("/public.zip", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { http.ServeFile(w, r, zipFilePath) + if requests != nil { + atomic.AddInt64(requests, 1) + } })) testServer := httptest.NewServer(m) diff --git a/internal/vfs/zip/lru_cache.go b/internal/vfs/zip/lru_cache.go index d33c81e2..29aa26e0 100644 --- a/internal/vfs/zip/lru_cache.go +++ b/internal/vfs/zip/lru_cache.go @@ -4,6 +4,7 @@ import ( "time" "github.com/karlseguin/ccache/v2" + "gitlab.com/gitlab-org/gitlab-pages/metrics" ) diff --git a/internal/vfs/zip/vfs_test.go b/internal/vfs/zip/vfs_test.go index 62b5f62c..434fc84c 100644 --- a/internal/vfs/zip/vfs_test.go +++ b/internal/vfs/zip/vfs_test.go @@ -10,7 +10,7 @@ import ( ) func TestVFSRoot(t *testing.T) { - url, cleanup := newZipFileServerURL(t, "group/zip.gitlab.io/public.zip") + url, cleanup := newZipFileServerURL(t, "group/zip.gitlab.io/public.zip", nil) defer cleanup() tests := map[string]struct { @@ -63,7 +63,7 @@ func TestVFSRoot(t *testing.T) { } func TestVFSFindOrOpenArchiveConcurrentAccess(t *testing.T) { - testServerURL, cleanup := newZipFileServerURL(t, "group/zip.gitlab.io/public.zip") + testServerURL, cleanup := newZipFileServerURL(t, "group/zip.gitlab.io/public.zip", nil) defer cleanup() path := testServerURL + "/public.zip" |