fix(bsr): allow reading twice from the same fstest.MemFile (#3498)

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)
 }
 

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()