From da7e8f7c1c77adad50795fa73ed40363da2dd27f Mon Sep 17 00:00:00 2001 From: Damian Debkowski Date: Thu, 27 Jul 2023 09:25:01 -0700 Subject: [PATCH] fix(bsr): allow reading twice from the same fstest.MemFile (#3498) --- internal/bsr/internal/fstest/fs.go | 54 +++++++++++++---- internal/bsr/internal/fstest/fs_test.go | 81 +++++++++++++++++++++++-- 2 files changed, 118 insertions(+), 17 deletions(-) diff --git a/internal/bsr/internal/fstest/fs.go b/internal/bsr/internal/fstest/fs.go index 31b2031136..ef702300dd 100644 --- a/internal/bsr/internal/fstest/fs.go +++ b/internal/bsr/internal/fstest/fs.go @@ -124,7 +124,12 @@ func (m *MemContainer) Create(ctx context.Context, n string) (storage.File, erro } // OpenFile creates a storage.File in the container using the provided options -// It supports WithCloseSyncMode. +// It supports WithCloseSyncMode, WithFileAccessMode, WithCreateFile. +// +// When opening a file with the WithCreateFile option, any existing file will be truncated. +// When opening an existing file with ReadOnly option, a copy of the file is returned to allow concurrent reads of the same file. +// Note, ReadOnly files will only contain the snapshot of a file from when it was opened, any mutations to the file after it was +// opened will not be present in the Read call. func (m *MemContainer) OpenFile(_ context.Context, n string, option ...storage.Option) (storage.File, error) { m.Lock() defer m.Unlock() @@ -142,28 +147,48 @@ func (m *MemContainer) OpenFile(_ context.Context, n string, option ...storage.O return nil, fmt.Errorf("cannot create file in read-only mode: %w", ErrReadOnly) } - var f *MemFile + // src is a MemFile reference that is stored in container's Files map, this is the source of truth for a file. + // dst is a MemFile reference that is stored in container's Files map when creating a file. + // when reading from an existing file stored in the container's Files map, dst becomes a deep copy of the + // MemFile referencing the src of truth for the file, thus allowing for multiple reads of the same file. + var src, dst *MemFile if opts.WithCreateFile { // create or truncate just like os.Create - f = &MemFile{ + src = &MemFile{ Buf: bytes.NewBuffer([]byte{}), + src: []byte{}, } + dst = src } else { var ok bool - f, ok = m.Files[n] + src, ok = m.Files[n] if !ok { return nil, fmt.Errorf("file %s does not exist: %w", n, ErrDoesNotExist) } } - f.name = n - f.syncMode = opts.WithCloseSyncMode - f.accessMode = opts.WithFileAccessMode - f.mode = defaultFilePerm - f.Closed = false - m.Files[n] = f - return f, nil + src.Lock() + defer src.Unlock() + + src.name = n + src.syncMode = opts.WithCloseSyncMode + src.accessMode = opts.WithFileAccessMode + src.mode = defaultFilePerm + src.Closed = false + m.Files[n] = src + if dst == nil { + dst = &MemFile{ + Buf: bytes.NewBuffer(append([]byte{}, src.src...)), + src: src.src, + name: src.name, + syncMode: src.syncMode, + accessMode: src.accessMode, + mode: src.mode, + Closed: src.Closed, + } + } + return dst, nil } // SubContainer creates a new storage.Container in the container. @@ -220,6 +245,7 @@ func (m *memFileInfo) Sys() any { return nil } type MemFile struct { name string Buf *bytes.Buffer + src []byte mode sfs.FileMode modtime time.Time @@ -245,6 +271,7 @@ func NewMemFile(n string, mode sfs.FileMode, options ...Option) *MemFile { return &MemFile{ name: n, Buf: bytes.NewBuffer([]byte{}), + src: []byte{}, mode: mode, accessMode: storageOpts.WithFileAccessMode, syncMode: storageOpts.WithCloseSyncMode, @@ -261,6 +288,7 @@ func NewWritableMemFile(n string, options ...Option) *MemFile { return &MemFile{ name: n, Buf: bytes.NewBuffer([]byte{}), + src: []byte{}, mode: 0o664, accessMode: storage.WriteOnly, syncMode: storage.Asynchronous, @@ -283,7 +311,7 @@ func (m *MemFile) Stat() (sfs.FileInfo, error) { } return &memFileInfo{ name: m.name, - size: int64(m.Buf.Len()), + size: int64(len(m.src)), mode: m.mode, mod: m.modtime, }, nil @@ -300,6 +328,7 @@ func (m *MemFile) Read(p []byte) (int, error) { if m.Closed { return 0, fmt.Errorf("read on closed file") } + return m.Buf.Read(p) } @@ -342,6 +371,7 @@ func (m *MemFile) Write(p []byte) (n int, err error) { defer func() { m.modtime = time.Now() }() + m.src = append(m.src, p...) return m.Buf.Write(p) } diff --git a/internal/bsr/internal/fstest/fs_test.go b/internal/bsr/internal/fstest/fs_test.go index 9713f2f181..c667c42e32 100644 --- a/internal/bsr/internal/fstest/fs_test.go +++ b/internal/bsr/internal/fstest/fs_test.go @@ -222,11 +222,12 @@ func TestContainerOpenFile(t *testing.T) { } cases := []struct { - name string - setupFn func(t *testing.T, f storage.FS) storage.Container - n string - opts []storage.Option - wantErr error + name string + setupFn func(t *testing.T, f storage.FS) storage.Container + n string + opts []storage.Option + wantErr error + wantData string }{ { "create", @@ -238,6 +239,7 @@ func TestContainerOpenFile(t *testing.T) { "test", []storage.Option{storage.WithCreateFile(), storage.WithFileAccessMode(storage.ReadWrite)}, nil, + "", }, { "create-already-exists", @@ -251,6 +253,7 @@ func TestContainerOpenFile(t *testing.T) { "test", []storage.Option{storage.WithCreateFile(), storage.WithFileAccessMode(storage.ReadWrite)}, nil, + "", }, { "create-read-only", @@ -262,6 +265,7 @@ func TestContainerOpenFile(t *testing.T) { "test", []storage.Option{storage.WithCreateFile(), storage.WithFileAccessMode(storage.ReadOnly)}, fstest.ErrReadOnly, + "", }, { "read-only-does-not-exist", @@ -273,6 +277,7 @@ func TestContainerOpenFile(t *testing.T) { "test", []storage.Option{storage.WithFileAccessMode(storage.ReadOnly)}, fstest.ErrDoesNotExist, + "", }, { "read-only-exist", @@ -286,6 +291,30 @@ func TestContainerOpenFile(t *testing.T) { "test", []storage.Option{storage.WithFileAccessMode(storage.ReadOnly)}, nil, + "", + }, + { + "open-twice", + func(t *testing.T, f storage.FS) storage.Container { + c, err := f.New(ctx, "test") + require.NoError(t, err) + w, err := c.OpenFile(ctx, "test", storage.WithCreateFile(), storage.WithFileAccessMode(storage.ReadWrite)) + require.NoError(t, err) + n, err := w.WriteString("hello world") + require.NoError(t, err) + require.Equal(t, len("hello world"), n) + r, err := c.OpenFile(ctx, "test", storage.WithFileAccessMode(storage.ReadOnly)) + require.NoError(t, err) + data := make([]byte, len("hello world")) + n, err = r.Read(data) + require.NoError(t, err) + require.Equal(t, len("hello world"), n) + return c + }, + "test", + []storage.Option{storage.WithFileAccessMode(storage.ReadOnly)}, + nil, + "hello world", }, } for _, tfs := range fsCases { @@ -301,6 +330,13 @@ func TestContainerOpenFile(t *testing.T) { } require.NoError(t, err) require.NotNil(t, f) + if tc.wantData != "" { + data := make([]byte, len(tc.wantData)) + n, err := f.Read(data) + require.NoError(t, err) + require.Equal(t, len(tc.wantData), n) + require.Equal(t, tc.wantData, string(data)) + } }) } }) @@ -496,6 +532,41 @@ func TestMemContainerSubContainer(t *testing.T) { } } +func TestOpeningFileTwice(t *testing.T) { + ctx := context.Background() + f := fstest.NewMemFS() + c, err := f.New(ctx, "test") + require.NoError(t, err) + + expectedData := "hello world" + + // Create File + w, err := c.OpenFile(ctx, "test", storage.WithCreateFile(), storage.WithFileAccessMode(storage.ReadWrite)) + require.NoError(t, err) + n, err := w.WriteString(expectedData) + require.NoError(t, err) + require.Equal(t, len(expectedData), n) + + r1, err := c.OpenFile(ctx, "test", storage.WithFileAccessMode(storage.ReadOnly)) + require.NoError(t, err) + r2, err := c.OpenFile(ctx, "test", storage.WithFileAccessMode(storage.ReadOnly)) + require.NoError(t, err) + + // reader 1 works independently from reader 2 + actualData := make([]byte, len(expectedData)) + n, err = r1.Read(actualData) + require.NoError(t, err) + require.Equal(t, len(expectedData), n) + require.Equal(t, expectedData, string(actualData)) + + // reader 2 works independently from reader 1 + actualData = make([]byte, len(expectedData)) + n, err = r2.Read(actualData) + require.NoError(t, err) + require.Equal(t, len(expectedData), n) + require.Equal(t, expectedData, string(actualData)) +} + func TestOutOfSpace(t *testing.T) { ctx := context.Background()