Plugins: Fix module.js file not being closed when loading plugins (#66288)

* Plugins: Loader: Fix module.js file not being closed

* Plugins: LocalFS: Add comments, ensure same Read() behaviour as os.File's

* Changed comment for Close()

* Add tests for LocalFS

* "Fix" linter error

* "Fix" linter error again

pkg/plugins/localfiles.go

@@ -12,11 +12,18 @@ import (
 
 var _ fs.FS = (*LocalFS)(nil)
 
+// LocalFS is a plugins.FS that allows accessing files on the local file system.
 type LocalFS struct {
-	m        map[string]*LocalFile
+	// m is a map of relative file paths that can be accessed on the local filesystem.
+	// The path separator must be os-specific.
+	m map[string]*LocalFile
+
+	// basePath is the basePath that will be prepended to all the files (in m map) before accessing them.
 	basePath string
 }
 
+// NewLocalFS returns a new LocalFS that can access the specified files in the specified base path.
+// Both the map keys and basePath should use the os-specific path separator for Open() to work properly.
 func NewLocalFS(m map[string]struct{}, basePath string) LocalFS {
 	pfs := make(map[string]*LocalFile, len(m))
 	for k := range m {
@@ -31,6 +38,8 @@ func NewLocalFS(m map[string]struct{}, basePath string) LocalFS {
 	}
 }
 
+// Open opens the specified file on the local filesystem, and returns the corresponding fs.File.
+// If a nil error is returned, the caller should take care of closing the returned file.
 func (f LocalFS) Open(name string) (fs.File, error) {
 	cleanPath, err := util.CleanRelativePath(name)
 	if err != nil {
@@ -53,10 +62,13 @@ func (f LocalFS) Open(name string) (fs.File, error) {
 	return nil, ErrFileNotExist
 }
 
+// Base returns the base path for the LocalFS.
 func (f LocalFS) Base() string {
 	return f.basePath
 }
 
+// Files returns a slice of all the file paths in the LocalFS relative to the base path.
+// The returned strings use the same path separator as the
 func (f LocalFS) Files() []string {
 	var files []string
 	for p := range f.m {
@@ -72,11 +84,14 @@ func (f LocalFS) Files() []string {
 
 var _ fs.File = (*LocalFile)(nil)
 
+// LocalFile implements a fs.File for accessing the local filesystem.
 type LocalFile struct {
 	f    *os.File
 	path string
 }
 
+// Stat returns a FileInfo describing the named file.
+// It returns ErrFileNotExist if the file does not exist, or ErrPluginFileRead if another error occurs.
 func (p *LocalFile) Stat() (fs.FileInfo, error) {
 	fi, err := os.Stat(p.path)
 	if err != nil {
@@ -88,7 +103,19 @@ func (p *LocalFile) Stat() (fs.FileInfo, error) {
 	return fi, nil
 }
 
-func (p *LocalFile) Read(bytes []byte) (int, error) {
+// Read reads up to len(b) bytes from the File and stores them in b.
+// It returns the number of bytes read and any error encountered.
+// At end of file, Read returns 0, io.EOF.
+// If the file is already open, it is opened again, without closing it first.
+// The file is not closed at the end of the read operation. If a non-nil error is returned, it
+// must be manually closed by the caller by calling Close().
+func (p *LocalFile) Read(b []byte) (int, error) {
+	if p.f != nil {
+		// File is already open, Read() can be called more than once.
+		// io.EOF is returned if the file has been read entirely.
+		return p.f.Read(b)
+	}
+
 	var err error
 	p.f, err = os.Open(p.path)
 	if err != nil {
@@ -97,9 +124,12 @@ func (p *LocalFile) Read(bytes []byte) (int, error) {
 		}
 		return 0, ErrPluginFileRead
 	}
-	return p.f.Read(bytes)
+	return p.f.Read(b)
 }
 
+// Close closes the file.
+// If the file was never open, nil is returned.
+// If the file is already closed, an error is returned.
 func (p *LocalFile) Close() error {
 	if p.f != nil {
 		return p.f.Close()

pkg/plugins/localfiles_test.go

@@ -0,0 +1,143 @@
+package plugins
+
+import (
+	"io"
+	"os"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+type tempFileScenario struct {
+	filePath string
+}
+
+func (s tempFileScenario) cleanup() error {
+	return os.Remove(s.filePath)
+}
+
+func (s tempFileScenario) newLocalFile() LocalFile {
+	return LocalFile{path: s.filePath}
+}
+
+func newTempFileScenario() (tempFileScenario, error) {
+	tf, err := os.CreateTemp(os.TempDir(), "*")
+	if err != nil {
+		return tempFileScenario{}, err
+	}
+	defer tf.Close() //nolint
+	if _, err := tf.Write([]byte("hello\n")); err != nil {
+		return tempFileScenario{}, err
+	}
+	return tempFileScenario{
+		filePath: tf.Name(),
+	}, nil
+}
+
+func newTempFileScenarioForTest(t *testing.T) tempFileScenario {
+	s, err := newTempFileScenario()
+	require.NoError(t, err)
+	t.Cleanup(func() {
+		require.NoError(t, s.cleanup())
+	})
+	return s
+}
+
+func TestLocalFile_Read(t *testing.T) {
+	t.Run("not exists", func(t *testing.T) {
+		var out []byte
+		f := LocalFile{path: "does not exist"}
+		n, err := f.Read(out)
+		require.Zero(t, n)
+		require.Equal(t, ErrFileNotExist, err)
+	})
+
+	t.Run("read", func(t *testing.T) {
+		t.Run("extra", func(t *testing.T) {
+			s := newTempFileScenarioForTest(t)
+			f := s.newLocalFile()
+
+			const bufSize = 512
+			out := make([]byte, bufSize)
+			n, err := f.Read(out)
+			require.NoError(t, err)
+			require.NoError(t, f.Close())
+			const exp = "hello\n"
+			require.Equal(t, len(exp), n)
+			require.Equal(t, []byte(exp), out[:len(exp)])
+			require.Equal(t, make([]byte, bufSize-len(exp)), out[len(exp):])
+		})
+
+		t.Run("empty", func(t *testing.T) {
+			s := newTempFileScenarioForTest(t)
+			f := s.newLocalFile()
+
+			var out []byte
+			n, err := f.Read(out)
+			require.NoError(t, err)
+			require.NoError(t, f.Close())
+			require.Zero(t, n)
+			require.Empty(t, out)
+		})
+
+		t.Run("multiple", func(t *testing.T) {
+			s := newTempFileScenarioForTest(t)
+			f := s.newLocalFile()
+
+			a := make([]byte, 2)
+			b := make([]byte, 3)
+			c := make([]byte, 2)
+
+			t.Cleanup(func() {
+				require.NoError(t, f.Close())
+			})
+
+			n, err := f.Read(a)
+			require.NoError(t, err)
+			require.Equal(t, 2, n)
+			require.Equal(t, []byte("he"), a)
+
+			n, err = f.Read(b)
+			require.NoError(t, err)
+			require.Equal(t, 3, n)
+			require.Equal(t, []byte("llo"), b)
+
+			n, err = f.Read(c)
+			require.NoError(t, err)
+			require.Equal(t, 1, n)
+			require.Equal(t, []byte{'\n', 0}, c)
+
+			n, err = f.Read(c)
+			require.Zero(t, n)
+			require.Equal(t, io.EOF, err)
+		})
+	})
+}
+
+func TestLocalFile_Close(t *testing.T) {
+	t.Run("once after read", func(t *testing.T) {
+		s := newTempFileScenarioForTest(t)
+		f := s.newLocalFile()
+
+		_, err := f.Read(nil)
+		require.NoError(t, err)
+		require.NoError(t, f.Close())
+	})
+
+	t.Run("never opened", func(t *testing.T) {
+		s := newTempFileScenarioForTest(t)
+		f := s.newLocalFile()
+
+		require.NoError(t, f.Close())
+	})
+
+	t.Run("twice", func(t *testing.T) {
+		s := newTempFileScenarioForTest(t)
+		f := s.newLocalFile()
+
+		_, err := f.Read(nil)
+		require.NoError(t, err)
+		require.NoError(t, f.Close())
+		require.Error(t, f.Close())
+	})
+}

pkg/plugins/manager/loader/loader.go

@@ -141,12 +141,16 @@ func (l *Loader) loadPlugins(ctx context.Context, src plugins.PluginSource, foun
 		// verify module.js exists for SystemJS to load.
 		// CDN plugins can be loaded with plugin.json only, so do not warn for those.
 		if !plugin.IsRenderer() && !plugin.IsCorePlugin() {
-			_, err := plugin.FS.Open("module.js")
+			f, err := plugin.FS.Open("module.js")
 			if err != nil {
 				if errors.Is(err, plugins.ErrFileNotExist) {
 					l.log.Warn("Plugin missing module.js", "pluginID", plugin.ID,
 						"warning", "Missing module.js, If you loaded this plugin from git, make sure to compile it.")
 				}
+			} else if f != nil {
+				if err := f.Close(); err != nil {
+					l.log.Warn("Could not close module.js", "pluginID", plugin.ID, "err", err)
+				}
 			}
 		}